[PATCH] D60593: [GwpAsan] Introduce GWP-ASan.

[Vlad Tsyrklevich via Phabricator via llvm-commits]

vlad.tsyrklevich added a comment.

Some thoughts after taking a quick glance at this:

- AllocationStrategy/AlignmentStrategy don’t seem useful to me as they are. Are they just for testing? Could we get rid of them by allocating full-page allocations instead in the under/overflow tests instead? They complicate the implementation and I don’t know why people would use them outside of flipping a flag to say “full right-alignment” instead of whatever reasonable alignment strategy we choose for them. Perhaps collapse both into a single bool PerfectlyRightAlign flag and just have the default alignment strategy be something we choose?
- What is the plan for adding stack trace support?



================
Comment at: compiler-rt/cmake/config-ix.cmake:676
+if (COMPILER_RT_HAS_SANITIZER_COMMON AND GWP_ASAN_SUPPORTED_ARCH AND
+    OS_NAME MATCHES "Android|Linux|NetBSD|FreeBSD|OpenBSD")
+  set(COMPILER_RT_HAS_GWP_ASAN TRUE)
----------------
I would remove the BSDs and let them add it themselves, otherwise we risk breaking them without having tested it. (e.g. they may have system calls like gettid() with different names/APIs)


================
Comment at: compiler-rt/lib/gwp_asan/guarded_pool_allocator.cpp:55
+  // Allocate memory and set up the free pages queue.
+  FreePages = reinterpret_cast<std::size_t *>(
+      malloc(NumGuardedSlots * sizeof(std::size_t)));
----------------
Is std::size_t a scudo thing? Otherwise I'd just replace with just size_t?


================
Comment at: compiler-rt/lib/gwp_asan/guarded_pool_allocator.cpp:153
+
+bool GuardedPoolAllocator::reserveSlotAndMetadata(std::size_t *PageIndex,
+                                                  std::size_t *MetadataIndex) {
----------------
Given that there is no page/metadata index distinction right now I'd make this just one parameter.


================
Comment at: compiler-rt/lib/gwp_asan/guarded_pool_allocator.cpp:321
+  } else {
+    Meta = addrToMetadata(AccessPtr);
+  }
----------------
The rest of this routine assumes Meta is valid but it may not be (e.g. a wild access to a page that's never been allocated yet.)


================
Comment at: compiler-rt/lib/gwp_asan/guarded_pool_allocator.h:57
+
+    // Should be compiled to a inlined jump table, resulting in a single
+    // indirect branch overhead, no matter what the value of the sample
----------------
The Chrome sampling code does this more simply by treating 0 as "sample again" and then returning whether the next SampleCounter is equal to 1 to indicate "sample now" https://github.com/chromium/chromium/blob/master/components/gwp_asan/client/sampling_allocator_shims.cc#L50


================
Comment at: compiler-rt/lib/gwp_asan/guarded_pool_allocator.h:67
+    case 1:
+      NextSampleCounter = (Random::getRandomUnsigned64() % SampleRate) + 1;
+      return true;
----------------
Random::getRandomUnsigned64() % SampleRate means we are sampling more often than the actual sampling rate. This should be % (SampleRate * 2), or, even better, sample from a geometric distribution


================
Comment at: compiler-rt/lib/gwp_asan/guarded_pool_allocator_posix.cpp:88
+
+uint64_t GuardedPoolAllocator::getThreadID() { return getpid(); }
+
----------------
gettid()?


================
Comment at: compiler-rt/lib/scudo/scudo_allocator.cpp:306
+      void *Ptr = GuardedAlloc.allocate(Size);
+      if (Ptr)
+        return Ptr;
----------------
Can collapse this into a single if (void *Ptr = GuardedAlloc.allocate(Size))


================
Comment at: compiler-rt/lib/scudo/scudo_allocator.cpp:491
+      void *NewPtr = allocate(NewSize, MinAlignment, FromMalloc);
+      if (NewPtr) {
+        gwp_asan::AllocationMetadata *Meta =
----------------
if (!NewPtr) then we never deallocate? Also is it possible that this will be reached with NewSize=0 like in the realloc() case or not? e.g. do we need to explicitly handle it here


================
Comment at: compiler-rt/lib/scudo/scudo_allocator.cpp:494
+            GuardedAlloc.addrToMetadata(reinterpret_cast<uintptr_t>(OldPtr));
+        memcpy(NewPtr, OldPtr, Meta->Size);
+        GuardedAlloc.deallocate(OldPtr);
----------------
std::min(Meta->Size, NewSize)


================
Comment at: compiler-rt/lib/scudo/scudo_allocator.cpp:545
+          GuardedAlloc.addrToMetadata(reinterpret_cast<uintptr_t>(Ptr));
+      return Meta->RealSize;
+    }
----------------
I would return Size here, there is no reason for users of this API to see more than that. For one I'm not sure how other allocators handle the slack between Size and RealSize and things like reallocations.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60593/new/

https://reviews.llvm.org/D60593