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

[Vlad Tsyrklevich via Phabricator via llvm-commits]

vlad.tsyrklevich added inline comments.


================
Comment at: compiler-rt/lib/gwp_asan/allocation_metadata.h:37
+
+  // Is this allocation left aligned (true) or right aligned (false).
+  bool LeftAlignedAllocation = false;
----------------
This can be removed (now that it's no longer used in the crash identification logic it's only used locally in a single function.)


================
Comment at: compiler-rt/lib/gwp_asan/guarded_pool_allocator.cpp:85
+
+  std::size_t RequestedSize = Size;
+  applyAllocationStrategy(&Size, &Ptr, &Meta->LeftAlignedAllocation);
----------------
RequestedSize seems vestigial, the returned value of Size is no longer used so it could be removed and RequestedSize replaced with Size.


================
Comment at: compiler-rt/lib/gwp_asan/guarded_pool_allocator.cpp:175
+    if (*Size <= 2)
+      *Size = 2;
+    else if (*Size <= 4)
----------------
This should really be blank (e.g. if Size==1 there's no reason to make off-by-ones not crash.)


================
Comment at: compiler-rt/lib/gwp_asan/guarded_pool_allocator.cpp:215
+
+  if (Ptr % PageSize <= PageSize / 2)
+    return addrToSlot(Ptr - PageSize); // Round down.
----------------
This logic won't work for the first half of the very first guard page or  the second half of the last guard page, they need to be special cased (this can be rolled into the GuardedPagePool/GuardedPagePoolEnd checks at the top.)


================
Comment at: compiler-rt/lib/gwp_asan/guarded_pool_allocator.cpp:233
+
+    // Ensure that this slot was allocated once upon a time.
+    if (!Meta->Addr) {
----------------
It should be an invalid free() if it actually crashed on free(). It doesn't seem possible to ever hit this routine for an INVALID_FREE since INVALID_FREE is always reported directly with an error type.


================
Comment at: compiler-rt/lib/gwp_asan/guarded_pool_allocator.cpp:296
+    // allocated. If this is the case, there is no valid metadata.
+    if (Meta == nullptr)
+      exit(EXIT_FAILURE);
----------------
Meta won't be nullptr in this case though right? You need to look into the metadata to check that it's never been allocated.


================
Comment at: compiler-rt/lib/gwp_asan/guarded_pool_allocator.h:55
+    if (UNLIKELY(SampleRate == 0))
+      return false;
+
----------------
hctim wrote:
> vlad.tsyrklevich wrote:
> > morehouse wrote:
> > > morehouse wrote:
> > > > hctim wrote:
> > > > > eugenis wrote:
> > > > > > morehouse wrote:
> > > > > > > It would be nice to avoid this check on the fast path.
> > > > > > > 
> > > > > > > Perhaps we can do `rand() % (SampleRate + 1)` and then check `SampleRate !=0` only just before we would return true.
> > > > > > Or it could be another special value of NextSampleCounter, which could also serve as a way to temporarily disable sampling in a thread.
> > > > > > 
> > > > > Yes and no. We would have to set `SampleRate = Opts.SampleRate - 1` to get precise sampling behaviour, which would make a sample rate of always on ("1") be the same as uninitialised.
> > > > > 
> > > > > I've added another flag `IsInitialised` in order to counteract this.
> > > > This doesn't seem any better.  We're still checking for initialization on the fast path.
> > > Sorry, disregard the last comment; it's actually not on the fast path.  This looks like it will have better perf.
> > Is it not still on the fast path? I thought we'd want the if (IsInitialized) check to be in if (UNLIKELY(counter ==0)) to get it out of there?
> Tying to the discussion in `scudo_allocator:236`, I think that we should scrap calling `malloc()` from within GwpAsan's init. This would allow us to not have an initialisation check entirely, with the strict requirement that `GuardedPoolAllocator::init()` is called before any malloc takes place. WDYT?
I'm fine with getting rid of it. I do think it's much nicer to use the simple malloc primitives, but it's not the end of the world.


================
Comment at: compiler-rt/lib/gwp_asan/options.inc:15
+
+GWP_ASAN_OPTION(
+    bool, RightAlignAll, false,
----------------
I personally would also eliminate this option, most users shouldn't have to think about the likelihood of overflow versus underflow.


================
Comment at: compiler-rt/lib/scudo/scudo_allocator.cpp:501
+      gwp_asan::AllocationMetadata *Meta =
+          GuardedAlloc.addrToMetadata(reinterpret_cast<uintptr_t>(OldPtr));
+      void *NewPtr = allocate(NewSize, MinAlignment, FromMalloc);
----------------
Unlikely but there may be no metadata here for a bad realloc. If you implement the logic for checking the pointer as I mention below, you may be able to piggy back on that here and just called GPA.GetSize(ptr) here and get that check for free instead of accessing the metadata. (This gives the additional added advantage of being able to place addrToMetadata as a private member and hiding the metadata from upstream users.)


================
Comment at: compiler-rt/lib/scudo/scudo_allocator.cpp:556
+          GuardedAlloc.addrToMetadata(reinterpret_cast<uintptr_t>(Ptr));
+      return Meta->Size;
+    }
----------------
I would check that ptr is allocated and matches the metadata here. (It turns out that e.g. on macOS the system libs can reach this with an incorrect pointer and on that platform they expect you to return 0. For now I'd just assert())


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60593