[PATCH] D62872: [GWP-ASan] Core Guarded Pool Allocator [4].

[Vlad Tsyrklevich via Phabricator via llvm-commits]

vlad.tsyrklevich added inline comments.


================
Comment at: compiler-rt/lib/gwp_asan/guarded_pool_allocator.cpp:162
+
+  if (Meta->IsDeallocated) {
+    reportError(UPtr, Error::DOUBLE_FREE);
----------------
There's a race checking this and having it set it later in RecordDeallocation.


================
Comment at: compiler-rt/lib/gwp_asan/guarded_pool_allocator.cpp:167
+
+  Meta->RecordDeallocation();
+  markInaccessible(reinterpret_cast<void *>(SlotStart),
----------------
Perhaps add an explicit comment about RecordDeallocation being performed before markInaccessible to make sure a UAF crash has metadata (I've almost changed the ordering by accident but been saved by a similar comment.)


================
Comment at: compiler-rt/lib/gwp_asan/guarded_pool_allocator.cpp:186
+AllocationMetadata *GuardedPoolAllocator::addrToMetadata(uintptr_t Ptr) const {
+  assert(pointerIsMine(reinterpret_cast<void *>(Ptr)));
+  return &Metadata[addrToSlot(Ptr)];
----------------
Put this assert in addrToSlot and then this function is covered as well.


================
Comment at: compiler-rt/lib/gwp_asan/guarded_pool_allocator.h:173
+
+  // Returns the address of to the N-th guarded slot.
+  uintptr_t slotToAddr(size_t N) const;
----------------
s/to //


================
Comment at: compiler-rt/lib/gwp_asan/guarded_pool_allocator.h:244
+  // before GPA::init() is called. This would cause an error in shouldSample(),
+  // where we would calculate modulo zero. This value is set UINT64_MAX, as when
+  // GWP-ASan is disabled, we wish to never spend wasted cycles recalculating
----------------
s/64/32/


================
Comment at: compiler-rt/lib/gwp_asan/tests/basic.cpp:56
+
+TEST_F(CustomGuardedPoolAllocator, AllocTooManySlots) {
+  constexpr unsigned kNumSlots = 128;
----------------
You can just delete the test above as it's a subset of this one.


================
Comment at: compiler-rt/lib/gwp_asan/tests/thread_contention.cpp:82
+
+  runThreadContentionTest(NumThreads, NumIterations, &GPA);
+}
----------------
Given that NumIterations >> NumThreads (e.g. NumSlots), isn't this test redundant?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62872