libFuzzer: prevent irrelevant strings from leaking into auto-dictionary

[pdknsk]

Summary

This is a fix for bug 37047.

https://bugs.llvm.org/show_bug.cgi?id=37047

Implemented by basically reversing the logic. Previously all strings were considered, with some operations excluded. Now strings are excluded by default, and only strings during the CB considered.

[kcc]

Why is it safe to remove ScopedDoingMyOwnMemOrStr from the places you've removed it from?

[pdknsk]

In D48800#1150324, @kcc wrote:

Why is it safe to remove ScopedDoingMyOwnMemOrStr from the places you've removed it from?

Note that this removes ScopedDoingMyOwnMemOrStr completely. It's safe because the functions using it run outside the callback. MakeDictionaryEntryFromCMP before, operator== (used in ContainsWord) after, and operator< appears unused.

It'd be different if the functions (TPC.AddValueForMemcmp, TPC.MMT.Add) used inside the hooks (recursively) triggered the hooks, but that's not the case.

[kcc]

In D48800#1150953, @pdknsk wrote:

In D48800#1150324, @kcc wrote:

Why is it safe to remove ScopedDoingMyOwnMemOrStr from the places you've removed it from?

Note that this removes ScopedDoingMyOwnMemOrStr completely. It's safe because the functions using it run outside the callback. MakeDictionaryEntryFromCMP before, operator== (used in ContainsWord) after, and operator< appears unused.

It'd be different if the functions (TPC.AddValueForMemcmp, TPC.MMT.Add) used inside the hooks (recursively) triggered the hooks, but that's not the case.

Thanks for checking. Indeed, looks like ScopedDoingMyOwnMemOrStr is now redundant.
I've just removed the stale operator<

Why do you need the new variable InCB?
Will the existing RunningCB not work?

[pdknsk]

In D48800#1151316, @kcc wrote:

Why do you need the new variable InCB?
Will the existing RunningCB not work?

To be honest, my C++ knowledge becomes wobbly here. I don't think the hooks can access Fuzzer (and its RunningCB).

[kcc]

In D48800#1152873, @pdknsk wrote:

In D48800#1151316, @kcc wrote:

Why do you need the new variable InCB?
Will the existing RunningCB not work?

To be honest, my C++ knowledge becomes wobbly here. I don't think the hooks can access Fuzzer (and its RunningCB).

you will need to add one accessor method, just like in your current variant:

bool isRunningCB() const { return RunningCB; }

[pdknsk]

In D48800#1154720, @kcc wrote:

you will need to add one accessor method, just like in your current variant:

bool isRunningCB() const { return RunningCB; }

That I know. I meant accessing Fuzzer (F). I have a construct that stores *F on TracePC (TPC), but I'm not sure that's good.

[kcc]

yea, you are right, you can't directly use F->RunningCB if you don't have F visible.
But I don't want to have two copies of this flag, one in Fuzzer and one in TracePC.

I would rather just have this flag as a global -- just rename the already existing ScopedDoingMyOwnMemOrStr
into something more suitable (ScopedRunningUserCallback) and get rid of RunningCB.

[pdknsk]

I've removed the performance claim from the summary, because it's not so easy to prove. The problem is that runs even with the same seed are not comparable before and after. I've run the same target using the same 5 seeds for 60 seconds, with the following results.

(before)

1: #455495 DONE cov: 514 ft: 1838 corp: 408/7693b lim: 53 exec/s: 7467 rss: 297Mb
2: #600688 DONE cov: 559 ft: 2052 corp: 479/9439b lim: 48 exec/s: 9847 rss: 197Mb
3: #695888 DONE cov: 630 ft: 2913 corp: 793/11841b lim: 38 exec/s: 11408 rss: 213Mb
4: #273007 DONE cov: 455 ft: 1530 corp: 349/5243b lim: 25 exec/s: 4475 rss: 193Mb
5: #729988 DONE cov: 1198 ft: 4092 corp: 897/16Kb lim: 29 exec/s: 11967 rss: 278Mb

(after)

