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

Using 'catch_unwind' to convert panic into error is still detected as crash #85

Open
nbigaouette opened this issue Oct 19, 2021 · 6 comments

Comments

@nbigaouette
Copy link

I wrap a function that I know is buggy into a std::panic::catch_unwind() so that when it panics my own code does not. I can then gracefully handle the error.

Unfortunately, libfuzzer still detects this as an error and stops.

Here's an example fuzzing target:

#![no_main]
use libfuzzer_sys::fuzz_target;

fn may_panic(input: &[u8]) -> u8 {
    let value = *input.get(0).unwrap_or(&0);
    if value > 240 {
        panic!("Value is greater than 240! value={}", value);
    }
    value
}

fn wrapper(input: &[u8]) -> Option<u8> {
    match std::panic::catch_unwind(|| may_panic(input)) {
        Ok(value) => Some(value),
        Err(err) => {
            println!("Caught panic, returning None");
            None
        }
    }
}

fuzz_target!(|data: &[u8]| {
    wrapper(data);
});
INFO: Running with entropic power schedule (0xFF, 100).
INFO: Seed: 2547700486
INFO: Loaded 1 modules   (743 inline 8-bit counters): 743 [0x10946a9c0, 0x10946aca7),
INFO: Loaded 1 PC tables (743 PCs): 743 [0x10946aca8,0x10946db18),
fuzz/target/x86_64-apple-darwin/release/issue: Running 1 inputs 1 time(s) each.
Running: fuzz/artifacts/issue/minimized-from-ac85e832d98dd32e68baaafa21973abcb7a73101
thread '<unnamed>' panicked at 'Value is greater than 240! value=243', fuzz_targets/issue.rs:7:9
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
==26058== ERROR: libFuzzer: deadly signal
    #0 0x109605b15 in __sanitizer_print_stack_trace+0x35 (librustc-nightly_rt.asan.dylib:x86_64+0x4fb15)
    rust-fuzz/cargo-fuzz#1 0x10934b14a in fuzzer::PrintStackTrace()+0x2a (issue:x86_64+0x10001d14a)
    rust-fuzz/cargo-fuzz#2 0x10933d093 in fuzzer::Fuzzer::CrashCallback()+0x43 (issue:x86_64+0x10000f093)
    rust-fuzz/cargo-fuzz#3 0x7fff734065fc in _sigtramp+0x1c (libsystem_platform.dylib:x86_64+0x35fc)
    rust-fuzz/cargo-fuzz#4 0x10946db1f  (<unknown module>)
    rust-fuzz/cargo-fuzz#5 0x7fff732dc807 in abort+0x77 (libsystem_c.dylib:x86_64+0x7f807)
    rust-fuzz/cargo-fuzz#6 0x1093c9528 in std::sys::unix::abort_internal::h70ee130d4bec8a65+0x8 (issue:x86_64+0x10009b528)
    rust-fuzz/cargo-fuzz#7 0x10942df08 in std::process::abort::h8a4caf764e23a06d+0x8 (issue:x86_64+0x1000fff08)
    rust-fuzz/cargo-fuzz#8 0x10933bf74 in libfuzzer_sys::initialize::_$u7b$$u7b$closure$u7d$$u7d$::h09a768b82a6aeb19+0x54 (issue:x86_64+0x10000df74)
    rust-fuzz/cargo-fuzz#9 0x1093bdb65 in std::panicking::rust_panic_with_hook::h2f2e429268b7f845+0x255 (issue:x86_64+0x10008fb65)
    rust-fuzz/cargo-fuzz#10 0x1093bd5dd in std::panicking::begin_panic_handler::_$u7b$$u7b$closure$u7d$$u7d$::he71680fea17513cb+0x8d (issue:x86_64+0x10008f5dd)
    rust-fuzz/cargo-fuzz#11 0x1093ba376 in std::sys_common::backtrace::__rust_end_short_backtrace::h32d0456e90e0ce79+0x16 (issue:x86_64+0x10008c376)
    rust-fuzz/cargo-fuzz#12 0x1093bd549 in rust_begin_unwind+0x49 (issue:x86_64+0x10008f549)
    rust-fuzz/cargo-fuzz#13 0x10942e39a in std::panicking::begin_panic_fmt::hf8e6c99ec321f361+0x3a (issue:x86_64+0x10010039a)
    rust-fuzz/cargo-fuzz#14 0x10933277b in std::panicking::try::do_call::hcdaafa6b928860c7+0x3ab (issue:x86_64+0x10000477b)
    rust-fuzz/cargo-fuzz#15 0x109337eb3 in __rust_try+0x13 (issue:x86_64+0x100009eb3)
    rust-fuzz/cargo-fuzz#16 0x109336da8 in issue::wrapper::h9ac1378f8c7f7847+0x128 (issue:x86_64+0x100008da8)
    rust-fuzz/cargo-fuzz#17 0x109337a5f in rust_fuzzer_test_input+0x49f (issue:x86_64+0x100009a5f)
    rust-fuzz/cargo-fuzz#18 0x10933c083 in __rust_try+0x13 (issue:x86_64+0x10000e083)
    rust-fuzz/cargo-fuzz#19 0x10933b7d1 in LLVMFuzzerTestOneInput+0x131 (issue:x86_64+0x10000d7d1)
    rust-fuzz/cargo-fuzz#20 0x10933edd1 in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long)+0x131 (issue:x86_64+0x100010dd1)
    rust-fuzz/cargo-fuzz#21 0x10935c315 in fuzzer::RunOneTest(fuzzer::Fuzzer*, char const*, unsigned long)+0xe5 (issue:x86_64+0x10002e315)
    rust-fuzz/cargo-fuzz#22 0x1093625fd in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long))+0x1edd (issue:x86_64+0x1000345fd)
    rust-fuzz/cargo-fuzz#23 0x109371b42 in main+0x22 (issue:x86_64+0x100043b42)
    rust-fuzz/cargo-fuzz#24 0x7fff7320dcc8 in start+0x0 (libdyld.dylib:x86_64+0x1acc8)

