oss-fuzz icon indicating copy to clipboard operation
oss-fuzz copied to clipboard

[infra] Check that fuzzers don't fail on an empty input file

Open fmeum opened this issue 3 years ago • 12 comments

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

fmeum avatar Aug 24 '22 04:08 fmeum

@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?

fmeum avatar Aug 24 '22 07:08 fmeum

@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.

jonathanmetzman avatar Aug 24 '22 15:08 jonathanmetzman

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.

jonathanmetzman avatar Aug 24 '22 15:08 jonathanmetzman

@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.

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?

oliverchang avatar Sep 01 '22 04:09 oliverchang

@fmeum could you please merge master into this PR to see if that fixes the infra tests?

oliverchang avatar Sep 01 '22 04:09 oliverchang

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?

fmeum avatar Sep 01 '22 07:09 fmeum

Thanks @fmeum! @jonathanmetzman WDYT?

oliverchang avatar Sep 05 '22 02:09 oliverchang

I'm not sold on this change, i don't think it's necessary and it slows things down.

jonathanmetzman avatar Sep 06 '22 15:09 jonathanmetzman

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.

oliverchang avatar Sep 07 '22 07:09 oliverchang

@oliverchang Some of the checks fail with:

2022-09-09T14:50:01.9843353Z [0m[91mERROR: /src/centipede/BUILD:502:8: Compiling blob_file_test.cc failed: (Exit 1): clang-15 failed: error executing command /usr/local/bin/clang-15 -U_FORTIFY_SOURCE -fstack-protector -Wall -Wthread-safety -Wself-assign -Wunused-but-set-parameter -Wno-free-nonheap-object -fcolor-diagnostics -fno-omit-frame-pointer -g0 -O2 ... (remaining 53 arguments skipped)
2022-09-09T14:50:01.9844040Z 
2022-09-09T14:50:01.9844480Z Use --sandbox_debug to see verbose messages from the sandbox and retain the sandbox build root for debugging
2022-09-09T14:50:01.9916525Z [0m[91mblob_file_test.cc:50:5: error: use of undeclared identifier 'EXPECT_OK'
2022-09-09T14:50:01.9916996Z     EXPECT_OK(appender->Open(path));

I'm also running into this locally.

fmeum avatar Sep 09 '22 15:09 fmeum

@oliverchang Some of the checks fail with:

2022-09-09T14:50:01.9843353Z �[0m�[91mERROR: /src/centipede/BUILD:502:8: Compiling blob_file_test.cc failed: (Exit 1): clang-15 failed: error executing command /usr/local/bin/clang-15 -U_FORTIFY_SOURCE -fstack-protector -Wall -Wthread-safety -Wself-assign -Wunused-but-set-parameter -Wno-free-nonheap-object -fcolor-diagnostics -fno-omit-frame-pointer -g0 -O2 ... (remaining 53 arguments skipped)
2022-09-09T14:50:01.9844040Z 
2022-09-09T14:50:01.9844480Z Use --sandbox_debug to see verbose messages from the sandbox and retain the sandbox build root for debugging
2022-09-09T14:50:01.9916525Z �[0m�[91mblob_file_test.cc:50:5: error: use of undeclared identifier 'EXPECT_OK'
2022-09-09T14:50:01.9916996Z     EXPECT_OK(appender->Open(path));

I'm also running into this locally.

Please merge master. This should be fixed by https://github.com/google/oss-fuzz/pull/8467

oliverchang avatar Sep 12 '22 01:09 oliverchang

@oliverchang Looks good now.

fmeum avatar Sep 12 '22 08:09 fmeum