-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
[infra] Check that fuzzers don't fail on an empty input file #8341
base: master
Are you sure you want to change the base?
Conversation
f5a7786
to
6691a7d
Compare
@oliverchang I don't see how the test failure is related to my change as it diffs cloud build steps. Do you know what could cause it? |
@oliverchang I'm not sure I like this. I think it's super tailored to catching the last problem (which should be caught through pinning and tests there) and I don't want to add overhead to bad_build_check which is used in ClusterFuzzLite. |
There may be fuzzers that are slow to startup and frankly I think any amount of time spent on this in ClusterFuzzLite is too much, it doesn't belong there and I think we should do something like this elsewhere. |
I think this is still important. We have a lot of wrapper scripts now with languages like Java, Python and we need to make sure reproducing works. Even ignoring the last issue -- it seems to make perfect sense to me that a target should not crash with an empty input. Re ClusterFuzzLite slowness -- is this something we can fix there instead? Does bad build check make sense there in general? |
@fmeum could you please merge master into this PR to see if that fixes the infra tests? |
Will do. @oliverchang If this check is mostly meant to apply to Jazzer, Atheris and potential future wrappers, maybe limiting the check to fuzzing languages other than C/C++ could help with @jonathanmetzman's concerns? |
6691a7d
to
de3b375
Compare
Thanks @fmeum! @jonathanmetzman WDYT? |
I'm not sold on this change, i don't think it's necessary and it slows things down. |
Spoke with @jonathanmetzman offline here. The main concern was slowing down CFLite -- we could just gate this behind a flag and let CFLite opt out. |
de3b375
to
e98011c
Compare
@oliverchang Some of the checks fail with:
I'm also running into this locally. |
Please merge master. This should be fixed by #8467 |
e98011c
to
fbc87e7
Compare
@oliverchang Looks good now. |
@@ -433,6 +466,13 @@ function main { | |||
result=$? | |||
checks_failed=$(( $checks_failed + $result )) | |||
|
|||
if [ -z ${MORE_BAD_BUILD_CHECKS+x} ] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be -n
instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, fixed.
@@ -433,6 +466,13 @@ function main { | |||
result=$? | |||
checks_failed=$(( $checks_failed + $result )) | |||
|
|||
if [ -n "$MORE_BAD_BUILD_CHECKS" ] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this may still break due to the -u
argument passed to the hashbang bash command.
You'll need something like ${MORE_BAD_BUILD_CHECKS:-}
instead
Adds a new bad build check for libfuzzer targets that runs the fuzzer on an empty file without any further arguments. This is specifically meant to catch issues with argument parsing in libFuzzer derivates such as Jazzer. Fixes google#8276
e6c1b3a
to
22e021d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! I'll merge this tomorrow since it's close to EOD for me.
Adds a new bad build check for libfuzzer targets that runs the fuzzer on
an empty file without any further arguments. This is specifically meant
to catch issues with argument parsing in libFuzzer derivates such as
Jazzer.
Fixes #8276