NOTE: libFuzzer has rudimentary signal handlers.
      Combine libFuzzer with AddressSanitizer or similar for better crash reports.
SUMMARY: libFuzzer: deadly signal
────────────────────────────────────────────────────────────────────────────────

Error: Fuzz target exited with exit code: 77

Where the file fuzz/artifacts/issue/minimized-from-ac85e832d98dd32e68baaafa21973abcb7a73101 contains a single byte: 0xF3 (243).

Can we instruct libfuzzer to not bother with these kinds of errors?

Thanks!

@nagisa
Copy link
Member

nagisa commented Oct 19, 2021

This appears to be related to -Cpanic=abort panic strategy.

@fitzgen fitzgen transferred this issue from rust-fuzz/cargo-fuzz Oct 19, 2021
@fitzgen
Copy link
Member

fitzgen commented Oct 19, 2021

I'm not sure why we went with the -Cpanic=abort approach; that was already in place before I started hacking on any of this stuff. Maybe someone else who remembers why it is like this can share?

It seems like using catch_unwind inside the fuzz_target! macro's expanded code would be more flexible and allow for users to catch+handle irrelevant panics inside the fuzz target.

@Manishearth
Copy link
Member

AIUI the fuzzer works better if panics lead to immediate aborts since there's less logic to trace. But I'm not quite sure if there is a major difference.

@nagisa
Copy link
Member

nagisa commented Oct 19, 2021

I recall it being much better in a sense that it gives libfuzzer an opportunity to see unique crashes easier. Or at least that was the case back when I first MVPd the thing.

@fitzgen
Copy link
Member

fitzgen commented Oct 19, 2021

I recall it being much better in a sense that it gives libfuzzer an opportunity to see unique crashes easier. Or at least that was the case back when I first MVPd the thing.

This makes a ton of sense, since tools like oss-fuzz will ~essentially use only the crashing stack trace to differentiate bugs, AFAIK.


I wonder if we can make -Cpanic=abort a CLI flag in cargo fuzz? Would require some cooperation in libfuzzer-sys too for whether to set the panic hook or not, probably with env vars like our other points of cooperation.

@Manishearth
Copy link
Member

Yeah I think the solution here is to have an on by default option

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

4 participants