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

Acceptance for wasmtime project and Rust fuzz targets #3285

Merged
merged 1 commit into from
Jan 23, 2020
Merged

Acceptance for wasmtime project and Rust fuzz targets #3285

merged 1 commit into from
Jan 23, 2020

Conversation

jfoote
Copy link
Contributor

@jfoote jfoote commented Jan 23, 2020

Hello,

Bytecode Alliance projects are developed in the open, largely in Rust. As part of designing continuous fuzzing for Bytecode Alliance projects we are evaluating how we might integrate Rust fuzz targets with oss-fuzz. (some background: Bytecode Alliance discussion issue, oss-fuzz PR for an unrelated Rust fuzz target from last year).

This PR is for project acceptance for Rust fuzzing targets for wasmtime. @fitzgen and I did some analysis and made a few modifications to Rust's libfuzzer wrapper and cargo-fuzz to towards an integration with oss-fuzz. It works, but it isn't perfect yet. Here are some highlights:

  • cargo-fuzz targets can now link the oss-fuzz build environment's libfuzzer (libFuzzingEngine)
    • because -fsanitizer=fuzzer is not supported directly, we use the default flags used by cargo fuzz and link with LIB_FUZZING_ENGINE_DEPRECATED
    • cargo-fuzz supports only libfuzzer at this time
  • cargo-fuzz can build statically linked fuzz targets (without also running them)
  • cargo-fuzz builds support address sanitizer, but not other oss-fuzz-supported sanitizers
    • as of this writing, cargo-fuzz supports address, memory, leak, and thread sanitizers
    • while memory sanitizer is supported by both cargo-fuzz and oss-fuzz, building Rust fuzz targets with memory sanitizer and libFuzzingEngine.a produces a linker error. I suspect an incompatibility in the instrumenting/linking used in libFuzzingEngine.a and what rustc/libfuzzer are using, but I did not dive in.
    • ubsan is not supported by the Rust toolchain at this time AFAICT
    • The fsanitize* flags are not supported, so they must be stripped. As a workaround I hard-coded the CFLAGS/CXXFLAGS from infra/base-images/base-clang into this project's build.sh since this seems to be what is used in the oss-fuzz build container today once you remove the sanitizer flags, but this is not a good long-term solution.
  • Rust does not support clang source-based coverage
    • there is discussion of supporting an analogous feature in Rust, with some recent activity, but no commitment/timeline AFAICT

We have a draft oss-fuzz wasmtime integration staged here.

We wanted to get your thoughts on integrating with Rust projects: Do you feel this is a viable path forward for long-term integration with Rust targets? If so, what else do you think would need to be done before we can start using oss-fuzz for Bytecode Alliance projects in the short-term?

Thanks,
Jon

@TravisBuddy
Copy link

Hey @jfoote,
Your changes look good to me!

View build log

TravisBuddy Request Identifier: c06579a0-3d82-11ea-bf94-57f545f30904

@jonathanmetzman
Copy link
Contributor

We approve.
Sad about the LIB_FUZZING_ENGINE_DEPRECATED thing. We should fix this long term because I really would like to get rid of this (or switch to a single solution for everyone).

Shameless plug for my own work fuzzing C binaries in the WASM runtime (SQLite demo, slides)

@jonathanmetzman jonathanmetzman merged commit 7964a95 into google:master Jan 23, 2020
@fitzgen
Copy link
Contributor

fitzgen commented Jan 23, 2020

We approve.

🎉

Sad about the LIB_FUZZING_ENGINE_DEPRECATED thing. We should fix this long term because I really would like to get rid of this (or switch to a single solution for everyone).

The way that rustc does sanitizers is with the nightly-channel only -Z flag, e.g.

$ rustc foo.rs -Z sanitizer=address

Would adding support for -Z sanitizer=fuzzer help at all here, or is the complication that rustc doesn't accept identical flags as clang?

Shameless plug for my own work fuzzing C binaries in the WASM runtime (SQLite demo, slides)

Very cool!

@jfoote
Copy link
Contributor Author

jfoote commented Jan 23, 2020

Great, thanks @jonathanmetzman!

Shameless plug for my own work fuzzing C binaries in the WASM runtime (SQLite demo, slides)

This is really cool.

Would adding support for -Z sanitizer=fuzzer help at all here, or is the complication that rustc doesn't accept identical flags as clang?

IIUC if we go the -Z sanitizer=fuzzer route would we link the version of libfuzzer that is vendored into libfuzzer-sys, rather than the libfuzzer library that is supplied by the version of clang that is installed in the oss-fuzz build environment. Regardless, if that is acceptable we may not need to use $LIB_FUZZING_ENGINE or $LIB_FUZZING_ENGINE_DEPRECATED at all for building Rust targets, so this effort would not block removal of the latter.

@fitzgen
Copy link
Contributor

fitzgen commented Jan 23, 2020

IIUC if we go the -Z sanitizer=fuzzer route would we link the version of libfuzzer that is vendored into libfuzzer-sys, rather than the libfuzzer library that is supplied by the version of clang that is installed in the oss-fuzz build environment.

Adding -Z sanitizer=fuzzer to rustc would probably work something like vendoring libfuzzer inside rustc alongside LLVM and then updating it whenever we update LLVM as well. Note that I'm not sure whether or not this is something the Rust compiler team would want to support.

Then the oss-fuzz build environment would control what libfuzzer was used with -Z sanitizer=fuzzer by providing a certain rustc version. It would probably not be the same version that any given clang was using.

Not clear to me whether this is an improvement over providing a path to a libfuzzer that we can link against.

@jonathanmetzman
Copy link
Contributor

jonathanmetzman commented Jan 23, 2020

IIUC if we go the -Z sanitizer=fuzzer route would we link the version of libfuzzer that is vendored into libfuzzer-sys, rather than the libfuzzer library that is supplied by the version of clang that is installed in the oss-fuzz build environment.

Adding -Z sanitizer=fuzzer to rustc would probably work something like vendoring libfuzzer inside rustc alongside LLVM and then updating it whenever we update LLVM as well. Note that I'm not sure whether or not this is something the Rust compiler team would want to support.

Then the oss-fuzz build environment would control what libfuzzer was used with -Z sanitizer=fuzzer by providing a certain rustc version. It would probably not be the same version that any given clang was using.

Not clear to me whether this is an improvement over providing a path to a libfuzzer that we can link against.

I think I agree after learning more that the status quo would be better for rust than supporting LIB_FUZZING_ENGINE/-fsanitize=fuzzer.

For context, LIB_FUZZING_ENGINE_DEPRECATED uses a version of libFuzzer that is compiled with the sanitizer used by the fuzzer (to support MSAN). We prefer the bundled libFuzzer because it uses a private libc++ so it works regardless of sanitizer. We've seen that compiling libFuzzer with ASAN can have some significant performance costs in some cases (more info on all of this here).

I don't think the perf issue is critical (we did nothing to stop it until this spring and OSS-Fuzz was useful before then). I'm almost more concerned that it is ugly for rust to official depend on something with DEPRECATED in the name. Maybe we need to change it?
Thoughts @kcc @Dor1s ?

@Dor1s
Copy link
Contributor

Dor1s commented Jan 24, 2020

I feel like we don't have much of a trouble supporting LIB_FUZZING_ENGINE_DEPRECATED, so I'm fine with renaming it and (maybe) having more clear documentation around it (but not necessary).

It also might be handy for some experiments, as updating / patching / reverting libFuzzer sources only is easier than doing LLVM roll ourselves (right now we depend on Chromium for that).

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

Successfully merging this pull request may close these issues.

5 participants