fix uninitialized read in tests #889

PiRK commented at 5:53 PM on January 31, 2021: contributor

I tried compiling secp256k1 with clang and -Wconditional-uninitialized and got the following warning:

$ make check
  CC       src/tests-tests.o
src/tests.c:4336:15: warning: variable 'recid' may be uninitialized when used here [-Wconditional-uninitialized]
        CHECK(recid >= 0 && recid < 4);
              ^~~~~
./src/util.h:54:18: note: expanded from macro 'CHECK'
    if (EXPECT(!(cond), 0)) { \
                 ^~~~
./src/util.h:41:39: note: expanded from macro 'EXPECT'
#define EXPECT(x,c) __builtin_expect((x),(c))
                                      ^
src/tests.c:4327:14: note: initialize the variable 'recid' to silence this warning
    int recid;
             ^
              = 0
1 warning generated.
  CCLD     tests
make  check-TESTS

This initializes recid and adds the -Wconditional-uninitialized flag when building with clang.

elichai commented at 6:52 PM on January 31, 2021: contributor

I think this is a false positive, if getrec is 1 we pass a pointer to recid into random_sign and inside we write into it, and then we only call CHECK if getrec is 1, so it shouldn't call CHECK on an uninitialized recid

in src/tests.c:4327 in 8566ae1bfc outdated

4323 | @@ -4324,7 +4324,7 @@ void test_ecdsa_sign_verify(void) {
4324 |      secp256k1_scalar one;
4325 |      secp256k1_scalar msg, key;
4326 |      secp256k1_scalar sigr, sigs;
4327 | -    int recid;
4328 | +    int recid = 0;

real-or-random commented at 9:24 AM on February 1, 2021:

Can we set this init to -1 instead to make sure that the CHECK below will fail if recid is never written to?

in configure.ac:75 in 8566ae1bfc outdated

  69 | @@ -70,6 +70,9 @@ esac
  70 |  CFLAGS="-W $CFLAGS"
  71 |  
  72 |  warn_CFLAGS="-std=c89 -pedantic -Wall -Wextra -Wcast-align -Wnested-externs -Wshadow -Wstrict-prototypes -Wundef -Wno-unused-function -Wno-long-long -Wno-overlength-strings"
  73 | +if test x"$CC" = x"clang"; then
  74 | +    warn_CFLAGS="$warn_CFLAGS -Wconditional-uninitialized"
  75 | +fi

real-or-random commented at 9:44 AM on February 1, 2021:

This is somewhat coarse: It won't match cases like CC=clang-9 and it could match old clang versions which don't support the flag.

The proper way is to check support for the flag directly. Two possible approaches can be found in some of our PRs:

simple direct way: https://github.com/bitcoin-core/secp256k1/pull/833/files#diff-49473dca262eeab3b4a43002adb08b4db31020d190caaad1594b47f1d5daa810
using autoconf "libraries": see #744

In this case, I'd prefer the simple direct way (but I may be biased here because it's my PR).

Alternatively, feel free to drop this commit and not add the warning at all.

real-or-random commented at 9:45 AM on February 1, 2021: contributor

So yes, this is a false positive but it's still a good idea to "fix" it. Ideally. the test should also check that recid is indeed written to by secp256k1_ecdsa_sig_sign.

PiRK force-pushed on Feb 1, 2021

PiRK commented at 12:26 PM on February 1, 2021: contributor

Thanks for your reviews. I have updated the commits with your feedback.

real-or-random commented at 1:36 PM on February 1, 2021: contributor

Thanks for the quick update. This looks good.

ACK 64ebb155a05f854df8010fe64f2b9d33d87d9a48 changes looks good but I haven't tested them

real-or-random approved

gmaxwell commented at 6:00 AM on February 2, 2021: contributor

It would probably be a good idea to VG_UNDEF() that variable.

Unnecessarily initialization of variables is usually not the best practice. It causes actual harm because it undermines the sensitivity of undefined behaviour checking -- e.g. if there is some change to the code and the dummy initialization which isn't supposed to get used starts to get used, there isn't any way to detect it where otherwise with valgrind/ubsan it would get flagged.

Because this is in a test you have the option of VG_UNDEF(&recid,sizeof()) it to restore the lost sensitivity at least for valgrind builds running in valgrind... and since this seems to allow turning on an additional warning it might be worth it. OTOH, if the warning produces additional false positives in the future it'll probably have to get turned off.

It's probably worth testing this with multiple clang versions-- in the past compilers have sometimes introduced new warnings in forms that spewed false positives and then improved them. If the codebase (with all the modules enabled) gets a lot of falses with an older but popular clang then it'll probably need to get compiler version restricted.

I tested this PR with ARM8 ubuntu clang 10 and all modules enabled and got no warnings.

in src/tests.c:4331 in 64ebb155a0 outdated

4323 | @@ -4324,7 +4324,7 @@ void test_ecdsa_sign_verify(void) {
4324 |      secp256k1_scalar one;
4325 |      secp256k1_scalar msg, key;
4326 |      secp256k1_scalar sigr, sigs;
4327 | -    int recid;
4328 | +    int recid = -1;
4329 |      int getrec;
4330 |      random_scalar_order_test(&msg);

real-or-random commented at 10:26 AM on February 2, 2021:

@gmaxwell Is this your suggestion?

    int getrec;
    /* Initialize recid to suppress a false positive -Wconditional-uninitialized in clang.
       VG_UNDEF ensures that valgrind will still treat the variable as uninitialized. */
    int recid = -1; VG_UNDEF(&recid, sizeof(recid));
    random_scalar_order_test(&msg);

gmaxwell commented at 8:11 AM on February 4, 2021:

yep!

PiRK commented at 8:55 AM on February 4, 2021:

Thank you. I amended the first commit with your changes.

real-or-random commented at 10:32 AM on February 2, 2021: contributor

Fwiw, I had checked that the current PR doesn't give further warnings on

clang version 11.0.1
Target: x86_64-pc-linux-gnu

real-or-random commented at 10:39 AM on February 2, 2021: contributor

I wanted to write "let's ask Cirrus" about clang 9 but this PR didn't get a build. I'll try closing and opening this but I don't think it'll help.

real-or-random closed this on Feb 2, 2021

real-or-random reopened this on Feb 2, 2021

real-or-random commented at 12:35 PM on February 2, 2021: contributor

Oh I see the issue now: This PR is not rebased on the most recent master, so it just not have the new .cirrus.yml. @PiRK Can you rebase this on the current master? Sorry for all the fuss about this small change, we recently switched our CI from Travis to Cirrus and this hits us here.

real-or-random closed this on Feb 2, 2021

real-or-random reopened this on Feb 2, 2021

PiRK commented at 5:54 PM on February 2, 2021: contributor

Np, I will rebase it tomorrow. I'm on the road today.

PiRK force-pushed on Feb 3, 2021

initialize variable in tests

This was detected while running the tests with the `-Wconditional-uninitialized` flag

```
./autogen.sh
CC=clang CFLAGS="-Wconditional-uninitialized" ./configure
make check
```

The resulting warning is a false positive, but setting the value to -1
ensures that the CHECK below will fail if recid is never written to.

3d2cf6c5bd

print warnings for conditional-uninitialized

This compiler flag is available for clang but not gcc.

Test plan:

```
autogen.sh
./configure
make check
CC=clang ./configure
make check
```

If a variable is used uninitialized, the warning should look something
like:
```
  CC       src/tests-tests.o
src/tests.c:4336:15: warning: variable 'recid' may be uninitialized when used here [-Wconditional-uninitialized]
        CHECK(recid >= 0 && recid < 4);
              ^~~~~
./src/util.h:54:18: note: expanded from macro 'CHECK'
    if (EXPECT(!(cond), 0)) { \
                 ^~~~
./src/util.h:41:39: note: expanded from macro 'EXPECT'
                                      ^
src/tests.c:4327:14: note: initialize the variable 'recid' to silence this warning
    int recid;
             ^
              = 0
1 warning generated.
```

99a1cfec17

PiRK force-pushed on Feb 4, 2021

real-or-random closed this on Mar 18, 2021

real-or-random commented at 8:37 AM on March 18, 2021: contributor

Closing and reopening to trigger Cirrus CI, which now really uses clang (version 7).

edit: Oh, Cirrus simply looks up the old results in its cache... This shouldn't be the case since we changed some settings in between.

real-or-random reopened this on Mar 18, 2021

real-or-random cross-referenced this on Mar 18, 2021 from issue A way to manually retrigger a build by real-or-random

real-or-random approved

real-or-random commented at 2:06 PM on March 23, 2021: contributor

ACK 99a1cfec1740a914aa416a87fd0acbde5426b969 code inspection

We now know that there are no additional warnings on clang 7 (Cirrus), clang 10 (@gmaxwell) and clang 11 (me). This should be enough.

So even there are two CI hiccups that could be solved by rebasing (the issue here is that .cirrus.yml is wrongly merged), these failures should disappear on master after merging. So I think we shouldn't bother @PiRK with a further rebase and just merge this.

jonasnick approved

jonasnick commented at 12:51 PM on April 7, 2021: contributor

No further warnings with clang version 11.1.1

ACK 99a1cfec1740a914aa416a87fd0acbde5426b969

jonasnick merged this on Apr 7, 2021

jonasnick closed this on Apr 7, 2021

deadalnix referenced this in commit b0d8c607a6 on Apr 8, 2021

deadalnix referenced this in commit e250d103bf on Apr 9, 2021

real-or-random cross-referenced this on May 4, 2021 from issue Have ge_set_gej_var, gej_double_var and ge_set_all_gej_var initialize all fields of their outputs. by roconnor-blockstream

jonasnick cross-referenced this on Jun 14, 2021 from issue Upstream PRs 831, 907, 903, 889, 918, 906, 928, 922, 933, Merge bitcoin-core/secp256k1#936: Fix gen_context/ASM build on ARM, 925, 937, 926, Merge bitcoin-core/secp256k1#940: contrib: Explain explicit header guards, 850, 930, 941, 846, 947, 662, 950 by jonasnick