fix uninitialized read in tests #889

pull PiRK wants to merge 2 commits into bitcoin-core:master from PiRK:uninitialized_reads changing 2 files +12 −1
  1. 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:

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

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

  2. 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
  3. 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?
  4. 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:

    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.

  5. 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.
  6. PiRK force-pushed on Feb 1, 2021
  7. PiRK commented at 12:26 pm on February 1, 2021: contributor
    Thanks for your reviews. I have updated the commits with your feedback.
  8. 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

  9. real-or-random approved
  10. real-or-random approved
  11. 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.

  12. 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?

    0    int getrec;
1    /* Initialize recid to suppress a false positive -Wconditional-uninitialized in clang.
2       VG_UNDEF ensures that valgrind will still treat the variable as uninitialized. */
3    int recid = -1; VG_UNDEF(&recid, sizeof(recid));
4    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.
  13. 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

    0clang version 11.0.1
1Target: x86_64-pc-linux-gnu
  14. 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.
  15. real-or-random closed this on Feb 2, 2021
  16. real-or-random reopened this on Feb 2, 2021
  17. 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.
  18. real-or-random closed this on Feb 2, 2021
  19. real-or-random reopened this on Feb 2, 2021
  20. PiRK commented at 5:54 pm on February 2, 2021: contributor
    Np, I will rebase it tomorrow. I’m on the road today.
  21. PiRK force-pushed on Feb 3, 2021
  22. 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
  23. 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
  24. PiRK force-pushed on Feb 4, 2021
  25. real-or-random closed this on Mar 18, 2021
  26. 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.

  27. real-or-random reopened this on Mar 18, 2021
  28. real-or-random cross-referenced this on Mar 18, 2021 from issue A way to manually retrigger a build by real-or-random
  29. real-or-random approved
  30. 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.

  31. jonasnick approved
  32. jonasnick commented at 12:51 pm on April 7, 2021: contributor

    No further warnings with clang version 11.1.1

    ACK 99a1cfec1740a914aa416a87fd0acbde5426b969

  33. jonasnick merged this on Apr 7, 2021
  34. jonasnick closed this on Apr 7, 2021
  35. deadalnix referenced this in commit b0d8c607a6 on Apr 8, 2021
  36. deadalnix referenced this in commit e250d103bf on Apr 9, 2021
  37. 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
  38. 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


PiRK elichai real-or-random gmaxwell jonasnick

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin-core/secp256k1. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-11-24 12:15 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me