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

[Dor1s]

Summary

Fuzzer::TryDetectingAMemoryLeak may call ExecuteCallback which would
increment TotalNumberOfRuns, but it doesn't respect Options.MaxNumberOfRuns
value specified by a user.

Context: https://github.com/google/oss-fuzz/issues/822#issuecomment-328153970

[kcc]

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

[Dor1s]

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

[kcc]

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

[Dor1s]

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

[Dor1s]

Added the test.

[Dor1s]

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]

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

Testing Time: 15.34s
  Expected Passes    : 132

[kcc]

test/fuzzer/max-number-of-runs.test

3 ↗

(On Diff #114423)

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?

3 ↗

(On Diff #114423)

CHECK: line

[Dor1s]

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

[Dor1s]

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]

Capitalize the first letter of local variable

[Dor1s]

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

[kcc]

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]

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

[Dor1s]

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]

LGTM

lib/fuzzer/FuzzerLoop.cpp

 }
 
 // Tries detecting a memory leak on the particular input that we have just
 // executed before calling this function.
 void Fuzzer::TryDetectingAMemoryLeak(const uint8_t *Data, size_t Size,
                                      bool DuringInitialCorpusExecution) {
   if (!HasMoreMallocsThanFrees) return;  // mallocs==frees, a leak is unlikely.
   if (!Options.DetectLeaks) return;
+  if (TotalNumberOfRuns >= Options.MaxNumberOfRuns) return;
   if (!&(EF->__lsan_enable) || !&(EF->__lsan_disable) ||
       !(EF->__lsan_do_recoverable_leak_check))
     return;  // No lsan.
   // Run the target once again, but with lsan disabled so that if there is
   // a real leak we do not report it twice.
   EF->__lsan_disable();
   ExecuteCallback(Data, Size);
   EF->__lsan_enable();