fix uninitialized read in tests

[PiRK]

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

[real-or-random]

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]

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

[real-or-random]

Thanks for the quick update. This looks good.

ACK 64ebb15 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,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.

[real-or-random]

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]

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.

[real-or-random]

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]

ACK 99a1cfe 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]

No further warnings with clang version 11.1.1

ACK 99a1cfe