1: #236342 DONE cov: 432 ft: 1451 corp: 320/4767b lim: 29 exec/s: 3874 rss: 246Mb
2: #695694 DONE cov: 636 ft: 2616 corp: 724/12693b lim: 48 exec/s: 11404 rss: 248Mb
3: #933497 DONE cov: 629 ft: 2719 corp: 744/14398b lim: 53 exec/s: 15303 rss: 269Mb
4: #865391 DONE cov: 1211 ft: 4827 corp: 1102/26Kb lim: 48 exec/s: 14186 rss: 242Mb
5: #694553 DONE cov: 579 ft: 2181 corp: 492/11882b lim: 68 exec/s: 11386 rss: 205Mb

[pdknsk]

Even simpler now than it was.

[morehouse]

LGTM

[kcc]

LGTM

Matt, please test locally and land.

[george.karpenkov]

@morehouse @vitalybuka @kcc this breaks shrink.test, namely, the run line:

/Volumes/Transcend/code/llvm/relbuild-with-asserts/projects/compiler-rt/test/fuzzer/Output/shrink.test.tmp-ShrinkControlFlowTest -seed=1 -exit_on_item=0eb8e4ed029b774d80f2b66408203801cb982a60 -runs=1000000 -shrink=1 -reduce_inputs=0

Any ideas on what went wrong here?

[george.karpenkov]

@kcc I can fix the test by doubling the limit given to -runs, not sure whether it's a correct course of actions.

[george.karpenkov]

Moreover, the test passes on x86_64, but fails on x86_64h.

[morehouse]

We should probably just increase the runs for shrink.test. This patch changes how items are added to libFuzzer's dictionary, which will change the exact sequence of inputs libFuzzer tries for a given seed.

When I landed this I had to modify three-bytes.test as well because this patch made libFuzzer hit a line sooner than we expected in that test.

compiler-rt/trunk/lib/fuzzer/FuzzerDefs.h

 using Set = std::set<T, std::less<T>, fuzzer_allocator<T>>;
 
 typedef Vector<uint8_t> Unit;
 typedef Vector<Unit> UnitVector;
 typedef int (*UserCallback)(const uint8_t *Data, size_t Size);
 
 int FuzzerDriver(int *argc, char ***argv, UserCallback Callback);
 
-struct ScopedDoingMyOwnMemOrStr {
-  ScopedDoingMyOwnMemOrStr() { DoingMyOwnMemOrStr++; }
-  ~ScopedDoingMyOwnMemOrStr() { DoingMyOwnMemOrStr--; }
-  static int DoingMyOwnMemOrStr;
-};
-
 inline uint8_t  Bswap(uint8_t x)  { return x; }
 inline uint16_t Bswap(uint16_t x) { return __builtin_bswap16(x); }
 inline uint32_t Bswap(uint32_t x) { return __builtin_bswap32(x); }
 inline uint64_t Bswap(uint64_t x) { return __builtin_bswap64(x); }
 
 uint8_t *ExtraCountersBegin();
 uint8_t *ExtraCountersEnd();
 void ClearExtraCounters();
 
+extern bool RunningUserCallback;
+
 }  // namespace fuzzer
 
 #endif  // LLVM_FUZZER_DEFS_H

compiler-rt/trunk/lib/fuzzer/FuzzerDictionary.h

public:
 
   void Set(const uint8_t *B, uint8_t S) {
     assert(S <= kMaxSize);
     memcpy(Data, B, S);
     Size = S;
   }
 
   bool operator==(const FixedWord<kMaxSize> &w) const {
-    ScopedDoingMyOwnMemOrStr scoped_doing_my_own_mem_os_str;
     return Size == w.Size && 0 == memcmp(Data, w.Data, Size);
   }
 
   static size_t GetMaxSize() { return kMaxSize; }
   const uint8_t *data() const { return Data; }
   uint8_t size() const { return Size; }
 
 private:

compiler-rt/trunk/lib/fuzzer/FuzzerInternal.h

private:
   static void StaticDeathCallback();
   void DumpCurrentUnit(const char *Prefix);
   void DeathCallback();
 
   void AllocateCurrentUnitData();
   uint8_t *CurrentUnitData = nullptr;
   std::atomic<size_t> CurrentUnitSize;
   uint8_t BaseSha1[kSHA1NumBytes];  // Checksum of the base unit.
