Skip to content

Commit

Permalink
[secp256k1] Update the CI docker to Debian Bullseye
Browse files Browse the repository at this point in the history
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
  • Loading branch information
real-or-random authored and Fabcien committed Jan 6, 2023
1 parent dc940d8 commit f7e8b6d
Show file tree
Hide file tree
Showing 2 changed files with 8 additions and 8 deletions.
6 changes: 2 additions & 4 deletions ci/linux-debian.Dockerfile
Original file line number Diff line number Diff line change
@@ -1,14 +1,12 @@
FROM debian:buster
FROM debian:bullseye

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

# dkpg-dev: to make pkg-config work in cross-builds
RUN apt-get install --no-install-recommends --no-upgrade -y \
automake default-jdk dpkg-dev libssl-dev libtool make ninja-build pkg-config python3 qemu-user valgrind \
automake cmake default-jdk dpkg-dev libssl-dev libtool make ninja-build pkg-config python3 qemu-user valgrind \
gcc clang libc6-dbg \
gcc-i686-linux-gnu libc6-dev-i386-cross libc6-dbg:i386 \
gcc-s390x-linux-gnu libc6-dev-s390x-cross libc6-dbg:s390x
RUN apt-get install -t buster-backports --no-install-recommends --no-upgrade -y cmake
10 changes: 6 additions & 4 deletions src/tests.c
Original file line number Diff line number Diff line change
Expand Up @@ -4545,17 +4545,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);
Expand Down

0 comments on commit f7e8b6d

Please sign in to comment.