build: Pass sanitize flags to instrument `libsecp256k1` code #28875

pull hebasto wants to merge 1 commits into bitcoin:master from hebasto:231114-sanitize changing 2 files +7 −2

hebasto commented at 4:59 PM on November 14, 2023: member

This PR is a revived #27991 with an addressed comment.

Might be tested as follows:

$ ./autogen.sh && ./configure --enable-fuzz --with-sanitizers=fuzzer CC=clang-13 CXX=clang++-13
$ make clean > /dev/null && make
$ objdump --disassemble=secp256k1_xonly_pubkey_serialize src/test/fuzz/fuzz | grep __sanitizer_cov
 1953bd0:	e8 bb c6 05 ff       	call   9b0290 <__sanitizer_cov_trace_const_cmp8>
 1953d32:	e8 69 c4 05 ff       	call   9b01a0 <__sanitizer_cov_trace_pc_indir>
 1953d58:	e8 43 c4 05 ff       	call   9b01a0 <__sanitizer_cov_trace_pc_indir>
 1953d82:	e8 19 c4 05 ff       	call   9b01a0 <__sanitizer_cov_trace_pc_indir>

DrahtBot commented at 4:59 PM on November 14, 2023: contributor


The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.



Code Coverage

For detailed information about the code coverage, see the test coverage report.



Reviews

See the guideline for information on the review process.

Type Reviewers

ACK fanquake, dergoegge

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.



Conflicts

Reviewers, this pull request conflicts with the following ones:
- #28983 (Stratum v2 Template Provider (take 2) by Sjors)
- #28241 (Silent payment index (for light wallets and consistency check) by Sjors)
- #28201 (Silent Payments: sending by josibake)
- #28122 (Silent Payments: Implement BIP352 by josibake)
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
DrahtBot added the label Build system on Nov 14, 2023
dergoegge commented at 5:02 PM on November 14, 2023: member

Concept ACK

Thanks! will review soon
DrahtBot added the label CI failed on Nov 14, 2023
hebasto force-pushed on Nov 14, 2023
hebasto force-pushed on Nov 14, 2023
hebasto force-pushed on Nov 14, 2023
hebasto force-pushed on Nov 14, 2023
hebasto marked this as a draft on Nov 14, 2023
hebasto commented at 10:48 PM on November 14, 2023: member

Converted to a draft, ~as I'm not sure how to suppress new UBSan reports~.
DrahtBot added the label Needs rebase on Nov 15, 2023
hebasto force-pushed on Jan 6, 2024
DrahtBot removed the label Needs rebase on Jan 6, 2024
hebasto force-pushed on Jan 6, 2024
hebasto marked this as ready for review on Jan 6, 2024
DrahtBot removed the label CI failed on Jan 6, 2024
hebasto commented at 11:05 PM on January 6, 2024: member

The CI has been fixed and this PR is ready for review now.

Friendly ping @dergoegge :)
dergoegge approved
dergoegge commented at 4:06 PM on January 10, 2024: member

tACK a0419151541fd5698cb4ddb4754395a22ec749a6

It might be better to reverse the order of the commits or squash them to avoid bisect failures in between.
fanquake requested review from TheCharlatan on Jan 10, 2024
hebasto force-pushed on Jan 13, 2024
hebasto commented at 9:59 AM on January 13, 2024: member

It might be better to reverse the order of the commits or squash them to avoid bisect failures in between.

Squashed.
dergoegge approved
dergoegge commented at 2:33 PM on January 18, 2024: member

reACK e39bae122c79b8dfaa5f7e02ff199dc8c2051d8a
in test/sanitizer_suppressions/ubsan:69 in e39bae122c outdated
```
  65 | @@ -66,6 +66,7 @@ implicit-integer-sign-change:verify_flags
  66 |  implicit-integer-sign-change:EvalScript
  67 |  implicit-integer-sign-change:serialize.h
  68 |  implicit-signed-integer-truncation:crypto/
  69 | +implicit-signed-integer-truncation,implicit-integer-sign-change:secp256k1_modinv64_posdivsteps_62_var
```
real-or-random commented at 4:42 PM on January 18, 2024:

Shouldn't this be covered by https://github.com/hebasto/bitcoin/blob/e39bae122c79b8dfaa5f7e02ff199dc8c2051d8a/test/sanitizer_suppressions/ubsan#L22-L28 already?

hebasto commented at 4:43 PM on January 18, 2024:

It expects exactly "implicit-signed-integer-truncation,implicit-integer-sign-change".