-  bool RunningCB = false;
 
   bool GracefulExitRequested = false;
 
   size_t TotalNumberOfRuns = 0;
   size_t NumberOfNewUnitsAdded = 0;
 
   size_t LastCorpusUpdateRun = 0;
 

compiler-rt/trunk/lib/fuzzer/FuzzerLoop.cpp

 
 namespace fuzzer {
 static const size_t kMaxUnitSizeToPrint = 256;
 
 thread_local bool Fuzzer::IsMyThread;
 
 SharedMemoryRegion SMR;
 
+bool RunningUserCallback = false;
+
 // Only one Fuzzer per process.
 static Fuzzer *F;
 
 // Leak detection is expensive, so we first check if there were more mallocs
 // than frees (using the sanitizer malloc hooks) and only then try to call lsan.
 struct MallocFreeTracer {
   void Start(int TraceLevel) {
     this->TraceLevel = TraceLevel;
Printf("NOTE: libFuzzer has rudimentary signal handlers.\n"
          "crash reports.\n");
   Printf("SUMMARY: libFuzzer: deadly signal\n");
   DumpCurrentUnit("crash-");
   PrintFinalStats();
   _Exit(Options.ErrorExitCode); // Stop right now.
 }
 
 void Fuzzer::ExitCallback() {
-  if (!RunningCB)
+  if (!RunningUserCallback)
     return; // This exit did not come from the user callback
   if (EF->__sanitizer_acquire_crash_state &&
       !EF->__sanitizer_acquire_crash_state())
     return;
   Printf("==%lu== ERROR: libFuzzer: fuzz target exited\n", GetPid());
   PrintStackTrace();
   Printf("SUMMARY: libFuzzer: fuzz target exited\n");
   DumpCurrentUnit("crash-");

 NO_SANITIZE_MEMORY
 void Fuzzer::AlarmCallback() {
   assert(Options.UnitTimeoutSec > 0);
   // In Windows Alarm callback is executed by a different thread.
 #if !LIBFUZZER_WINDOWS
   if (!InFuzzingThread())
     return;
 #endif
-  if (!RunningCB)
+  if (!RunningUserCallback)
     return; // We have not started running units yet.
   size_t Seconds =
       duration_cast<seconds>(system_clock::now() - UnitStartTime).count();
   if (Seconds == 0)
     return;
   if (Options.Verbosity >= 2)
     Printf("AlarmCallback %zd\n", Seconds);
   if (Seconds >= (size_t)Options.UnitTimeoutSec) {
void Fuzzer::PrintPulseAndReportSlowInput(const uint8_t *Data, size_t Size) {
   }
 }
 
 void Fuzzer::CheckForUnstableCounters(const uint8_t *Data, size_t Size) {
   auto CBSetupAndRun = [&]() {
     ScopedEnableMsanInterceptorChecks S;
     UnitStartTime = system_clock::now();
     TPC.ResetMaps();
-    RunningCB = true;
+    RunningUserCallback = true;
     CB(Data, Size);
-    RunningCB = false;
+    RunningUserCallback = false;
     UnitStopTime = system_clock::now();
   };
 
   // Copy original run counters into our unstable counters
   TPC.InitializeUnstableCounters();
 
   // First Rerun
   CBSetupAndRun();
void Fuzzer::ExecuteCallback(const uint8_t *Data, size_t Size) {
   if (CurrentUnitData && CurrentUnitData != Data)
     memcpy(CurrentUnitData, Data, Size);
   CurrentUnitSize = Size;
   {
     ScopedEnableMsanInterceptorChecks S;
     AllocTracer.Start(Options.TraceMalloc);
     UnitStartTime = system_clock::now();
     TPC.ResetMaps();
-    RunningCB = true;
+    RunningUserCallback = true;
     int Res = CB(DataCopy, Size);
-    RunningCB = false;
+    RunningUserCallback = false;
     UnitStopTime = system_clock::now();
     (void)Res;
     assert(Res == 0);
     HasMoreMallocsThanFrees = AllocTracer.Stop();
   }
   if (!LooseMemeq(DataCopy, Data, Size))
     CrashOnOverwrittenData();
   CurrentUnitSize = 0;

compiler-rt/trunk/lib/fuzzer/FuzzerMutate.cpp

 // It first tries to find one of the arguments (possibly swapped) in the
 // input and if it succeeds it creates a DE with a position hint.
 // Otherwise it creates a DE with one of the arguments w/o a position hint.
 DictionaryEntry MutationDispatcher::MakeDictionaryEntryFromCMP(
     const void *Arg1, const void *Arg2,
     const void *Arg1Mutation, const void *Arg2Mutation,
     size_t ArgSize, const uint8_t *Data,
     size_t Size) {
-  ScopedDoingMyOwnMemOrStr scoped_doing_my_own_mem_os_str;
   bool HandleFirst = Rand.RandBool();
   const void *ExistingBytes, *DesiredBytes;
   Word W;
   const uint8_t *End = Data + Size;
   for (int Arg = 0; Arg < 2; Arg++) {
     ExistingBytes = HandleFirst ? Arg1 : Arg2;
     DesiredBytes = HandleFirst ? Arg2Mutation : Arg1Mutation;
     HandleFirst = !HandleFirst;

compiler-rt/trunk/lib/fuzzer/FuzzerTracePC.h

private:
   uint8_t *Counters() const;
   uintptr_t *PCs() const;
 
   Set<uintptr_t> ObservedPCs;
   Set<uintptr_t> ObservedFuncs;
 
   std::pair<size_t, size_t> FocusFunction = {-1, -1};  // Module and PC IDs.
 
-
   ValueBitMap ValueProfileMap;
   uintptr_t InitialStack;
 };
 
 template <class Callback>
 // void Callback(size_t FirstFeature, size_t Idx, uint8_t Value);
 ATTRIBUTE_NO_SANITIZE_ALL
 void ForEachNonZeroByte(const uint8_t *Begin, const uint8_t *End,

compiler-rt/trunk/lib/fuzzer/FuzzerTracePC.cpp

 // Used by -fsanitize-coverage=stack-depth to track stack depth
 ATTRIBUTE_INTERFACE __attribute__((tls_model("initial-exec")))
 thread_local uintptr_t __sancov_lowest_stack;
 
 namespace fuzzer {
 
 TracePC TPC;
 
-int ScopedDoingMyOwnMemOrStr::DoingMyOwnMemOrStr;
-
 uint8_t *TracePC::Counters() const {
   return __sancov_trace_pc_guard_8bit_counters;
 }
 
 uintptr_t *TracePC::PCs() const {
   return __sancov_trace_pc_pcs;
 }
 

 void __sanitizer_cov_trace_gep(uintptr_t Idx) {
   uintptr_t PC = reinterpret_cast<uintptr_t>(__builtin_return_address(0));
   fuzzer::TPC.HandleCmp(PC, Idx, (uintptr_t)0);
 }
 
 ATTRIBUTE_INTERFACE ATTRIBUTE_NO_SANITIZE_MEMORY
 void __sanitizer_weak_hook_memcmp(void *caller_pc, const void *s1,
                                   const void *s2, size_t n, int result) {
-  if (fuzzer::ScopedDoingMyOwnMemOrStr::DoingMyOwnMemOrStr) return;
+  if (!fuzzer::RunningUserCallback) return;
   if (result == 0) return;  // No reason to mutate.
   if (n <= 1) return;  // Not interesting.
   fuzzer::TPC.AddValueForMemcmp(caller_pc, s1, s2, n, /*StopAtZero*/false);
 }
 
 ATTRIBUTE_INTERFACE ATTRIBUTE_NO_SANITIZE_MEMORY
 void __sanitizer_weak_hook_strncmp(void *caller_pc, const char *s1,
                                    const char *s2, size_t n, int result) {
-  if (fuzzer::ScopedDoingMyOwnMemOrStr::DoingMyOwnMemOrStr) return;
+  if (!fuzzer::RunningUserCallback) return;
   if (result == 0) return;  // No reason to mutate.
   size_t Len1 = fuzzer::InternalStrnlen(s1, n);
   size_t Len2 = fuzzer::InternalStrnlen(s2, n);
   n = std::min(n, Len1);
   n = std::min(n, Len2);
   if (n <= 1) return;  // Not interesting.
   fuzzer::TPC.AddValueForMemcmp(caller_pc, s1, s2, n, /*StopAtZero*/true);
 }
 
 ATTRIBUTE_INTERFACE ATTRIBUTE_NO_SANITIZE_MEMORY
 void __sanitizer_weak_hook_strcmp(void *caller_pc, const char *s1,
                                    const char *s2, int result) {
-  if (fuzzer::ScopedDoingMyOwnMemOrStr::DoingMyOwnMemOrStr) return;
+  if (!fuzzer::RunningUserCallback) return;
   if (result == 0) return;  // No reason to mutate.
   size_t N = fuzzer::InternalStrnlen2(s1, s2);
   if (N <= 1) return;  // Not interesting.
   fuzzer::TPC.AddValueForMemcmp(caller_pc, s1, s2, N, /*StopAtZero*/true);
 }
 
 ATTRIBUTE_INTERFACE ATTRIBUTE_NO_SANITIZE_MEMORY
 void __sanitizer_weak_hook_strncasecmp(void *called_pc, const char *s1,
                                        const char *s2, size_t n, int result) {
-  if (fuzzer::ScopedDoingMyOwnMemOrStr::DoingMyOwnMemOrStr) return;
+  if (!fuzzer::RunningUserCallback) return;
   return __sanitizer_weak_hook_strncmp(called_pc, s1, s2, n, result);
 }
 
 ATTRIBUTE_INTERFACE ATTRIBUTE_NO_SANITIZE_MEMORY
 void __sanitizer_weak_hook_strcasecmp(void *called_pc, const char *s1,
                                       const char *s2, int result) {
-  if (fuzzer::ScopedDoingMyOwnMemOrStr::DoingMyOwnMemOrStr) return;
+  if (!fuzzer::RunningUserCallback) return;
   return __sanitizer_weak_hook_strcmp(called_pc, s1, s2, result);
 }
 
 ATTRIBUTE_INTERFACE ATTRIBUTE_NO_SANITIZE_MEMORY
 void __sanitizer_weak_hook_strstr(void *called_pc, const char *s1,
                                   const char *s2, char *result) {
-  if (fuzzer::ScopedDoingMyOwnMemOrStr::DoingMyOwnMemOrStr) return;
+  if (!fuzzer::RunningUserCallback) return;
   fuzzer::TPC.MMT.Add(reinterpret_cast<const uint8_t *>(s2), strlen(s2));
 }
 
 ATTRIBUTE_INTERFACE ATTRIBUTE_NO_SANITIZE_MEMORY
 void __sanitizer_weak_hook_strcasestr(void *called_pc, const char *s1,
                                       const char *s2, char *result) {
-  if (fuzzer::ScopedDoingMyOwnMemOrStr::DoingMyOwnMemOrStr) return;
+  if (!fuzzer::RunningUserCallback) return;
   fuzzer::TPC.MMT.Add(reinterpret_cast<const uint8_t *>(s2), strlen(s2));
 }
 
 ATTRIBUTE_INTERFACE ATTRIBUTE_NO_SANITIZE_MEMORY
 void __sanitizer_weak_hook_memmem(void *called_pc, const void *s1, size_t len1,
                                   const void *s2, size_t len2, void *result) {
-  if (fuzzer::ScopedDoingMyOwnMemOrStr::DoingMyOwnMemOrStr) return;
+  if (!fuzzer::RunningUserCallback) return;
   fuzzer::TPC.MMT.Add(reinterpret_cast<const uint8_t *>(s2), len2);
 }
 }  // extern "C"

compiler-rt/trunk/test/fuzzer/three-bytes.test

 Tests -use_value_profile=2 (alternative VP metric).
 RUN: %cpp_compiler %S/ThreeBytes.cpp -o %t
 
-RUN:     %run %t -seed=1 -runs=100000
-RUN:     %run %t -seed=1 -runs=100000 -use_value_profile=1
+RUN:     %run %t -seed=1 -runs=30000
+RUN:     %run %t -seed=1 -runs=30000 -use_value_profile=1
 RUN: not %run %t -seed=1 -runs=1000000 -use_value_profile=2 2>&1 | FileCheck %s
 
 CHECK: Test unit written