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

hebasto · 2023-11-14T16:59:21Z

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 · 2023-11-14T16:59:24Z

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.

dergoegge · 2023-11-14T17:02:58Z

Concept ACK

Thanks! will review soon

hebasto · 2023-11-14T22:48:21Z

Converted to a draft, ~~as I'm not sure how to suppress new UBSan reports~~.

hebasto · 2024-01-06T23:05:01Z

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

Friendly ping @dergoegge :)

dergoegge

tACK a041915

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

hebasto · 2024-01-13T09:59:41Z

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

Squashed.

dergoegge

reACK e39bae1

test/sanitizer_suppressions/ubsan

Also a new UBSan suppression has been added.

fanquake

ACK cbea49c

real-or-random · 2024-01-25T16:07:33Z

configure.ac

@@ -1946,6 +1947,9 @@ CPPFLAGS_TEMP="$CPPFLAGS"
 unset CPPFLAGS
 CPPFLAGS="$CPPFLAGS_TEMP"

+if test -n "$use_sanitizers"; then
+  export SECP_CFLAGS="$SECP_CFLAGS $SANITIZER_CFLAGS"


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...

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.

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.

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.

real-or-random · 2024-01-25T16:09:34Z

configure.ac

@@ -2006,7 +2010,7 @@ echo "  target os       = $host_os"
 echo "  build os        = $build_os"
 echo
 echo "  CC              = $CC"
-echo "  CFLAGS          = $PTHREAD_CFLAGS $CFLAGS"
+echo "  CFLAGS          = $PTHREAD_CFLAGS $SANITIZER_CFLAGS $CFLAGS"


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?

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

reACK cbea49c

DrahtBot added the Build system label Nov 14, 2023

DrahtBot added the CI failed label Nov 14, 2023

This was referenced Nov 14, 2023

Silent payment index (for light wallets and consistency check) #28241

Draft

Silent Payments: Implement BIP352 #28122

Draft

hebasto force-pushed the 231114-sanitize branch 4 times, most recently from a6de1d1 to 9937fcb Compare November 14, 2023 22:17

hebasto marked this pull request as draft November 14, 2023 22:46

This was referenced Nov 15, 2023

build: Fix LTO functionality #28876

Closed

test: migrate to some per-symbol ubsan suppressions #28865

Merged

DrahtBot added the Needs rebase label Nov 15, 2023

hebasto force-pushed the 231114-sanitize branch from 9937fcb to 9065c11 Compare January 6, 2024 20:23

DrahtBot removed the Needs rebase label Jan 6, 2024

hebasto force-pushed the 231114-sanitize branch from 9065c11 to a041915 Compare January 6, 2024 21:55

hebasto marked this pull request as ready for review January 6, 2024 22:22

DrahtBot removed the CI failed label Jan 6, 2024

DrahtBot mentioned this pull request Jan 6, 2024

Stratum v2 Template Provider (take 2) #28983

Closed

30 tasks

dergoegge approved these changes Jan 10, 2024

View reviewed changes

fanquake requested a review from TheCharlatan January 10, 2024 16:19

hebasto force-pushed the 231114-sanitize branch from a041915 to e39bae1 Compare January 13, 2024 09:58

DrahtBot mentioned this pull request Jan 16, 2024

Silent Payments: sending #28201

Draft

dergoegge approved these changes Jan 18, 2024

View reviewed changes

real-or-random reviewed Jan 18, 2024

View reviewed changes

test/sanitizer_suppressions/ubsan Outdated Show resolved Hide resolved

build: Pass sanitize flags to instrument libsecp256k1 code

cbea49c

Also a new UBSan suppression has been added.

hebasto force-pushed the 231114-sanitize branch from e39bae1 to cbea49c Compare January 19, 2024 10:09

fanquake approved these changes Jan 25, 2024

View reviewed changes

DrahtBot requested a review from dergoegge January 25, 2024 15:44

real-or-random reviewed Jan 25, 2024

View reviewed changes

dergoegge approved these changes Jan 25, 2024

View reviewed changes

fanquake merged commit e3b68b3 into bitcoin:master Jan 26, 2024
16 checks passed

hebasto deleted the 231114-sanitize branch January 26, 2024 12:00

dergoegge mentioned this pull request Jan 26, 2024

Add secp covering inputs bitcoin-core/qa-assets#168

Merged

hebasto mentioned this pull request Jan 26, 2024

cmake: Regular rebasing of the cmake-staging branch hebasto/bitcoin#85

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

hebasto commented Nov 14, 2023

DrahtBot commented Nov 14, 2023 •

edited

dergoegge commented Nov 14, 2023

hebasto commented Nov 14, 2023 •

edited

hebasto commented Jan 6, 2024

dergoegge left a comment

hebasto commented Jan 13, 2024

dergoegge left a comment

fanquake left a comment

real-or-random Jan 25, 2024

fanquake Jan 25, 2024 •

edited

real-or-random Jan 25, 2024

fanquake Jan 26, 2024

real-or-random Jan 25, 2024

fanquake Jan 26, 2024

dergoegge left a comment

build: Pass sanitize flags to instrument libsecp256k1 code #28875

build: Pass sanitize flags to instrument libsecp256k1 code #28875

Conversation

hebasto commented Nov 14, 2023

DrahtBot commented Nov 14, 2023 • edited

Code Coverage

Reviews

Conflicts

dergoegge commented Nov 14, 2023

hebasto commented Nov 14, 2023 • edited

hebasto commented Jan 6, 2024

dergoegge left a comment

Choose a reason for hiding this comment

hebasto commented Jan 13, 2024

dergoegge left a comment

Choose a reason for hiding this comment

fanquake left a comment

Choose a reason for hiding this comment

real-or-random Jan 25, 2024

Choose a reason for hiding this comment

fanquake Jan 25, 2024 • edited

Choose a reason for hiding this comment

real-or-random Jan 25, 2024

Choose a reason for hiding this comment

fanquake Jan 26, 2024

Choose a reason for hiding this comment

real-or-random Jan 25, 2024

Choose a reason for hiding this comment

fanquake Jan 26, 2024

Choose a reason for hiding this comment

dergoegge left a comment

Choose a reason for hiding this comment

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

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

DrahtBot commented Nov 14, 2023 •

edited

hebasto commented Nov 14, 2023 •

edited

fanquake Jan 25, 2024 •

edited