[scudo] Add hooks to mark the range of realloc #74353

ChiaHungDuan · 2023-12-04T17:55:55Z

realloc may involve both allocation and deallocation. Given that the reporting the events is not atomic and which may lead the hook user to a false case that the double-use pattern happens. In general, this can be resolved on the hook side. To alleviate the task of handling it, we add two new hooks to mark the range so that the hook user can combine those calls together.

llvmbot · 2023-12-04T17:56:26Z

@llvm/pr-subscribers-compiler-rt-sanitizer

Author: None (ChiaHungDuan)

Changes

realloc may involve both allocation and deallocation. Given that the reporting the events is not atomic and which may lead the hook user to a false case that the double-use pattern happens. In general, this can be resolved on the hook side. To alleviate the task of handling it, we add two new hooks to mark the range and reorder the reporting.

Full diff: https://github.com/llvm/llvm-project/pull/74353.diff

3 Files Affected:

(modified) compiler-rt/lib/scudo/standalone/include/scudo/interface.h (+7)
(modified) compiler-rt/lib/scudo/standalone/tests/wrappers_c_test.cpp (+22)
(modified) compiler-rt/lib/scudo/standalone/wrappers_c.inc (+39-19)

diff --git a/compiler-rt/lib/scudo/standalone/include/scudo/interface.h b/compiler-rt/lib/scudo/standalone/include/scudo/interface.h
index 260f1a7bd84bb..9ac900b96533e 100644
--- a/compiler-rt/lib/scudo/standalone/include/scudo/interface.h
+++ b/compiler-rt/lib/scudo/standalone/include/scudo/interface.h
@@ -20,6 +20,13 @@ __attribute__((weak)) const char *__scudo_default_options(void);
 __attribute__((weak)) void __scudo_allocate_hook(void *ptr, size_t size);
 __attribute__((weak)) void __scudo_deallocate_hook(void *ptr);
 
+// These hooks are used to mark the scope of doing realloc(). Note that the
+// allocation/deallocation are still reported through the hooks above, this is
+// only used when a hook user wants to know that the allocation/deallocation
+// operations are a single realloc operation.
+__attribute__((weak)) void __scudo_realloc_begin_hook(void *old_ptr);
+__attribute__((weak)) void __scudo_realloc_end_hook(void *old_ptr);
+
 void __scudo_print_stats(void);
 
 typedef void (*iterate_callback)(uintptr_t base, size_t size, void *arg);
