Skip to content

Commit c297b01

Browse files
real-or-randomFabcien
authored andcommitted
[secp256k1] Update the CI docker to Debian Bullseye
Summary: Also includes a fix extracted from upstream: bitcoin-core/secp256k1@5d5c74a Partial backport of [[bitcoin-core/secp256k1#969 | secp256k1#969]]. ``` 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 at least for binaries produced by clang 11 (but not clang 7), valgrind complains about a branch on unitialized data in the recid variable 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 might be the case 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. ``` Test Plan: ninja check-secp256k1 Tested against cirrus on my personal github fork of the project. Reviewers: #bitcoin_abc, sdulfari, PiRK Reviewed By: #bitcoin_abc, sdulfari, PiRK Subscribers: PiRK Differential Revision: https://reviews.bitcoinabc.org/D12957
1 parent 449d01c commit c297b01

File tree

2 files changed

+8
-8
lines changed

2 files changed

+8
-8
lines changed
Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,14 +1,12 @@
1-
FROM debian:buster
1+
FROM debian:bullseye
22

33
RUN dpkg --add-architecture i386
44
RUN dpkg --add-architecture s390x
5-
RUN echo "deb http://deb.debian.org/debian buster-backports main" | tee -a /etc/apt/sources.list
65
RUN apt-get update
76

87
# dkpg-dev: to make pkg-config work in cross-builds
98
RUN apt-get install --no-install-recommends --no-upgrade -y \
10-
automake default-jdk dpkg-dev libssl-dev libtool make ninja-build pkg-config python3 qemu-user valgrind \
9+
automake cmake default-jdk dpkg-dev libssl-dev libtool make ninja-build pkg-config python3 qemu-user valgrind \
1110
gcc clang libc6-dbg \
1211
gcc-i686-linux-gnu libc6-dev-i386-cross libc6-dbg:i386 \
1312
gcc-s390x-linux-gnu libc6-dev-s390x-cross libc6-dbg:s390x
14-
RUN apt-get install -t buster-backports --no-install-recommends --no-upgrade -y cmake

src/secp256k1/src/tests.c

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -4545,17 +4545,19 @@ void test_ecdsa_sign_verify(void) {
45454545
secp256k1_scalar msg, key;
45464546
secp256k1_scalar sigr, sigs;
45474547
int getrec;
4548-
/* Initialize recid to suppress a false positive -Wconditional-uninitialized in clang.
4549-
VG_UNDEF ensures that valgrind will still treat the variable as uninitialized. */
4550-
int recid = -1; VG_UNDEF(&recid, sizeof(recid));
4548+
int recid;
45514549
random_scalar_order_test(&msg);
45524550
random_scalar_order_test(&key);
45534551
secp256k1_ecmult_gen(&ctx->ecmult_gen_ctx, &pubj, &key);
45544552
secp256k1_ge_set_gej(&pub, &pubj);
45554553
getrec = secp256k1_testrand_bits(1);
4556-
random_sign(&sigr, &sigs, &key, &msg, getrec?&recid:NULL);
4554+
/* The specific way in which this conditional is written sidesteps a potential bug in clang.
4555+
See the commit messages of the commit that introduced this comment for details. */
45574556
if (getrec) {
4557+
random_sign(&sigr, &sigs, &key, &msg, &recid);
45584558
CHECK(recid >= 0 && recid < 4);
4559+
} else {
4560+
random_sign(&sigr, &sigs, &key, &msg, NULL);
45594561
}
45604562
CHECK(secp256k1_ecdsa_sig_verify(&ctx->ecmult_ctx, &sigr, &sigs, &pub, &msg));
45614563
secp256k1_scalar_set_int(&one, 1);

0 commit comments

Comments
 (0)