Page MenuHomePhabricator

[GWP-ASan] Guard against recursive allocs. Pack TLS for perf.
ClosedPublic

Authored by hctim on Jun 24 2019, 1:04 PM.

Details

Summary

Add a recursivity guard for GPA::allocate(). This means that any
recursive allocations will fall back to the supporting allocator. In future
patches, we will introduce stack trace collection support. The unwinder will be
provided by the supporting allocator, and we can't guarantee they don't call
malloc() (e.g. backtrace() on posix may call dlopen(), which may call malloc().

Furthermore, this patch packs the new TLS recursivity guard into a thread local
struct, so that TLS variables should be hopefully not fall across cache lines.

Diff Detail

Repository
rL LLVM

Event Timeline

hctim created this revision.Jun 24 2019, 1:04 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptJun 24 2019, 1:04 PM
Herald added subscribers: llvm-commits, Restricted Project, kubamracek. · View Herald Transcript
hctim retitled this revision from [GWP-ASan] Guard against reentrant allocs. Pack TLS for perf. to [GWP-ASan] Guard against recursive allocs. Pack TLS for perf..Jun 24 2019, 1:08 PM
hctim edited the summary of this revision. (Show Details)
hctim updated this revision to Diff 206287.Jun 24 2019, 1:08 PM
hctim retitled this revision from [GWP-ASan] Guard against recursive allocs. Pack TLS for perf. to [GWP-ASan] Guard against reentrant allocs. Pack TLS for perf..
hctim edited the summary of this revision. (Show Details)
  • /s/reentrant/recursive
hctim retitled this revision from [GWP-ASan] Guard against reentrant allocs. Pack TLS for perf. to [GWP-ASan] Guard against recursive allocs. Pack TLS for perf..Jun 24 2019, 1:08 PM
hctim edited the summary of this revision. (Show Details)
hctim edited the summary of this revision. (Show Details)
eugenis added inline comments.Jun 24 2019, 1:20 PM
compiler-rt/lib/gwp_asan/guarded_pool_allocator.cpp
131 ↗(On Diff #206287)

Set to true in constructor, false in destructor. This !Bool stuff makes the code harder to read for no reason at all.

compiler-rt/lib/gwp_asan/guarded_pool_allocator.h
249 ↗(On Diff #206287)

alignas(8), at least

hctim updated this revision to Diff 206291.Jun 24 2019, 1:28 PM
hctim marked 2 inline comments as done.
  • Add alignment and explicitly set ScopedBoolean values.

Do we need similar protection in deallocate?

hctim added a comment.Jun 24 2019, 2:54 PM

Do we need similar protection in deallocate?

We shouldn't need to. GPA::deallocate() may call malloc() and free(), but can only be called with guarded allocations. Recursion is okay (and desirable, as the unwinder's implementation is okay to be sampled) in deallocate().

Do we need similar protection in deallocate?

We shouldn't need to. GPA::deallocate() may call malloc() and free(), but can only be called with guarded allocations. Recursion is okay (and desirable, as the unwinder's implementation is okay to be sampled) in deallocate().

Don't we hold the Mutex during AllocationMetadata::RecordDeallocation()? Doesn't that mean that the unwinder could cause deadlock during deallocation?

Do we need similar protection in deallocate?

We shouldn't need to. GPA::deallocate() may call malloc() and free(), but can only be called with guarded allocations. Recursion is okay (and desirable, as the unwinder's implementation is okay to be sampled) in deallocate().

Don't we hold the Mutex during AllocationMetadata::RecordDeallocation()? Doesn't that mean that the unwinder could cause deadlock during deallocation?

We can not assume that the unwinder is reentrant, so we need to guard all unwinder calls. We can not really fallback to the system allocator in free(), which means that the rest of the deallocation code will have to be reentrant.

hctim added a comment.Jun 24 2019, 4:51 PM

We can not assume that the unwinder is reentrant, so we need to guard all unwinder calls. We can not really fallback to the system allocator in free(), which means that the rest of the deallocation code will have to be reentrant.

So in summary:

  • The unwinder can call any combination of malloc(), free(), in any way/shape/form it wants. The unwinder is assumed to not be reentrant.
  • GPA::allocate() will automatically fallback to the default allocator if it detects reentrant behaviour.
  • GPA::deallocate() has no way to fallback (as it must deallocate any guarded allocations). In this case, we have to avoid reentrant calls to deallocate(), and so we never call the unwinder recursively.

SGTY?

Also, we don't want to place the reentrant-guard around the entirety of RecordDeallocation() (as then the slot won't be marked as deallocated in the metadata). The ScopedBool has to wrap the call to the unwinder only, and therefore must be in a follow up patch.

hctim updated this revision to Diff 206333.Jun 24 2019, 4:52 PM
  • Added note for future implementation to recursive-guard the unwinder on dealloc.
eugenis accepted this revision.Jun 25 2019, 1:43 PM

LGTM

This revision is now accepted and ready to land.Jun 25 2019, 1:43 PM
This revision was automatically updated to reflect the committed changes.