Skip to content

suppressions: note that type:ClassName::MethodName should be used #28147

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
fanquake merged 1 commit into from
Jul 26, 2023
Merged

suppressions: note that type:ClassName::MethodName should be used #28147

fanquake merged 1 commit into from
Jul 26, 2023

Conversation

@fanquake
Copy link
Member

@fanquake fanquake commented Jul 25, 2023

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

@fanquake
suppressions: note that 'type:ClassName::MethodName' should be used
d0c6cc4 
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 •
edited
Loading

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

hebasto
hebasto approved these changes Jul 25, 2023
View reviewed changes
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
@fanquake
Merge bitcoin#28147: suppressions: note that `type:ClassName::MethodN…
41341f0 
…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 fanquake mentioned this pull request Nov 7, 2023
Merged
fanquake added a commit to fanquake/bitcoin that referenced this pull request Nov 7, 2023
@fanquake
fuzz: explicitly specify llvm-symbolizer path in runner
49d9532 
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
@fanquake
Merge bitcoin/bitcoin#28814: test: symbolizer improvements
b5d8f00 
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.

Reviewers

@hebasto hebasto hebasto approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@fanquake @DrahtBot @maflcko @hebasto