Skip to content

Re-enable the debug ReentrancyGuard in TCMalloc's slow paths. - #1008

Draft
copybara-service[bot] wants to merge 1 commit into
masterfrom
test_983923033
Draft

copybara-service[bot] wants to merge 1 commit into
masterfrom
test_983923033

Conversation

@copybara-service

Copy link
Copy Markdown

Re-enable the debug ReentrancyGuard in TCMalloc's slow paths.

In debug builds, directly guard against reentrancy. AllocationGuard installs a
new hook while we hold locks, which catches bugs inside TCMalloc, but cannot
detect misuse by allocation/deallocation hooks or by code interposing
low-level C functions (open(), close(), pthread_setspecific()) that allocates
while TCMalloc is on the stack.

ReentrancyGuard and tcmalloc_reentrancy_count have been declared in
internal/logging.h since the check was first landed, but with the TC_CHECK_EQ
replaced by a TODO and no definition or users. This restores the definition,
the check, the guards in tcmalloc.cc, and testing/reentrancy_test.cc, with two
changes to the original placement:

  • Guards live only in the ABSL_ATTRIBUTE_NOINLINE slow paths
    (slow_alloc_small, alloc_small_sampled_hooks_or_perthread,
    slow_alloc_large, FreeSmallSlow, FreeWithHooksOrPerThread,
    InvokeHooksAndFreePages) and the in-place branch of do_realloc, rather
    than at the top of fast_alloc/do_free/do_free_with_size. The fast
    paths make no calls that can reenter, so this loses no coverage, and it
    keeps TCMALLOC_MUSTTAIL unconditional. Where a slow path sibling-calls
    another guarded slow path, the guard is constructed after that branch so
    the callee's frame replaces the caller's exactly as before.

    This matters because an RAII guard pins its frame until the destructor
    runs. With the original placement, __libc_malloc could no longer
    tail-call slow_alloc_small (jmp became call; decl %fs:count; ret),
    adding one frame between the allocation entry point and every new hook.
    The heap checker only ignores JVM allocations when a libjvm.so frame
    appears within the top 4 frames of its raw stack
    ([NewHook, HookList::InvokeSlow, alloc_small_sampled_hooks_or_perthread, caller]), so the extra frame pushed libjvm to index 4 and every JVM
    allocation in every fastbuild Java test was reported as a leak.

  • tcmalloc_reentrancy_count is declared ABSL_ATTRIBUTE_INITIAL_EXEC,
    matching tcmalloc_slabs, __rseq_abi, tcmalloc_sampler, and
    ThreadCache::thread_local_data_. The guard runs on every slow-path
    allocation, and a general-dynamic access through __tls_get_addr can
    itself allocate when the DTV needs to grow after a dlopen, which is
    exactly the reentrancy the guard is meant to catch.

Under NDEBUG, ReentrancyGuard is an empty constexpr type and codegen is
unchanged (fast_path.*.golden are byte-identical).

In debug builds, directly guard against reentrancy.  AllocationGuard installs a
new hook while we hold locks, which catches bugs inside TCMalloc, but cannot
detect misuse by allocation/deallocation hooks or by code interposing
low-level C functions (open(), close(), pthread_setspecific()) that allocates
while TCMalloc is on the stack.

`ReentrancyGuard` and `tcmalloc_reentrancy_count` have been declared in
internal/logging.h since the check was first landed, but with the `TC_CHECK_EQ`
replaced by a TODO and no definition or users.  This restores the definition,
the check, the guards in tcmalloc.cc, and testing/reentrancy_test.cc, with two
changes to the original placement:

*   Guards live only in the `ABSL_ATTRIBUTE_NOINLINE` slow paths
    (`slow_alloc_small`, `alloc_small_sampled_hooks_or_perthread`,
    `slow_alloc_large`, `FreeSmallSlow`, `FreeWithHooksOrPerThread`,
    `InvokeHooksAndFreePages`) and the in-place branch of `do_realloc`, rather
    than at the top of `fast_alloc`/`do_free`/`do_free_with_size`.  The fast
    paths make no calls that can reenter, so this loses no coverage, and it
    keeps `TCMALLOC_MUSTTAIL` unconditional.  Where a slow path sibling-calls
    another guarded slow path, the guard is constructed after that branch so
    the callee's frame replaces the caller's exactly as before.

    This matters because an RAII guard pins its frame until the destructor
    runs.  With the original placement, `__libc_malloc` could no longer
    tail-call `slow_alloc_small` (`jmp` became `call; decl %fs:count; ret`),
    adding one frame between the allocation entry point and every new hook.
    The heap checker only ignores JVM allocations when a libjvm.so frame
    appears within the top 4 frames of its raw stack
    (`[NewHook, HookList::InvokeSlow, alloc_small_sampled_hooks_or_perthread,
    caller]`), so the extra frame pushed libjvm to index 4 and every JVM
    allocation in every fastbuild Java test was reported as a leak.

*   `tcmalloc_reentrancy_count` is declared `ABSL_ATTRIBUTE_INITIAL_EXEC`,
    matching `tcmalloc_slabs`, `__rseq_abi`, `tcmalloc_sampler`, and
    `ThreadCache::thread_local_data_`.  The guard runs on every slow-path
    allocation, and a general-dynamic access through `__tls_get_addr` can
    itself allocate when the DTV needs to grow after a `dlopen`, which is
    exactly the reentrancy the guard is meant to catch.

Under `NDEBUG`, `ReentrancyGuard` is an empty constexpr type and codegen is
unchanged (fast_path.*.golden are byte-identical).

PiperOrigin-RevId: 983923033
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant