Repository: bitcoin-core/secp256k1
Pull request #889 (2021-01-31 17:53:07)

fix uninitialized read in tests
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: 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`
PiRK: Thanks for your reviews. I have updated the commits with your feedback.
real-or-random: Thanks for the quick update. This looks good.

ACK 64ebb155a05f854df8010fe64f2b9d33d87d9a48 changes looks good but I haven't tested them
gmaxwell: 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,sizof()) 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.

real-or-random: 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: 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. 
PiRK: Np, I will rebase it tomorrow. I'm on the road today. 

Patch:
diff --git a/configure.ac b/configure.ac
index 451915c..d9a7ddb 100644
--- a/configure.ac
+++ b/configure.ac
@@ -79,6 +79,15 @@ AC_COMPILE_IFELSE([AC_LANG_SOURCE([[char foo;]])],
       CFLAGS="$saved_CFLAGS"
     ])
 
+saved_CFLAGS="$CFLAGS"
+CFLAGS="-Wconditional-uninitialized $CFLAGS"
+AC_MSG_CHECKING([if ${CC} supports -Wconditional-uninitialized])
+AC_COMPILE_IFELSE([AC_LANG_SOURCE([[char foo;]])],
+    [ AC_MSG_RESULT([yes]) ],
+    [ AC_MSG_RESULT([no])
+      CFLAGS="$saved_CFLAGS"
+    ])
+
 saved_CFLAGS="$CFLAGS"
 CFLAGS="-fvisibility=hidden $CFLAGS"
 AC_MSG_CHECKING([if ${CC} supports -fvisibility=hidden])


Test patch:
diff --git a/src/tests.c b/src/tests.c
index c2d5e28..4110bb9 100644
--- a/src/tests.c
+++ b/src/tests.c
@@ -4324,8 +4324,10 @@ void test_ecdsa_sign_verify(void) {
     secp256k1_scalar one;
     secp256k1_scalar msg, key;
     secp256k1_scalar sigr, sigs;
-    int recid;
     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);
     random_scalar_order_test(&key);
     secp256k1_ecmult_gen(&ctx->ecmult_gen_ctx, &pubj, &key);