[scudo] Detach the hooks from Scudo's internal implementation

[Chia-hungDuan]

Summary

Move the invocation of hooks from Scudo internal to wrapper_c.cpp and
wrapper_c_bionic.cpp respectively. Therefore, Scudo's core algorithm
doesnt need to worry about the reentrant of hooks and leave the caring
of reentrant to the hook users.

[Chia-hungDuan]

Note, I'll update scudo_hooks_test.cpp later. Now this is more like a RFC for hook users

[fabio-d]

Adding some preliminary (minor) notes, as I haven't tested this CL on Fuchsia yet.

compiler-rt/lib/scudo/standalone/wrappers_c.inc

34

Should this pass Product as the second argument?

134

nit: This can be moved after the if below

156

Should the hook report the rounded-up size to avoid changing the current behavior? (on the other hand I'd be surprised if any existing user depends on it)

177

Similarly to above, the current behavior is to call the allocate hook first, then then deallocate hook.

nit: the two NewPtr != ptr checks can be grouped under only one if

[Chia-hungDuan]

Address review comment.

Still haven't changed the test cases. Will wait the opinion of @chelfi who is
using the hooks and wrote the tests

compiler-rt/lib/scudo/standalone/wrappers_c.inc

156

thanks, the bug from copypasta!

177

Order swapped.

I may prefer single if statement which has less indent.

[chelfi]

Thanks @Chia-hungDuan for looking into this!

Those changes look good to me. Do you also need to change wrappers_cpp.cpp similarly?

[Chia-hungDuan]

Remove scudo_hook_test.cpp and move the hook tests to wrappers_c_test.cpp and wrappers_cpp_test.cpp

[Chia-hungDuan]

In D152188#4399536, @chelfi wrote:

Thanks @Chia-hungDuan for looking into this!

Those changes look good to me. Do you also need to change wrappers_cpp.cpp similarly?

Done.

[hctim]

LGTM

[chelfi]

LGTM, thanks for working on that, @Chia-hungDuan .

I have not tested this patch yet on Fuchsia, but I intend to do so shortly this week.

[Chia-hungDuan]

Rebase

[Chia-hungDuan]

FYI, @chelfi @fabio-d, I'm planning to submit this change by the end of this week.

[cferris]

That sounds like a good plan.

compiler-rt/lib/scudo/standalone/combined.h

if (UNLIKELY(Alignment > MaxAlignment)) {
       reportAlignmentTooBig(Alignment, MaxAlignment);
     }
     if (Alignment < MinAlignment)
       Alignment = MinAlignment;
 
 #ifdef GWP_ASAN_HOOKS
     if (UNLIKELY(GuardedAlloc.shouldSample())) {
       if (void *Ptr = GuardedAlloc.allocate(Size, Alignment)) {
-        if (UNLIKELY(&__scudo_allocate_hook))
-          __scudo_allocate_hook(Ptr, Size);
         Stats.lock();
         Stats.add(StatAllocated, GuardedAllocSlotSize);
         Stats.sub(StatFree, GuardedAllocSlotSize);
         Stats.unlock();
         return Ptr;
       }
     }
 #endif // GWP_ASAN_HOOKS
#endif // GWP_ASAN_HOOKS
     Header.ClassId = ClassId & Chunk::ClassIdMask;
     Header.State = Chunk::State::Allocated;
     Header.OriginOrWasZeroed = Origin & Chunk::OriginMask;
     Header.SizeOrUnusedBytes =
         (ClassId ? Size : SecondaryBlockEnd - (UserPtr + Size)) &
         Chunk::SizeOrUnusedBytesMask;
     Chunk::storeHeader(Cookie, Ptr, &Header);
 
-    if (UNLIKELY(&__scudo_allocate_hook))
-      __scudo_allocate_hook(TaggedPtr, Size);
-
     return TaggedPtr;
   }
 
   NOINLINE void deallocate(void *Ptr, Chunk::Origin Origin, uptr DeleteSize = 0,
                            UNUSED uptr Alignment = MinAlignment) {
     // For a deallocation, we only ensure minimal initialization, meaning thread
     // local data will be left uninitialized for now (when using ELF TLS). The
     // fallback cache will be used instead. This is a workaround for a situation
     // where the only heap operation performed in a thread would be a free past
     // the TLS destructors, ending up in initialized thread specific data never
     // being destroyed properly. Any other heap operation will do a full init.
     initThreadMaybe(/*MinimalInit=*/true);
 
-    if (UNLIKELY(&__scudo_deallocate_hook))
-      __scudo_deallocate_hook(Ptr);
-
     if (UNLIKELY(!Ptr))
       return;
 
 #ifdef GWP_ASAN_HOOKS
     if (UNLIKELY(GuardedAlloc.pointerIsMine(Ptr))) {
       GuardedAlloc.deallocate(Ptr);
       Stats.lock();
       Stats.add(StatFree, GuardedAllocSlotSize);
#endif // GWP_ASAN_HOOKS
 
     // Otherwise we allocate a new one, and deallocate the old one. Some
     // allocators will allocate an even larger chunk (by a fixed factor) to
     // allow for potential further in-place realloc. The gains of such a trick
     // are currently unclear.
     void *NewPtr = allocate(NewSize, Chunk::Origin::Malloc, Alignment);
     if (LIKELY(NewPtr)) {
       memcpy(NewPtr, OldTaggedPtr, Min(NewSize, OldSize));
-      if (UNLIKELY(&__scudo_deallocate_hook))
-        __scudo_deallocate_hook(OldTaggedPtr);
       quarantineOrDeallocateChunk(Options, OldTaggedPtr, &OldHeader, OldSize);
     }
     return NewPtr;
   }
 
   // TODO(kostyak): disable() is currently best-effort. There are some small
   //                windows of time when an allocation could still succeed after
   //                this function finishes. We will revisit that later.

compiler-rt/lib/scudo/standalone/wrappers_c.cpp

 //===-- wrappers_c.cpp ------------------------------------------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "platform.h"
 
 // Skip this compilation unit if compiled as part of Bionic.
 #if !SCUDO_ANDROID || !_BIONIC
 
 #include "allocator_config.h"
+#include "scudo/interface.h"
 #include "wrappers_c.h"
 #include "wrappers_c_checks.h"
 
 #include <stdint.h>
 #include <stdio.h>
 
 #define SCUDO_PREFIX(name) name
 #define SCUDO_ALLOCATOR Allocator

compiler-rt/lib/scudo/standalone/wrappers_c.inc

INTERFACE WEAK void *SCUDO_PREFIX(calloc)(size_t nmemb, size_t size) {
   scudo::uptr Product;
   if (UNLIKELY(scudo::checkForCallocOverflow(size, nmemb, &Product))) {
     if (SCUDO_ALLOCATOR.canReturnNull()) {
       errno = ENOMEM;
       return nullptr;
     }
     scudo::reportCallocOverflow(nmemb, size);
   }
-  return scudo::setErrnoOnNull(SCUDO_ALLOCATOR.allocate(
-      Product, scudo::Chunk::Origin::Malloc, SCUDO_MALLOC_ALIGNMENT, true));
+  void *Ptr = SCUDO_ALLOCATOR.allocate(
+      Product, scudo::Chunk::Origin::Malloc, SCUDO_MALLOC_ALIGNMENT, true);
+  if (__scudo_allocate_hook && Ptr)
+    __scudo_allocate_hook(Ptr, size);
+
+  return scudo::setErrnoOnNull(Ptr);
 }
 
 INTERFACE WEAK void SCUDO_PREFIX(free)(void *ptr) {
+  if (__scudo_deallocate_hook)
+    __scudo_deallocate_hook(ptr);
   SCUDO_ALLOCATOR.deallocate(ptr, scudo::Chunk::Origin::Malloc);
 }
 
 INTERFACE WEAK struct SCUDO_MALLINFO SCUDO_PREFIX(mallinfo)(void) {
   struct SCUDO_MALLINFO Info = {};
   scudo::StatCounters Stats;
   SCUDO_ALLOCATOR.getStats(Stats);
   // Space allocated in mmapped regions (bytes)
INTERFACE WEAK struct __scudo_mallinfo2 SCUDO_PREFIX(mallinfo2)(void) {
   Info.uordblks = Stats[scudo::StatAllocated];
   // Total free space (bytes)
   Info.fordblks = Info.fsmblks;
   return Info;
 }
 #endif
 
 INTERFACE WEAK void *SCUDO_PREFIX(malloc)(size_t size) {
-  return scudo::setErrnoOnNull(SCUDO_ALLOCATOR.allocate(
-      size, scudo::Chunk::Origin::Malloc, SCUDO_MALLOC_ALIGNMENT));
+  void *Ptr = SCUDO_ALLOCATOR.allocate(size, scudo::Chunk::Origin::Malloc,
+                                       SCUDO_MALLOC_ALIGNMENT);
+  if (__scudo_allocate_hook && Ptr)
+    __scudo_allocate_hook(Ptr, size);
+  return scudo::setErrnoOnNull(Ptr);
 }
 
 #if SCUDO_ANDROID
 INTERFACE WEAK size_t SCUDO_PREFIX(malloc_usable_size)(const void *ptr) {
 #else
 INTERFACE WEAK size_t SCUDO_PREFIX(malloc_usable_size)(void *ptr) {
 #endif
   return SCUDO_ALLOCATOR.getUsableSize(ptr);
if (SCUDO_ANDROID) {
     if (UNLIKELY(!scudo::isPowerOfTwo(alignment))) {
       if (SCUDO_ALLOCATOR.canReturnNull()) {
         errno = EINVAL;
         return nullptr;
       }
       scudo::reportAlignmentNotPowerOfTwo(alignment);
     }
   }
-  return SCUDO_ALLOCATOR.allocate(size, scudo::Chunk::Origin::Memalign,
+  void * Ptr = SCUDO_ALLOCATOR.allocate(size, scudo::Chunk::Origin::Memalign,
                                         alignment);
+  if (__scudo_allocate_hook && Ptr)
+    __scudo_allocate_hook(Ptr, size);
+
+  return Ptr;
 }
 
 INTERFACE WEAK int SCUDO_PREFIX(posix_memalign)(void **memptr, size_t alignment,
                                                 size_t size) {
   if (UNLIKELY(scudo::checkPosixMemalignAlignment(alignment))) {
     if (!SCUDO_ALLOCATOR.canReturnNull())
       scudo::reportInvalidPosixMemalignAlignment(alignment);
     return EINVAL;
   }
   void *Ptr =
       SCUDO_ALLOCATOR.allocate(size, scudo::Chunk::Origin::Memalign, alignment);
+  if (__scudo_allocate_hook && Ptr)
+    __scudo_allocate_hook(Ptr, size);
   if (UNLIKELY(!Ptr))
     return ENOMEM;
+
   *memptr = Ptr;
   return 0;
 }
 
 INTERFACE WEAK void *SCUDO_PREFIX(pvalloc)(size_t size) {
   const scudo::uptr PageSize = scudo::getPageSizeCached();
   if (UNLIKELY(scudo::checkForPvallocOverflow(size, PageSize))) {
     if (SCUDO_ALLOCATOR.canReturnNull()) {
       errno = ENOMEM;
       return nullptr;
     }
     scudo::reportPvallocOverflow(size);
   }
   // pvalloc(0) should allocate one page.
-  return scudo::setErrnoOnNull(
-      SCUDO_ALLOCATOR.allocate(size ? scudo::roundUp(size, PageSize) : PageSize,
-                               scudo::Chunk::Origin::Memalign, PageSize));
+  void *Ptr = SCUDO_ALLOCATOR.allocate(size ? scudo::roundUp(size, PageSize) : PageSize,
+                                       scudo::Chunk::Origin::Memalign, PageSize);
+  if (__scudo_allocate_hook && Ptr)
+    __scudo_allocate_hook(Ptr, size);
+
+  return scudo::setErrnoOnNull(Ptr);
 }
 
 INTERFACE WEAK void *SCUDO_PREFIX(realloc)(void *ptr, size_t size) {
-  if (!ptr)
-    return scudo::setErrnoOnNull(SCUDO_ALLOCATOR.allocate(
-        size, scudo::Chunk::Origin::Malloc, SCUDO_MALLOC_ALIGNMENT));
+  if (!ptr) {
+    void *Ptr = SCUDO_ALLOCATOR.allocate(size, scudo::Chunk::Origin::Malloc,
+                                         SCUDO_MALLOC_ALIGNMENT);
+    if (__scudo_allocate_hook && Ptr)
+      __scudo_allocate_hook(Ptr, size);
+    return scudo::setErrnoOnNull(Ptr);
+  }
   if (size == 0) {
+    if (__scudo_deallocate_hook)
+      __scudo_deallocate_hook(ptr);
     SCUDO_ALLOCATOR.deallocate(ptr, scudo::Chunk::Origin::Malloc);
     return nullptr;
   }
-  return scudo::setErrnoOnNull(
-      SCUDO_ALLOCATOR.reallocate(ptr, size, SCUDO_MALLOC_ALIGNMENT));
+
+  void *NewPtr = SCUDO_ALLOCATOR.reallocate(ptr, size, SCUDO_MALLOC_ALIGNMENT);
+  if (__scudo_deallocate_hook && NewPtr != ptr)
+    __scudo_deallocate_hook(ptr);
+  if (__scudo_allocate_hook && NewPtr != ptr)
+    __scudo_allocate_hook(NewPtr, size);
+
+  return scudo::setErrnoOnNull(NewPtr);
 }
 
 INTERFACE WEAK void *SCUDO_PREFIX(valloc)(size_t size) {
-  return scudo::setErrnoOnNull(SCUDO_ALLOCATOR.allocate(
-      size, scudo::Chunk::Origin::Memalign, scudo::getPageSizeCached()));
+  void *Ptr = SCUDO_ALLOCATOR.allocate(size, scudo::Chunk::Origin::Memalign,
+                                       scudo::getPageSizeCached());
+  if (__scudo_allocate_hook && Ptr)
+    __scudo_allocate_hook(Ptr, size);
+
+  return scudo::setErrnoOnNull(Ptr);
 }
 
 INTERFACE WEAK int SCUDO_PREFIX(malloc_iterate)(
     uintptr_t base, size_t size,
     void (*callback)(uintptr_t base, size_t size, void *arg), void *arg) {
   SCUDO_ALLOCATOR.iterateOverChunks(base, size, callback, arg);
   return 0;
 }
INTERFACE WEAK void *SCUDO_PREFIX(aligned_alloc)(size_t alignment,
                                                  size_t size) {
   if (UNLIKELY(scudo::checkAlignedAllocAlignmentAndSize(alignment, size))) {
     if (SCUDO_ALLOCATOR.canReturnNull()) {
       errno = EINVAL;
       return nullptr;
     }
     scudo::reportInvalidAlignedAllocAlignment(alignment, size);
   }
-  return scudo::setErrnoOnNull(
-      SCUDO_ALLOCATOR.allocate(size, scudo::Chunk::Origin::Malloc, alignment));
+
+  void *Ptr = SCUDO_ALLOCATOR.allocate(size, scudo::Chunk::Origin::Malloc,
+                                       alignment);
+  if (__scudo_allocate_hook && Ptr)
+    __scudo_allocate_hook(Ptr, size);
+
+  return scudo::setErrnoOnNull(Ptr);
 }
 
 INTERFACE WEAK int SCUDO_PREFIX(malloc_info)(UNUSED int options, FILE *stream) {
   const scudo::uptr max_size =
       decltype(SCUDO_ALLOCATOR)::PrimaryT::SizeClassMap::MaxSize;
   auto *sizes = static_cast<scudo::uptr *>(
       SCUDO_PREFIX(calloc)(max_size, sizeof(scudo::uptr)));
   auto callback = [](uintptr_t, size_t size, void *arg) {

compiler-rt/lib/scudo/standalone/wrappers_c_bionic.cpp

 //===-- wrappers_c_bionic.cpp -----------------------------------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "platform.h"
 
 // This is only used when compiled as part of Bionic.
 #if SCUDO_ANDROID && _BIONIC
 
 #include "allocator_config.h"
+#include "scudo/interface.h"
 #include "wrappers_c.h"
 #include "wrappers_c_checks.h"
 
 #include <stdint.h>
 #include <stdio.h>
 
 // Regular MallocDispatch definitions.
 #define SCUDO_PREFIX(name) CONCATENATE(scudo_, name)