This is an archive of the discontinued LLVM Phabricator instance.

[libfuzzer] Compare TotalNumberOfRuns with MaxNumberOfRuns when testing a memory leak.
ClosedPublic

Authored by Dor1s on Sep 8 2017, 10:56 AM.

Event Timeline

Dor1s created this revision.Sep 8 2017, 10:56 AM
kcc edited edge metadata.Sep 8 2017, 11:01 AM

Code change LG, thanks!
Could you please also provide a (non-flaky) test?

Dor1s added a comment.Sep 8 2017, 11:02 AM

Alternatively, we may try not to add this + remove two existing checks and have only one inside of Fuzzer::ExecuteCallback, right after TotalNumberOfRuns++;.

kcc added a comment.Sep 8 2017, 11:06 AM

I prefer this way. ExecuteCallback is unaccounted for (e.g. it's used to run a dummy/emty input in the beginning)

Dor1s added a comment.Sep 8 2017, 11:09 AM

Makes sense! Yes, I'll add a reliable test.

Dor1s updated this revision to Diff 114423.Sep 8 2017, 1:11 PM

Added the test.

Dor1s added a comment.Sep 8 2017, 1:17 PM

Not ready for review yet... Adding a test was definitely a good idea. At first, I realized that my previous patch broke LEAK_IN_CORPUS test, becase with -runs=0 it wasn't detecting memory leaks. Due to that, I had to take into account DuringInitialCorpusExecution flag.

Now, value-profile-strcmp.test doesn't pass, which seems like some magic to me. If I manually compile libFuzzer + test, it works well. But when I do that via ninja check-fuzzer, it doesn't. I'll update my LLVM checkout and re-build all the stuff.

Dor1s added a comment.Sep 8 2017, 1:25 PM

Yeah, after the update everything works as expected. Please take a look! :)

Testing Time: 15.34s
  Expected Passes    : 132
kcc added inline comments.Sep 8 2017, 1:26 PM
test/fuzzer/max-number-of-runs.test
4

add a CHECK: like to verify the output.
Also, don't you want to run this test with -runs=4 and -runs=5 (or some such) to observe different behavior with and w/o your patch?

4

CHECK: line

Dor1s updated this revision to Diff 114425.Sep 8 2017, 1:38 PM

Added two more CHECKs for -runs=3 and -runs=5 to verify that libFuzzer always
performs correct number of runs.

Dor1s marked 2 inline comments as done.Sep 8 2017, 1:41 PM

w/o this patch, the test launched with -runs=4 always does 5 runs, but with the patch it never does more than expected

Dor1s updated this revision to Diff 114427.Sep 8 2017, 1:42 PM

Capitalize the first letter of local variable

Dor1s added a comment.Sep 11 2017, 8:02 AM

Ready for review. Is there anything else I should change?

kcc added a comment.Sep 11 2017, 6:03 PM

Looking at the test more, I think it's not really testing what we want to test.
And the test itself as a bad fuzz target, nondeterministic.

So, instead of adding a new .cpp test, maybe you could reuse AccumulateAllocationsTest.cpp?
You can run it with -runs=[234] and check the the number of runs is as expected.

Dor1s updated this revision to Diff 114748.Sep 11 2017, 6:36 PM

Re-use AccumulateAllocationsTest.cpp (with -runs=[234]) instead of adding a new
test.

Dor1s added a comment.Sep 11 2017, 6:38 PM

You are right, AccumulateAllocationsTest.cpp works well for that issue. I added a test based on it and removed a new .cpp file previously added.

kcc accepted this revision.Sep 11 2017, 6:39 PM

LGTM

This revision is now accepted and ready to land.Sep 11 2017, 6:39 PM
Dor1s closed this revision.Sep 11 2017, 7:03 PM