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

[Mitch Phillips via Phabricator via llvm-commits]

hctim added inline comments.


================
Comment at: compiler-rt/lib/gwp_asan/guarded_pool_allocator.cpp:34
+
+  if (Opts.InstallSignalHandlers)
+    installSignalHandlers();
----------------
vlad.tsyrklevich wrote:
> Any reason to do this here and not until just before IsInitialised is set? If we're very unlucky and hit an exception  on the next instruction the signal handler will fault on calling Printf, perhaps other such conditions will be introduced in the future as well.
Nope, have moved it. 


================
Comment at: compiler-rt/lib/gwp_asan/guarded_pool_allocator.cpp:125
+
+  freeSlot(addrToSlot(UPtr));
+  markInaccessible(reinterpret_cast<void *>(Page), maximumAllocationSize());
----------------
vlad.tsyrklevich wrote:
> Call freeSlot after markInaccessible, otherwise you introduce a very unlikely race where the slot is freed, then another threads takes it and marks the page RW before it's marked RO below.
Nice catch, thanks!


================
Comment at: compiler-rt/lib/gwp_asan/guarded_pool_allocator.h:74
+    uintptr_t P = reinterpret_cast<uintptr_t>(Ptr);
+    return GetGuardedPagePool() <= P && P < GetGuardedPagePoolEnd();
+  }
----------------
vlad.tsyrklevich wrote:
> We've given GuardedPagePool/GuardedPagePoolEnd the atomic treatment but since we access them sequentially it's possible that we get the sequence:
> T1 Load GuardedPagePool
> T2 Lazy Init
> T1 Load GuardedPagePoolEnd
> 
> and give a bad value if the pointer is between 0 and GuardedPagePoolEnd. It seems like these values should not be atomic but that instead we are forced to perform an IsInitialised check first instead.
Alternatively, we could set `GuardedPagePool = UINTPTR_MAX` in the constructor for value-initialisation, and not have the overhead of the check. Do you see any problems with this?


================
Comment at: compiler-rt/lib/gwp_asan/guarded_pool_allocator.h:234
+  // recalcualting the sample rate.
+  ALIGNED(8) uint64_t AdjustedSampleRate = UINT64_MAX - 1;
+  // Thread-local decrementing counter that indicates that a given allocation
----------------
vlad.tsyrklevich wrote:
> ALIGNED(8) seems unnecessary for a uint64_t?
On 32-bit platforms, 64-bit values aren't guaranteed to have 8 byte alignment (strangely enough).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60593