diff --git a/compiler-rt/lib/scudo/standalone/tests/wrappers_c_test.cpp b/compiler-rt/lib/scudo/standalone/tests/wrappers_c_test.cpp
index 150688b5b70a5..c6ad40c8cc2a3 100644
--- a/compiler-rt/lib/scudo/standalone/tests/wrappers_c_test.cpp
+++ b/compiler-rt/lib/scudo/standalone/tests/wrappers_c_test.cpp
@@ -61,8 +61,13 @@ struct AllocContext {
 struct DeallocContext {
   void *Ptr;
 };
+struct ReallocContext {
+  void *Begin;
+  void *End;
+};
 static AllocContext AC;
 static DeallocContext DC;
+static ReallocContext RC;
 
 #if (SCUDO_ENABLE_HOOKS_TESTS == 1)
 __attribute__((visibility("default"))) void __scudo_allocate_hook(void *Ptr,
@@ -73,6 +78,14 @@ __attribute__((visibility("default"))) void __scudo_allocate_hook(void *Ptr,
 __attribute__((visibility("default"))) void __scudo_deallocate_hook(void *Ptr) {
   DC.Ptr = Ptr;
 }
+__attribute__((visibility("default"))) void
+__scudo_realloc_begin_hook(void *Ptr) {
+  RC.Begin = Ptr;
+}
+__attribute__((visibility("default"))) void
+__scudo_realloc_end_hook(void *Ptr) {
+  RC.End = Ptr;
+}
 #endif // (SCUDO_ENABLE_HOOKS_TESTS == 1)
 }
 
@@ -88,6 +101,7 @@ class ScudoWrappersCTest : public Test {
       void *InvalidPtr = reinterpret_cast<void *>(0xdeadbeef);
       AC.Ptr = InvalidPtr;
       DC.Ptr = InvalidPtr;
+      RC.Begin = RC.End = InvalidPtr;
     }
   }
   void verifyAllocHookPtr(UNUSED void *Ptr) {
@@ -102,6 +116,12 @@ class ScudoWrappersCTest : public Test {
     if (SCUDO_ENABLE_HOOKS_TESTS)
       EXPECT_EQ(Ptr, DC.Ptr);
   }
+  void verifyReallocHooksScope(UNUSED void *Ptr) {
+    if (SCUDO_ENABLE_HOOKS_TESTS) {
+      EXPECT_EQ(Ptr, RC.Begin);
+      EXPECT_EQ(Ptr, RC.End);
+    }
+  }
 };
 using ScudoWrappersCDeathTest = ScudoWrappersCTest;
 
@@ -297,6 +317,7 @@ TEST_F(ScudoWrappersCDeathTest, Realloc) {
     verifyAllocHookSize(Size * 2U);
     verifyDeallocHookPtr(OldP);
   }
+  verifyReallocHooksScope(OldP);
 
   invalidateHookPtrs();
   OldP = P;
@@ -312,6 +333,7 @@ TEST_F(ScudoWrappersCDeathTest, Realloc) {
     verifyAllocHookPtr(P);
     verifyAllocHookSize(Size / 2U);
   }
+  verifyReallocHooksScope(OldP);
   free(P);
 
   EXPECT_DEATH(P = realloc(P, Size), "");
diff --git a/compiler-rt/lib/scudo/standalone/wrappers_c.inc b/compiler-rt/lib/scudo/standalone/wrappers_c.inc
index 0413ea49eac08..c6d0fb1132aa9 100644
--- a/compiler-rt/lib/scudo/standalone/wrappers_c.inc
+++ b/compiler-rt/lib/scudo/standalone/wrappers_c.inc
@@ -27,6 +27,40 @@ static void reportDeallocation(void *ptr) {
     if (__scudo_deallocate_hook)
       __scudo_deallocate_hook(ptr);
 }
+static void reportReallocBegin(void *old_ptr) {
+  if (SCUDO_ENABLE_HOOKS)
+    if (__scudo_realloc_begin_hook)
+      __scudo_realloc_begin_hook(old_ptr);
+}
+static void reportReallocEnd(void *old_ptr) {
+  if (SCUDO_ENABLE_HOOKS)
+    if (__scudo_realloc_end_hook)
+      __scudo_realloc_end_hook(old_ptr);
+}
+
+static void *reallocImpl(void *ptr, size_t size) {
+  // Given that the reporting of deallocation and allocation are not atomic, we
+  // always pretend the old pointer will be released so that the user doesn't
+  // need to worry about the false double-use case from the view of hooks.
+  //
+  // For example, assume that `realloc` releases the old pointer and allocates a
+  // new pointer. Before the reporting of both operations has been done, another
+  // thread may get the old pointer from `malloc`. It may be misinterpreted as
+  // double-use if it's not handled properly on the hook side.
+  reportDeallocation(ptr);
+  void *NewPtr = SCUDO_ALLOCATOR.reallocate(ptr, size, SCUDO_MALLOC_ALIGNMENT);
+  if (NewPtr != nullptr) {
+    // Note that even if NewPtr == ptr, the size has changed. We still need to
+    // report the new size.
+    reportAllocation(NewPtr, size);
+  } else {
+    // If `realloc` fails, the old pointer is not released. Report the old
+    // pointer as allocated back.
+    reportAllocation(ptr, SCUDO_ALLOCATOR.getAllocSize(ptr));
+  }
+
+  return NewPtr;
+}
 
 extern "C" {
 
@@ -175,25 +209,11 @@ INTERFACE WEAK void *SCUDO_PREFIX(realloc)(void *ptr, size_t size) {
     return nullptr;
   }
 
-  // Given that the reporting of deallocation and allocation are not atomic, we
-  // always pretend the old pointer will be released so that the user doesn't
-  // need to worry about the false double-use case from the view of hooks.
-  //
-  // For example, assume that `realloc` releases the old pointer and allocates a
-  // new pointer. Before the reporting of both operations has been done, another
-  // thread may get the old pointer from `malloc`. It may be misinterpreted as
-  // double-use if it's not handled properly on the hook side.
-  reportDeallocation(ptr);
-  void *NewPtr = SCUDO_ALLOCATOR.reallocate(ptr, size, SCUDO_MALLOC_ALIGNMENT);
-  if (NewPtr != nullptr) {
-    // Note that even if NewPtr == ptr, the size has changed. We still need to
-    // report the new size.
-    reportAllocation(NewPtr, size);
-  } else {
-    // If `realloc` fails, the old pointer is not released. Report the old
-    // pointer as allocated back.
-    reportAllocation(ptr, SCUDO_ALLOCATOR.getAllocSize(ptr));
-  }
+  reportReallocBegin(ptr);
+
+  void *NewPtr = reallocImpl(ptr, size);
+
+  reportReallocEnd(ptr);
 
   return scudo::setErrnoOnNull(NewPtr);
 }

`realloc` may involve both allocation and deallocation. Given that the reporting the events is not atomic and which may lead the hook user to a false case that the double-use pattern happens. In general, this can be resolved on the hook side. To alleviate the task of handling it, we add two new hooks to mark the range so that the hook user can combine those calls together.

cferris1000 · 2023-12-04T23:15:15Z

Fabio, since you are the person who is most interested in this change, can you indicate how this solves your problem? For example, it's not clear how a snapshot from a profiler potentially being slightly wrong if it gets caught between the deallocate hook and the allocate hook in the realloc code is a problem. I'm not familiar with the profilers and how they are used by your team, so this could be something that occurs frequently. If you are using a sampler profiler, there is always the possibility of a snapshot being incorrect since you could get caught before the hook is called but after the operation has completed depending on how the profiler is coded.

I'm not opposed to this change, I just want to make sure we are adding code that solves a real problem, and not adding code for a corner case that is not an issue for any current profilers.

cferris1000 · 2023-12-05T00:13:50Z

To add concrete numbers, I looked through the memory traces we have taken on Android devices, and reallocs tend to be about 1%-2% or 3%-4% of the operations. The highest I saw was 6%, which seems to be a particular app. So the chance of hitting this issue is already relatively low, but it's possible that you are seeing a different rate that is higher than on Android.

fabio-d

Thanks @ChiaHungDuan and @cferris1000. This new revision solves my problem of tracking memory precisely even while it's being reallocated.

fabio-d · 2023-12-07T16:42:48Z

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

+  if (SCUDO_ENABLE_HOOKS) {
+    if (__scudo_realloc_allocate_hook)
+      __scudo_realloc_allocate_hook(old_ptr, new_ptr, size);
+    else


I think we want this to be else if (__scudo_allocate_hook && new_ptr)

if new_ptr == nullptr, it'll be handled differently. See the caller for the detail.

The else if (__scudo_allocate_hook) has fixed.

BTW, added a DCHECK for the new_ptr != nullptr

fabio-d · 2023-12-07T16:43:12Z

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

+  if (SCUDO_ENABLE_HOOKS) {
+    if (__scudo_realloc_deallocate_hook)
+      __scudo_realloc_deallocate_hook(old_ptr);
+    else


And similarly else if (__scudo_deallocate_hook)

cferris1000 · 2023-12-07T20:33:08Z

compiler-rt/lib/scudo/standalone/include/scudo/interface.h

+// `realloc` involves both deallocation and allocation but they are not reported
+// atomically. In one specific case which may keep taking a snapshot right in
+// the middle of `realloc` reporting the deallocation and allocation, it may
+// confuse the user by the missing memory from `realloc`. To alleviate that


by the missing -> by missing

cferris1000 · 2023-12-07T20:33:51Z

compiler-rt/lib/scudo/standalone/include/scudo/interface.h

+// confuse the user by the missing memory from `realloc`. To alleviate that
+// case, define the two `realloc` hooks to get the knowledge of the bundled hook
+// calls. This hooks are optional and only used when you are pretty likely to
+// hit that specific case. Otherwise, the two general hooks above are pretty


Remove the word pretty, it make the sentence flow better.

cferris1000 · 2023-12-07T20:34:05Z

compiler-rt/lib/scudo/standalone/include/scudo/interface.h

+// the middle of `realloc` reporting the deallocation and allocation, it may
+// confuse the user by the missing memory from `realloc`. To alleviate that
+// case, define the two `realloc` hooks to get the knowledge of the bundled hook
+// calls. This hooks are optional and only used when you are pretty likely to


This -> These

and only used when you are pretty likely to -> and should only be used when a hooks user wants to track reallocs more closely.

or something similar.

Done. Also removed the last sentence.

cferris1000

One small typo.

cferris1000 · 2023-12-07T21:37:17Z

compiler-rt/lib/scudo/standalone/tests/wrappers_c_test.cpp

+  // Note that this is only used for testing. In general, only one pair of hooks
+  // will be invoked in `realloc`. if __scudo_realloc_*_hook are not defined,
+  // it'll call the general hooks only. To make the test easier, we call the
+  // general one here so that either case (wether __scudo_realloc_*_hook are


wether -> whether

cferris1000

LGTM.

This CL implements the new Scudo realloc hooks (see llvm/llvm-project#74353) by delaying any conflicting operation until the outcome of the realloc() call is known, i.e. until __scudo_realloc_allocate_hook is called. Bug: b/313882413, b/315316408 Change-Id: I991386e52b6be288ebffc78be5d9f6acfaf4cc75 Reviewed-on: https://fuchsia-review.googlesource.com/c/fuchsia/+/953633 Commit-Queue: Fabio D'Urso <[email protected]> Reviewed-by: Étienne J. Membrives <[email protected]>

ChiaHungDuan requested a review from cferris1000 December 4, 2023 17:55

llvmbot added compiler-rt compiler-rt:scudo Scudo Hardened Allocator compiler-rt:sanitizer labels Dec 4, 2023

ChiaHungDuan force-pushed the fix-hooks-on-realloc branch from 5fd11cf to dbb117b Compare December 4, 2023 19:32

ChiaHungDuan mentioned this pull request Dec 4, 2023

[scudo] Add hooks to mark the range of realloc #73883

Closed

fixup! Inline reallocImpl

ee27083

ChiaHungDuan requested a review from fabio-d December 4, 2023 22:13

fixup! Change the way of using realloc hooks

a4a18e2

fabio-d approved these changes Dec 7, 2023

View reviewed changes

fixup! Check if the general hooks are defined in realloc path

cc66d75

cferris1000 reviewed Dec 7, 2023

View reviewed changes

fixup! Address review comments

0aa10e2

cferris1000 reviewed Dec 7, 2023

View reviewed changes

fixup! Fix typo

f390dc2

cferris1000 approved these changes Dec 7, 2023

View reviewed changes

ChiaHungDuan merged commit 58c2a4e into llvm:main Dec 7, 2023

[scudo] Add hooks to mark the range of realloc #74353

[scudo] Add hooks to mark the range of realloc #74353

Uh oh!

Conversation

ChiaHungDuan commented Dec 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Dec 4, 2023

Uh oh!

cferris1000 commented Dec 4, 2023

Uh oh!

cferris1000 commented Dec 5, 2023

Uh oh!

fabio-d left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cferris1000 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cferris1000 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ChiaHungDuan commented Dec 4, 2023 •

edited

Loading