real-or-random commented at 4:57 PM on January 18, 2024:

I see. I think it still makes sense to move the line up so that it appears below # Suppressions in dependencies that are developed outside this repository.

hebasto commented at 10:09 AM on January 19, 2024:

Sure. Done.
build: Pass sanitize flags to instrument `libsecp256k1` code
Also a new UBSan suppression has been added.
cbea49c0d3
hebasto force-pushed on Jan 19, 2024
fanquake approved
fanquake commented at 3:44 PM on January 25, 2024: member

ACK cbea49c0d32badb975fbf22d44f8e25cc7972af7
DrahtBot requested review from dergoegge on Jan 25, 2024
in configure.ac:1951 in cbea49c0d3
```
1946 | @@ -1946,6 +1947,9 @@ CPPFLAGS_TEMP="$CPPFLAGS"
1947 |  unset CPPFLAGS
1948 |  CPPFLAGS="$CPPFLAGS_TEMP"
1949 |  
1950 | +if test -n "$use_sanitizers"; then
1951 | +  export SECP_CFLAGS="$SECP_CFLAGS $SANITIZER_CFLAGS"
```
real-or-random commented at 4:07 PM on January 25, 2024:

My take on SECP_CFLAGS is that it's an internal of the libsecp256k1 build system, and users are not supposed to set it (and are not guaranteed that it does anything meaningful). The proper user variable is just CFLAGS.

You could either export CFLAGS="-g $SANITIZER_CFLAGS" temporarily, or perhaps add a CFLAGS arg to ac_configure_args.

We could also expose the SECP_CFLAGS "officially" in the lib. Then it should probably be renamed to SECP256K1_CFLAGS. But I think it's a bit arbitrary, given there's an official way to set CFLAGS (namely setting CFLAGS ;)).

There's also this, which may be an elegant solution or overkill, depending on your taste... https://www.gnu.org/software/autoconf-archive/ax_subdirs_configure.html

Sorry for noticing this only now in a second review...

fanquake commented at 4:14 PM on January 25, 2024:

I was thinking about the best way to handle this, and my current take was that given this is only when we are using sanitizers, and we are a somewhat non-standard/more-tightly-coupled users of secp in any case, I wasn't too worried. Happy for any other approach that achieves the same thing, but also don't want secp to expose additional (non-standard) flags just for us.

edit: we have looked at the subdirs_configure before, and iirc it didn't work quite how we wanted for various reasons.

real-or-random commented at 4:20 PM on January 25, 2024:

I wasn't too worried.

Yep, I agree, any approach is ok in the end. My feeling is just that setting SECP_CFLAGS is not the cleanest choice and more likely than my other suggestions to cause breaks in the future.

fanquake commented at 11:26 AM on January 26, 2024:

My feeling is just that setting SECP_CFLAGS is not the cleanest choice and more likely than my other suggestions to cause breaks in the future.

We can accept that possibility for now, and at the next subtree update (if/when things break), we'll take another look.
in configure.ac:2013 in cbea49c0d3
```
2009 | @@ -2006,7 +2010,7 @@ echo "  target os       = $host_os"
2010 |  echo "  build os        = $build_os"
2011 |  echo
2012 |  echo "  CC              = $CC"
2013 | -echo "  CFLAGS          = $PTHREAD_CFLAGS $CFLAGS"
2014 | +echo "  CFLAGS          = $PTHREAD_CFLAGS $SANITIZER_CFLAGS $CFLAGS"
```
real-or-random commented at 4:09 PM on January 25, 2024:

Serious question: What is this line supposed to print, given that there are also ZMQ_CFLAGS, BDB_CFLAGS and EVENT_CFLAGS? One could say that the libsecp256k1 configure output already prints its CFLAGS, so there's no need to cover that here, but I don't know because I don't know what the purpose of this line is. Where else do we need CFLAGS?

fanquake commented at 11:26 AM on January 26, 2024:

These lines have never been super consistent, but are meant to be a rough overview of the (interesting) flags being used for compilation. I think you're correct in that printing secp flags (again) here would be overkill. For any other flags, I don't think they are very meaningful for most builders. I think it's also unlikely we'll significantly change anything here too much further, given that this is going to be completely redone with CMake.
dergoegge approved
dergoegge commented at 5:07 PM on January 25, 2024: member

reACK cbea49c0d32badb975fbf22d44f8e25cc7972af7
fanquake merged this on Jan 26, 2024
fanquake closed this on Jan 26, 2024
hebasto deleted the branch on Jan 26, 2024
bitcoin locked this on Jan 25, 2025