Add a subcommand to build a clusterfuzz tarball #162
Conversation
Ekleog-NEAR
force-pushed
the
build-clusterfuzz
branch
from
31dbbaa
to
0740f82
Compare
Ekleog-NEAR
force-pushed
the
build-clusterfuzz
branch
from
0740f82
to
05a32d5
Compare
|
Ok, this is now rebased and ready for review! If you need any specific information regarding clusterfuzz please feel free to ask; the overall idea is to build a tarball that has all the test binaries so they’re not detected as fuzzers by clusterfuzz, and to add shell scripts detected as fuzzers by clusterfuzz that just call the test binaries with the proper cargo-bolero way of passing arguments :) |
|
Oh, just to also say: clusterfuzz also theoretically supports afl, honggfuzz, syzcaller and other fuzzer types. I haven’t gotten to the point of implementing that yet, but I think the current PR would easily be extendable for it: we’d only have to add a Another potential future improvement would be to be able to filter the list of bolero tests, so that only some of them get introduced in the clusterfuzz tarball. TBH, I don’t think this would be necessary apart from very specific use cases (even within the already specific use case of clusterfuzz/oss-fuzz), so I don’t think it’s worth implementing it right now, especially when other fuzzer types aren’t supported yet :) |
Ekleog-NEAR
force-pushed
the
build-clusterfuzz
branch
from
05a32d5
to
30e2457
Compare
| std::fs::create_dir_all(OUTPUT_DIR).context("creating clusterfuzz build directory")?; | ||
| let output_path = Path::new(OUTPUT_DIR).join("clusterfuzz.tar"); |
There was a problem hiding this comment.
This technically means you have to run this command in the root of the workspace. Can we join the manifest dir to behave more like cargo?
There was a problem hiding this comment.
Done, the only drawback is it requires a new dependency on cargo_metadata so as to not re-implement it all, but it’s only a dependency from cargo-bolero so it should be fine :)
| assert!( | ||
| t.is_harnessed, | ||
| "Non-harnessed tests are not supported for clusterfuzz yet" | ||
| ); |
There was a problem hiding this comment.
I'm not sure what prevents us from supporting non-harnessed tests here. You just don't pass the libtest args, similar to what we did with libfuzzer.
There was a problem hiding this comment.
TBH it’s mostly that I don’t understand what exactly a non-harnessed test is or how it works exactly, so I didn’t want to fake support for it 😅
[edit: now-useless wall of code]
There was a problem hiding this comment.
Actually I just found my first example of non-harnessed tests while trying to write the integration test, and can confirm that this seems to work fine!
| (TESTFN.as_mut().expect("uninitialized test function"))(data_slice); | ||
| 0 |
There was a problem hiding this comment.
This and the below are due to the way test failures get recognized by clusterfuzz. Sorry I forgot to point towards the commit message, where I had explained it 😅
Here it is:
LLVMFuzzerTestOneInput is not supposed to ever return something other
than 0. Instead, the whole process should abort in case of a bug being
detected.Hence, this replaces the previous returning of 1 (which triggered a
libfuzzer assertion downstream, that was detected by clusterfuzz as the
root cause of the error) by an abort (which lets clusterfuzz detect the
actual cause of the error in the panic message)
There was a problem hiding this comment.
See also https://www.llvm.org/docs/LibFuzzer.html#fuzz-target ; where values other than 0 and -1 are "reserved for future use"
lib/bolero-libfuzzer/src/lib.rs
Outdated
| } | ||
|
|
||
| std::process::abort(); |
|
|
||
| /// Builds a tarball for uploading to clusterfuzz | ||
| #[derive(Debug, StructOpt)] | ||
| pub struct BuildClusterfuzz { |
There was a problem hiding this comment.
Could you add an integration test that shows this all works how we would expect? I'm not familiar with clusterfuzz and don't want to have an accidental regression in behavior in the future.
There was a problem hiding this comment.
I think I’ve added a reasonable-ish integration test in the cargo_bolero.rs file.
Hopefully I have now handled all your review comments! Please let me know what you think about the new code, and the integration test that gives an example of how clusterfuzz runs the fuzzers :)
Ekleog-NEAR
force-pushed
the
build-clusterfuzz
branch
from
30e2457
to
f06e0fe
Compare
LLVMFuzzerTestOneInput is not supposed to ever return something other than 0. Instead, the whole process should abort in case of a bug being detected. Hence, this replaces the previous returning of 1 (which triggered a libfuzzer assertion downstream, that was detected by clusterfuzz as the root cause of the error) by an abort (which should let clusterfuzz detect the actual cause of the error in the panic message)
Ekleog-NEAR
force-pushed
the
build-clusterfuzz
branch
from
f06e0fe
to
9bb4763
Compare
Ekleog-NEAR
force-pushed
the
build-clusterfuzz
branch
2 times, most recently
from
566e9cc
to
35dd63b
Compare
Ekleog-NEAR
force-pushed
the
build-clusterfuzz
branch
from
35dd63b
to
0e9d441
Compare
|
Thank you for the two reviews and merges! :D |
|
I'm planning on making one more PR and then I'll get a release out |
|
Awesome, thank you! On my end I’m currently switching our infra to the current tip of master (near/nearcore#9603) ; will be able to confirm that everything works fine once it lands and a few days pass :) (The not-yet-landed code is already in production at https://github.com/near/finite-wasm/blob/main/.github/workflows/fuzz.yml#L34; I’ll switch it to the release once it’s out) |
This makes it very to use bolero on clusterfuzz deployments, like private ones or oss-fuzz (though I have only tested with a private deployment, not having access to an oss-fuzz-enabled project).
This builds upon #161, so please ignore the first two commits for the review. Once #161 lands I’ll rebase on top of master, but clusterfuzz requires a working -jobs option to actually work.
Fixes #98