This is an archive of the discontinued LLVM Phabricator instance.

[GWP-ASan] Core Guarded Pool Allocator [4].
ClosedPublic

Authored by hctim on Jun 4 2019, 12:46 PM.

Details

Summary

See D60593 for further information.

This patch introduces the core of GWP-ASan, being the guarded pool allocator. This class contains the logic for creating and maintaining allocations in the guarded pool. Its public interface is to be utilised by supporting allocators in order to provide sampled guarded allocation behaviour.

This patch also contains basic functionality tests of the allocator as unittests. The error-catching behaviour will be tested in upcoming patches that use Scudo as an implementing allocator.

Diff Detail

Repository
rL LLVM

Event Timeline

hctim created this revision.Jun 4 2019, 12:46 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald Transcript
Herald added subscribers: llvm-commits, Restricted Project, jfb and 3 others. · View Herald Transcript
hctim removed a reviewer: jfb.Jun 4 2019, 1:20 PM
compiler-rt/lib/gwp_asan/guarded_pool_allocator.cpp
162 ↗(On Diff #203000)

There's a race checking this and having it set it later in RecordDeallocation.

167 ↗(On Diff #203000)

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.)

186 ↗(On Diff #203000)

Put this assert in addrToSlot and then this function is covered as well.

compiler-rt/lib/gwp_asan/guarded_pool_allocator.h
173 ↗(On Diff #203000)

s/to //

244 ↗(On Diff #203000)

s/64/32/

compiler-rt/lib/gwp_asan/tests/basic.cpp
56 ↗(On Diff #203000)

You can just delete the test above as it's a subset of this one.

compiler-rt/lib/gwp_asan/tests/thread_contention.cpp
82 ↗(On Diff #203000)

Given that NumIterations >> NumThreads (e.g. NumSlots), isn't this test redundant?

hctim marked 10 inline comments as done.Jun 4 2019, 7:05 PM
hctim added inline comments.
compiler-rt/lib/gwp_asan/guarded_pool_allocator.cpp
162 ↗(On Diff #203000)

Darn, I tried to be clever here and not hold the mutex during the expensive call to markInaccessible. Instead, I clearly need to scope this, unlock for the mprotect and then lock the critical path again.

186 ↗(On Diff #203000)

Done. Also added them to getPageAddr() and isGuardPage().

compiler-rt/lib/gwp_asan/tests/thread_contention.cpp
82 ↗(On Diff #203000)

Yep. Removed.

hctim updated this revision to Diff 203056.Jun 4 2019, 7:05 PM
hctim marked 3 inline comments as done.
  • Added GPA.h to CMakeLists.txt
  • Merge branch 'master' into gwp_asan/gpa
  • Vlad's comments.
vlad.tsyrklevich accepted this revision.Jun 5 2019, 12:01 PM

LGTM otherwise

compiler-rt/lib/gwp_asan/guarded_pool_allocator.cpp
162 ↗(On Diff #203000)

One thing to watch out for is that once you add unwinding you're holding the lock across the unwind call as well, which may be an issue depending on how unwinding works.

This revision is now accepted and ready to land.Jun 5 2019, 12:01 PM
hctim updated this revision to Diff 203227.Jun 5 2019, 12:38 PM
hctim marked 2 inline comments as done.
  • Merge branch 'master' into gwp_asan/gpa
  • Merged master. Added a TODO.
This revision was automatically updated to reflect the committed changes.