Skip to content

Conversation

fanquake
Copy link
Member

Now that the symbolizer is back in play, suppressions can once-again be targeted to functions, rather than file-wide.

Now that the symbolizer is back in play, suppressions can once-again be
targeted to functions, rather than file-wide.
@DrahtBot
Copy link
Contributor

DrahtBot commented Jul 25, 2023

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

Reviews

See the guideline for information on the review process.

Type Reviewers
ACK MarcoFalke, hebasto

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

@maflcko
Copy link
Member

maflcko commented Jul 25, 2023

lgtm ACK d0c6cc4

Copy link
Member

@hebasto hebasto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK d0c6cc4

@fanquake fanquake merged commit 4517e2f into bitcoin:master Jul 26, 2023
@fanquake fanquake deleted the suppressions_class_func branch July 26, 2023 08:54
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Aug 9, 2023
…ame` should be used

d0c6cc4 suppressions: note that 'type:ClassName::MethodName' should be used (fanquake)

Pull request description:

  Now that the symbolizer is back in play, suppressions can once-again be targeted to functions, rather than file-wide.

ACKs for top commit:
  MarcoFalke:
    lgtm ACK d0c6cc4
  hebasto:
    ACK d0c6cc4

Tree-SHA512: fb65398eae18a6ebc5f8414275c568cf2664ab5357c2b3160f3bf285b67bc3af788225c5dba3c824c0e098627789450bec775375f52529d71c6ef700a9632d65
@maflcko
Copy link
Member

maflcko commented Nov 1, 2023

Not sure why the symbolizer doesn't work on Fedora, but I guess this should be reverted? Steps to reproduce on a fresh install of Fedora 39:

dnf install gcc-c++ libtool make autoconf automake python3 clang llvm libcxx-devel libcxxabi-devel git ccache libevent-devel boost-devel -y && git clone https://github.com/bitcoin/bitcoin.git  --depth=1 ./bitcoin-core && cd bitcoin-core &&  ./autogen.sh &&   ./configure CC=clang CXX=clang++ --with-sanitizers=fuzzer,integer,undefined --enable-fuzz && make -j $(nproc) && ./test/fuzz/test_runner.py -j 1 -l DEBUG /tmp/empty_folder/ addition_overflow
Run addition_overflow with args ['/bitcoin-core/src/test/fuzz/fuzz', '-runs=1', PosixPath('/tmp/empty_folder/addition_overflow')]INFO: Running with entropic power schedule (0xFF, 100).
INFO: Seed: 2274899552
INFO: Loaded 1 modules   (426135 inline 8-bit counters): 426135 [0x55b591c25f98, 0x55b591c8e02f), 
INFO: Loaded 1 PC tables (426135 PCs): 426135 [0x55b591c8e030,0x55b59230e9a0), 
INFO:        0 files found in /tmp/empty_folder/addition_overflow
INFO: -max_len is not provided; libFuzzer will not generate inputs larger than 4096 bytes
test/fuzz/FuzzedDataProvider.h:212:47: runtime error: unsigned integer overflow: 9223372036854775807 - 9223372036854775808 cannot be represented in type 'uint64_t' (aka 'unsigned long')
    #0 0x55b5900cc028  (/bitcoin-core/src/test/fuzz/fuzz+0x121e028) (BuildId: 306b8873f90b44941bafa8f0ae2b1a11c835d541)
    #1 0x55b5900c7778  (/bitcoin-core/src/test/fuzz/fuzz+0x1219778) (BuildId: 306b8873f90b44941bafa8f0ae2b1a11c835d541)
    #2 0x55b590492f86  (/bitcoin-core/src/test/fuzz/fuzz+0x15e4f86) (BuildId: 306b8873f90b44941bafa8f0ae2b1a11c835d541)
    #3 0x55b590081e34  (/bitcoin-core/src/test/fuzz/fuzz+0x11d3e34) (BuildId: 306b8873f90b44941bafa8f0ae2b1a11c835d541)
    #4 0x55b590082be1  (/bitcoin-core/src/test/fuzz/fuzz+0x11d4be1) (BuildId: 306b8873f90b44941bafa8f0ae2b1a11c835d541)
    #5 0x55b590082fd6  (/bitcoin-core/src/test/fuzz/fuzz+0x11d4fd6) (BuildId: 306b8873f90b44941bafa8f0ae2b1a11c835d541)
    #6 0x55b59006ffec  (/bitcoin-core/src/test/fuzz/fuzz+0x11c1fec) (BuildId: 306b8873f90b44941bafa8f0ae2b1a11c835d541)
    #7 0x55b59009c556  (/bitcoin-core/src/test/fuzz/fuzz+0x11ee556) (BuildId: 306b8873f90b44941bafa8f0ae2b1a11c835d541)
    #8 0x7fc722965149  (/lib64/libc.so.6+0x28149) (BuildId: 7dd93cb9991a89f0ec53d9443a0b78ad952269bc)
    #9 0x7fc72296520a  (/lib64/libc.so.6+0x2820a) (BuildId: 7dd93cb9991a89f0ec53d9443a0b78ad952269bc)
    #10 0x55b590064ba4  (/bitcoin-core/src/test/fuzz/fuzz+0x11b6ba4) (BuildId: 306b8873f90b44941bafa8f0ae2b1a11c835d541)

SUMMARY: UndefinedBehaviorSanitizer: unsigned-integer-overflow test/fuzz/FuzzedDataProvider.h:212:47 in 
MS: 0 ; base unit: 0000000000000000000000000000000000000000


artifact_prefix='./'; Test unit written to ./crash-da39a3ee5e6b4b0d3255bfef95601890afd80709

@maflcko
Copy link
Member

maflcko commented Nov 6, 2023

Same on OpenSuse Tumbleweed:

zypper in -y libevent-devel boost-devel awk find bison gcc-c++ libtool make autoconf automake python3 clang llvm lbzip2 patch xz curl wget htop git vim ccache && git clone https://github.com/bitcoin/bitcoin.git --depth=1 ./bitcoin-core && cd bitcoin-core && ./autogen.sh && ./configure CC=clang CXX=clang++ --with-sanitizers=fuzzer,integer,undefined --enable-fuzz && make -j $(nproc) && ./test/fuzz/test_runner.py -j 1 -l DEBUG /tmp/empty_folder/ addition_overflow

fanquake added a commit to fanquake/bitcoin that referenced this pull request Nov 7, 2023
It's ont completely clear to me why this needs to be explicitly
specified in some environments, and not in others, also at the same time
that llvm-symbolizer is already in PATH, but this has fixed the 2 issues
outlined in bitcoin#28147 for me.
fanquake added a commit to fanquake/bitcoin that referenced this pull request Nov 7, 2023
It's not completely clear to me why this needs to be explicitly
specified in some environments, and not in others, while at the same time
that `llvm-symbolizer` is already in PATH, but this has fixed the 2 issues
outlined in bitcoin#28147.

In future, we might be able to use `LLVM_SYMBOLIZER_PATH` instead,
however that isn't currently used consistently across LLVM, i.e
it's checked for in the asan_symbolize script, but not in in the ubsan
script, or from in compiler-rt.
fanquake added a commit to fanquake/bitcoin that referenced this pull request Nov 7, 2023
It's not completely clear to me why this needs to be explicitly
specified in some environments, and not in others, while at the same time
that `llvm-symbolizer` is already in PATH, but this has fixed the 2 issues
outlined in bitcoin#28147.

In future, we might be able to use `LLVM_SYMBOLIZER_PATH` instead,
however that isn't currently used consistently across LLVM, i.e
it's checked for in the asan_symbolize script, but not in in the ubsan
script, or from in compiler-rt.
fanquake added a commit to fanquake/bitcoin that referenced this pull request Nov 7, 2023
It's not completely clear to me why this needs to be explicitly
specified in some environments, and not in others, while at the same time
that `llvm-symbolizer` is already in PATH, but this has fixed the 2 issues
outlined in bitcoin#28147.

In future, we might be able to use `LLVM_SYMBOLIZER_PATH` instead,
however that isn't currently used consistently across LLVM, i.e
it's checked for in the asan_symbolize script, but not in in the ubsan
script, or from in compiler-rt.
fanquake added a commit to fanquake/bitcoin that referenced this pull request Nov 7, 2023
It's not completely clear to me why this needs to be explicitly
specified in some environments, and not in others, while at the same time
that `llvm-symbolizer` is already in PATH, but this has fixed the 2 issues
outlined in bitcoin#28147.

In future, we might be able to use `LLVM_SYMBOLIZER_PATH` instead,
however that isn't currently used consistently across LLVM, i.e
it's checked for in the asan_symbolize script, but not in in the ubsan
script, or from in compiler-rt.
fanquake added a commit to fanquake/bitcoin that referenced this pull request Nov 7, 2023
It's not completely clear to me why this needs to be explicitly
specified in some environments, and not in others, while at the same time
that `llvm-symbolizer` is already in PATH, but this has fixed the 2 issues
outlined in bitcoin#28147.

Use `LLVM_SYMBOLIZER_PATH` as the env var, as that is somewhat also used
inside LLVM, but not consistently, i.e it's checked for in the asan_symbolize
script, but not in in the ubsan_symbolize script, or from in compiler-rt.
fanquake added a commit to fanquake/bitcoin that referenced this pull request Nov 7, 2023
It's not completely clear to me why this needs to be explicitly
specified in some environments, and not in others, while at the same time
that `llvm-symbolizer` is already in PATH, but this has fixed the 2 issues
outlined in bitcoin#28147.

Use `LLVM_SYMBOLIZER_PATH` as the env var, as that is somewhat also used
inside LLVM, but not consistently, i.e it's checked for in the asan_symbolize
script, but not in in the ubsan_symbolize script, or from in compiler-rt.
fanquake added a commit to fanquake/bitcoin that referenced this pull request Nov 7, 2023
It's not completely clear to me why this needs to be explicitly
specified in some environments, and not in others, while at the same time
that `llvm-symbolizer` is already in PATH, but this has fixed the 2 issues
outlined in bitcoin#28147.

Use `LLVM_SYMBOLIZER_PATH` as the env var, as that is somewhat also used
inside LLVM, but not consistently, i.e it's checked for in the asan_symbolize
script, but not in in the ubsan_symbolize script, or from in compiler-rt.
fanquake added a commit to bitcoin-core/gui that referenced this pull request Nov 8, 2023
49d9532 fuzz: explicitly specify llvm-symbolizer path in runner (fanquake)

Pull request description:

  It's not completely clear to me why this needs to be explicitly specified in some environments, and not in others, while at the same time that `llvm-symbolizer` is already in PATH, but this has fixed the 2 issues outlined in bitcoin/bitcoin#28147.

  Use `LLVM_SYMBOLIZER_PATH` as the env var, as that is somewhat also used inside LLVM, but not consistently, i.e it's checked for in the asan_symbolize script, but not in in the ubsan_symbolize script, or from in compiler-rt.

  Alternative to #28804.

ACKs for top commit:
  maflcko:
    lgtm ACK 49d9532

Tree-SHA512: c3d5bf1c3629793b342c70754a419b3c7a3cd39f800b9aa69ce3395cc2bf83b4d46f2b329974337b94b99573cd0b8600d3f147ed5c21387bf3812316570d1ee3
@bitcoin bitcoin locked and limited conversation to collaborators Nov 5, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants