From 892f0ce87ffc334ed13a24540d2a224dc545748c Mon Sep 17 00:00:00 2001 From: Tim Ruffing Date: Thu, 19 Aug 2021 12:22:36 +0200 Subject: [PATCH] tests: Rewrite code to circument potential bug in clang clang 7 to 11 (and maybe earlier versions) warn about recid being potentially unitiliazed in "CHECK(recid >= 0 [...]", which was mitigated in commit 3d2cf6c5bd35b0d72716b47bdd7e3892388aafc4 by initializing recid to make clang happy but VG_UNDEF'ing the variable after initializiation in order to ensure valgrind's memcheck analysis will still be sound and complain if recid is not actually written to when creating a signature. However, it turns out that valgrind complains about the binary branching on unitialized recid in that line before *and* after the aforementioned commit. While the complaint after the commit could be spurious (clang knows that recid is initialized, so it's fine to access it even though the access is stupid), the complaint before the commit indicates a real problem: it seems that clang is performing a wrong optimization that leads to a situation where recid is really not guaranteed to be initialized when it's accessed. As a result, clang warns about this and generates code that just accesses the variable. I'm not going to bother with this further because this is fixed in clang 12 and the problem is just in our test code, not in the tested code. This commit rewrites the code in a way that groups the signing together with the CHECK such that it's very easy to figure out for clang that recid will be initialized properly. This seems to circument the issue. --- src/tests.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/src/tests.c b/src/tests.c index 99d9468e29..ce8df45a29 100644 --- a/src/tests.c +++ b/src/tests.c @@ -5262,17 +5262,19 @@ void test_ecdsa_sign_verify(void) { secp256k1_scalar msg, key; secp256k1_scalar sigr, sigs; 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)); + int recid; random_scalar_order_test(&msg); random_scalar_order_test(&key); secp256k1_ecmult_gen(&ctx->ecmult_gen_ctx, &pubj, &key); secp256k1_ge_set_gej(&pub, &pubj); getrec = secp256k1_testrand_bits(1); - random_sign(&sigr, &sigs, &key, &msg, getrec?&recid:NULL); + /* The specific way in which this conditional is written sidesteps a potential bug in clang. + See the commit messages of the commit that introduced this comment for details. */ if (getrec) { + random_sign(&sigr, &sigs, &key, &msg, &recid); CHECK(recid >= 0 && recid < 4); + } else { + random_sign(&sigr, &sigs, &key, &msg, NULL); } CHECK(secp256k1_ecdsa_sig_verify(&ctx->ecmult_ctx, &sigr, &sigs, &pub, &msg)); secp256k1_scalar_set_int(&one, 1);