Skip to content
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.

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

Unique bugs deduplicated into the same issue #8389

Open
alexcrichton opened this issue Sep 1, 2022 · 6 comments
Open

Unique bugs deduplicated into the same issue #8389

alexcrichton opened this issue Sep 1, 2022 · 6 comments

Comments

@alexcrichton
Copy link
Contributor

Today I discovered a case in the Wasmtime project's fuzzing where two different fuzz bugs reported were lumped into the same issue reported on the Chromium issue tracker. The two original test cases and the corresponding issues in the Wasmtime repository are:

I believe that this is a case where oss-fuzz incorrectly categorized these two issues as the same bug in the oss-fuzz interface. The assert messages are similar (both are assert_eq!(a, b) failures tripping in Rust) but the actual underlying bugs were completely different. One thing we noticed was that the first ~10 stack frames are similar since that's how Rust programs are structured (panic handling machinery, etc). Is this perhaps a case where heuristics for Rust may need tweaking for determining whether two fuzz bugs are the same?

cc @fitzgen

@Navidem
Copy link
Contributor

Navidem commented Sep 1, 2022

Thanks for brining this to our attention,

One thing we noticed was that the first ~10 stack frames are similar

This is actually the reason for the two bugs being grouped together. Currently ClusterFuzz uses a heuristic based on the top 3 stack frame entries. We have plans to revisit the way ClusterFuzz deduplicates crashes.

I am not sure if having special heuristics for Rust is the way to go here, @oliverchang WDYT?

@alexcrichton
Copy link
Contributor Author

At least for Rust-based deduplication of bugs one heuristic which might help is to skip stack frames where the demangled name starts with core::, std::, or libfuzzer_sys:: since those are all infrastructural. Looking at the two test cases above that would have been sufficient to consider them unique. Rust may be relatively different from C++ here here an assertion failure goes through a fair bit of infrastructure:



#0 0x7fb2ed3e718b in raise /build/glibc-eX1tMB/glibc-2.31/sysdeps/unix/sysv/linux/raise.c:51:1
--
  | #1 0x7fb2ed3c6858 in abort /build/glibc-eX1tMB/glibc-2.31/stdlib/abort.c:79:7
  | #2 0x56513ce01496 in std::sys::unix::abort_internal::hf93f90007823438c /rustc/9243168fa5615ec8ebe9164c6bc2fdcccffd08b6/library/std/src/sys/unix/mod.rs:293:14
  | #3 0x56513bda6e06 in std::process::abort::h14a31135c4255a96 /rustc/9243168fa5615ec8ebe9164c6bc2fdcccffd08b6/library/std/src/process.rs:2119:5
  | #4 0x56513cd86d43 in libfuzzer_sys::initialize::_$u7b$u7b$closure$u7d$u7d$::hf9430643e0f21c8f /rust/registry/src/github.com-1ecc6299db9ec823/libfuzzer-sys-0.4.3/src/lib.rs:57:9
  | #5 0x56513cdfefec in std::panicking::rust_panic_with_hook::hbe406e485bc0b695 /rustc/9243168fa5615ec8ebe9164c6bc2fdcccffd08b6/library/std/src/panicking.rs:702:17
  | #6 0x56513cdfee46 in std::panicking::begin_panic_handler::_$u7b$u7b$closure$u7d$u7d$::ha6407ec35ab7337e /rustc/9243168fa5615ec8ebe9164c6bc2fdcccffd08b6/library/std/src/panicking.rs:588:13
  | #7 0x56513cdfd41b in std::sys_common::backtrace::__rust_end_short_backtrace::he7f334b01286fae5 /rustc/9243168fa5615ec8ebe9164c6bc2fdcccffd08b6/library/std/src/sys_common/backtrace.rs:138:18
  | #8 0x56513cdfeb61 in rust_begin_unwind /rustc/9243168fa5615ec8ebe9164c6bc2fdcccffd08b6/library/std/src/panicking.rs:584:5
  | #9 0x56513bda81d2 in core::panicking::panic_fmt::h9f4d3779a2b7f3e4 /rustc/9243168fa5615ec8ebe9164c6bc2fdcccffd08b6/library/core/src/panicking.rs:142:14
  | #10 0x56513ce1b17a in core::panicking::assert_failed_inner::he0106c9b459285c8 /rustc/9243168fa5615ec8ebe9164c6bc2fdcccffd08b6/library/core/src/panicking.rs:0
  | #11 0x56513bce6685 in core::panicking::assert_failed::h3c304748d00b2943 /rustc/9243168fa5615ec8ebe9164c6bc2fdcccffd08b6/library/core/src/panicking.rs:181:5

We could probably work on getting some of that inlined to have fewer stack frames though if that would help too

@oliverchang
Copy link
Collaborator

In these two cases, the deduplication is based on picking these things out of the stacktrace:

assertion failed: `(left == right)`
wasmtime_fuzzing::oracles::differential::h903c53537a025acb

and

assertion failed: `(left == right)`

(See the "Crash State" section on the testcases).

We already ignore most core::, std:: etc frames, and picked those out as the ones for use for deduplication.

I'm not sure how we can improve this case unfortunately, without negatively impacting other similar cases where these may in fact be the same bug (and resulting in more bug spam).

@alexcrichton
Copy link
Contributor Author

Is there a rule for filtering out the rust_fuzzer_test_input function? If so could that be removed? If not one possible fix we could apply in Rust is to force that function to call an #[inline(never)] function which would mean that the base call stack for the fuzz test case would be something like the_name_of_the_fuzzer::main::... which could help stack trace pruning realize that there's a fuzzer-local frame that isn't shared across fuzzers there.

alexcrichton added a commit to alexcrichton/libfuzzer that referenced this issue Sep 6, 2022
This change is an attempt to address the behavior found at
google/oss-fuzz#8389 where two distinct bugs were accidentally
deduplicated into the same bug report. One of the reasons for this is
that the stack traces between the two bugs were almost the same with
only very minor differences. My hope is that by forcing a unique stack
frame per fuzzer this will be less likely since there is guaranteed to
be at least one stack frame per fuzz target which is unique with this
change.

While I was here I wrapped up the generated function by the
`fuzz_target!` macro in a `const _: () = { ... }` to avoid adding this
new `run` function in to the normal module's namespace and accidentally
causing name collisions (e.g. if fuzz targets already have functions
named `run`)
@alexcrichton
Copy link
Contributor Author

I went ahead and sent rust-fuzz/libfuzzer#95 to update Rust's fuzzing infrastructure to ideally have at least one uniqe stack frame per fuzzer.

@oliverchang
Copy link
Collaborator

Thanks! Note that this may have downsides as well -- fuzzers within the same project will have overlap. Sometimes it's quite beneficial to have these be deduplicated rather than file N bugs (for N fuzzers) for the same underlying issue with the project.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants