[TySan] Intercept malloc_size on Apple platforms. #122133

fhahn · 2025-01-08T15:54:08Z

After #120563 malloc_size also needs intercepting on Apple platforms, otherwise all type-sanitized binaries crash on startup with an objc error:
realized class 0x12345 has corrupt data pointer: malloc_size(0x567) = 0

llvmbot · 2025-01-08T15:54:49Z

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

Author: Florian Hahn (fhahn)

Changes

After #120563 malloc_size also needs intercepting on Apple platforms, otherwise all type-sanitized binaries crash on startup with an objc error:
realized class 0x12345 has corrupt data pointer: malloc_size(0x567) = 0

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

2 Files Affected:

(modified) compiler-rt/lib/sanitizer_common/sanitizer_allocator_dlsym.h (+8-4)
(modified) compiler-rt/lib/tysan/tysan_interceptors.cpp (+8)

diff --git a/compiler-rt/lib/sanitizer_common/sanitizer_allocator_dlsym.h b/compiler-rt/lib/sanitizer_common/sanitizer_allocator_dlsym.h
index b360478a058a54..5465258e6a022d 100644
--- a/compiler-rt/lib/sanitizer_common/sanitizer_allocator_dlsym.h
+++ b/compiler-rt/lib/sanitizer_common/sanitizer_allocator_dlsym.h
@@ -37,7 +37,7 @@ struct DlSymAllocator {
     void *ptr = InternalAlloc(size_in_bytes, nullptr, align);
     CHECK(internal_allocator()->FromPrimary(ptr));
     Details::OnAllocate(ptr,
-                        internal_allocator()->GetActuallyAllocatedSize(ptr));
+                        Size(ptr));
     return ptr;
   }
 
@@ -45,12 +45,12 @@ struct DlSymAllocator {
     void *ptr = InternalCalloc(nmemb, size);
     CHECK(internal_allocator()->FromPrimary(ptr));
     Details::OnAllocate(ptr,
-                        internal_allocator()->GetActuallyAllocatedSize(ptr));
+                        Size(ptr));
     return ptr;
   }
 
   static void Free(void *ptr) {
-    uptr size = internal_allocator()->GetActuallyAllocatedSize(ptr);
+    uptr size = Size(ptr);
     Details::OnFree(ptr, size);
     InternalFree(ptr);
   }
@@ -63,7 +63,7 @@ struct DlSymAllocator {
       Free(ptr);
       return nullptr;
     }
-    uptr size = internal_allocator()->GetActuallyAllocatedSize(ptr);
+    uptr size = Size(ptr);
     uptr memcpy_size = Min(new_size, size);
     void *new_ptr = Allocate(new_size);
     if (new_ptr)
@@ -77,6 +77,10 @@ struct DlSymAllocator {
     return Realloc(ptr, count * size);
   }
 
+  static uptr Size(void *ptr) {
+    return internal_allocator()->GetActuallyAllocatedSize(ptr);
+  }
+
   static void OnAllocate(const void *ptr, uptr size) {}
   static void OnFree(const void *ptr, uptr size) {}
 };
diff --git a/compiler-rt/lib/tysan/tysan_interceptors.cpp b/compiler-rt/lib/tysan/tysan_interceptors.cpp
index 08b1010a48ecf0..4a89f0746230fe 100644
--- a/compiler-rt/lib/tysan/tysan_interceptors.cpp
+++ b/compiler-rt/lib/tysan/tysan_interceptors.cpp
@@ -108,6 +108,14 @@ INTERCEPTOR(void *, malloc, uptr size) {
   return res;
 }
 
+#if SANITIZER_APPLE
+INTERCEPTOR(uptr , malloc_size, void *ptr) {
+  if (DlsymAlloc::Use())
+    return DlsymAlloc::Size(ptr);
+  return REAL(malloc_size)(ptr);
+}
+#endif
+
 INTERCEPTOR(void *, realloc, void *ptr, uptr size) {
   if (DlsymAlloc::Use() || DlsymAlloc::PointerIsMine(ptr))
     return DlsymAlloc::Realloc(ptr, size);

github-actions · 2025-01-08T15:57:34Z

✅ With the latest revision this PR passed the C/C++ code formatter.

fmayer · 2025-01-08T16:01:03Z

compiler-rt/lib/tysan/tysan_interceptors.cpp

@@ -108,6 +108,14 @@ INTERCEPTOR(void *, malloc, uptr size) {
  return res;
 }

+#if SANITIZER_APPLE


drive by question: how do we decide between TYSAN_INTERCEPT___STRDUP and SANITIZER_APPLE style conditions?

fmayer · 2025-01-08T16:04:13Z

compiler-rt/lib/tysan/tysan_interceptors.cpp

@@ -108,6 +108,14 @@ INTERCEPTOR(void *, malloc, uptr size) {
  return res;
 }

+#if SANITIZER_APPLE
+INTERCEPTOR(uptr, malloc_size, void *ptr) {
+  if (DlsymAlloc::Use())


Why don't we need DlSymAlloc::PointerIsMine here but we do in realloc? (undo approval)

delcypher

Glad you figured out the problem. I think the implementation of malloc_size interceptor needs to be a little different to avoid a potential bug.

delcypher · 2025-01-08T16:06:43Z

compiler-rt/lib/tysan/tysan_interceptors.cpp

@@ -108,6 +108,14 @@ INTERCEPTOR(void *, malloc, uptr size) {
  return res;
 }

+#if SANITIZER_APPLE
+INTERCEPTOR(uptr, malloc_size, void *ptr) {
+  if (DlsymAlloc::Use())


A less fragile implementation here would be ask the DlsymAlloc allocator if it owns this allocation and if so request the size from it, otherwise call the system malloc_size. That way the malloc_size interceptor always reports the right things no matter which allocator actually allocated the memory.

The current implementation means that if malloc_size is called after TYSan init on an allocation that was allocated during TySan init then we'll call the system malloc_size which will fail because the system allocator didn't allocator the memory.

Thanks, I think that should be taken care of by checking PointerIsMine (as mentioned by @fmayer ). Update the code.

fmayer · 2025-01-08T16:55:56Z

compiler-rt/lib/tysan/tysan_interceptors.cpp

@@ -110,7 +110,7 @@ INTERCEPTOR(void *, malloc, uptr size) {

 #if SANITIZER_APPLE
 INTERCEPTOR(uptr, malloc_size, void *ptr) {
-  if (DlsymAlloc::Use())
+  if (DlsymAlloc::Use() || DlsymAlloc::PointerIsMine(ptr))


Same question as for realloc: please document the state transitions we support.

Specifically: could it be that DlSymAlloc::Use is true but NOT DlsymAlloc::PointerIsMine(ptr) (e.g. because the pointer was created before DlSymAlloc was enabled? That would be incorrect then I think.

For realloc, during TySan init realloc(0, size) can make DlSymAlloc::Use is true but DlsymAlloc::PointerIsMine(ptr) is false.

So do we need DlsymAlloc::Use() here? To me DlsymAlloc::PointerIsMine(ptr) sounds like what we actually care about?

Yep that should do the trick, done ,thanks

Enna1

Thanks for the fix!

On Apple platforms, malloc_size also needs intercepting with DlSymAllocator, otherwise all type-sanitized binaries crash on startup with an objc error: realized class 0x12345 has corrupt data pointer: malloc_size(0x567) = 0

delcypher

I have a minor nit. Other than that LGTM.

delcypher · 2025-01-09T12:42:11Z

compiler-rt/lib/sanitizer_common/sanitizer_allocator_dlsym.h

@@ -77,6 +75,10 @@ struct DlSymAllocator {
    return Realloc(ptr, count * size);
  }

+  static uptr Size(void *ptr) {


Nit: I'd call this GetSize() rather than Size() because other methods have verbs in their name (e.g. Allocate) but Size doesn't contain a verb but GetSize() does.

It's not really that important so feel free to ignore this suggestion.

Updated, thanks!

seanm · 2025-01-09T18:01:15Z

I've download this patch locally, and can confirm it fixes my issue. Now 94% of cmake's unit tests pass, which is the kind of fraction one might expect using a new stress testing tool. Now to actually take a look at some...

seanm · 2025-01-09T18:14:18Z

I've download this patch locally, and can confirm it fixes my issue. Now 94% of cmake's unit tests pass, which is the kind of fraction one might expect using a new stress testing tool. Now to actually take a look at some...

Actually, looks like most (all?) are some weird linking issue:

dyld[82529]: Symbol not found: __ZNSt13exception_ptr31__from_native_exception_pointerEPv
  Referenced from: <9FF30C7F-ED6B-3C65-AE51-8D211D47EBA4> /Users/sean/external/cmake-bin/Tests/CMakeLib/CMakeLibTests
  Expected in:     <C4E10D69-6F13-32B1-831D-82DA32D7DB88> /usr/lib/libc++.1.dylib

fhahn · 2025-01-09T18:30:32Z

I've download this patch locally, and can confirm it fixes my issue. Now 94% of cmake's unit tests pass, which is the kind of fraction one might expect using a new stress testing tool. Now to actually take a look at some...

Actually, looks like most (all?) are some weird linking issue:
dyld[82529]: Symbol not found: __ZNSt13exception_ptr31__from_native_exception_pointerEPv
  Referenced from: <9FF30C7F-ED6B-3C65-AE51-8D211D47EBA4> /Users/sean/external/cmake-bin/Tests/CMakeLib/CMakeLibTests
  Expected in:     <C4E10D69-6F13-32B1-831D-82DA32D7DB88> /usr/lib/libc++.1.dylib

@seanm Thanks for checking! Not sure what is going on here exactly, I've not seen this locally myself. Could you file an issue with steps to reproduce?

seanm · 2025-01-09T18:48:12Z

@seanm Thanks for checking! Not sure what is going on here exactly, I've not seen this locally myself. Could you file an issue with steps to reproduce?

OK I'll wait until this is merged so as to be using a more proper build of clang.

After llvm/llvm-project#120563 malloc_size also needs intercepting on Apple platforms, otherwise all type-sanitized binaries crash on startup with an objc error: realized class 0x12345 has corrupt data pointer: malloc_size(0x567) = 0 PR: llvm/llvm-project#122133

After llvm#120563 malloc_size also needs intercepting on Apple platforms, otherwise all type-sanitized binaries crash on startup with an objc error: realized class 0x12345 has corrupt data pointer: malloc_size(0x567) = 0 PR: llvm#122133

fhahn requested review from MaskRay, delcypher, kubamracek, vitalybuka and Enna1 January 8, 2025 15:54

llvmbot added compiler-rt compiler-rt:sanitizer labels Jan 8, 2025

fhahn mentioned this pull request Jan 8, 2025

Trying TySan: assert trying to build cmake: "Cannot get layout of forward declarations!" #121014

Closed

fhahn mentioned this pull request Jan 8, 2025

[tysan] Replace HandleEarlyAlloc with DlsymAlloc #120563

Merged

fmayer reviewed Jan 8, 2025

View reviewed changes

fmayer approved these changes Jan 8, 2025

View reviewed changes

fmayer reviewed Jan 8, 2025

View reviewed changes

delcypher requested changes Jan 8, 2025

View reviewed changes

fmayer reviewed Jan 8, 2025

View reviewed changes

vitalybuka approved these changes Jan 8, 2025

View reviewed changes

Enna1 approved these changes Jan 9, 2025

View reviewed changes

fhahn added 4 commits January 9, 2025 11:45

[TySan] Intercept malloc_size on Apple platforms.

6eb5550

On Apple platforms, malloc_size also needs intercepting with DlSymAllocator, otherwise all type-sanitized binaries crash on startup with an objc error: realized class 0x12345 has corrupt data pointer: malloc_size(0x567) = 0

!fixup fix formatting

d130709

!fixup also check PointerIsMine

17e8291

!fixup only use PointerIsMine

5688643

fhahn force-pushed the tysan-intercept-malloc-size branch from 8c8f9d9 to 5688643 Compare January 9, 2025 11:54

delcypher approved these changes Jan 9, 2025

View reviewed changes

!fixup Size -> GetSize()

3240cb2

fhahn merged commit e8c8543 into llvm:main Jan 9, 2025
5 of 6 checks passed

fhahn deleted the tysan-intercept-malloc-size branch January 9, 2025 20:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[TySan] Intercept malloc_size on Apple platforms. #122133

[TySan] Intercept malloc_size on Apple platforms. #122133

fhahn commented Jan 8, 2025

llvmbot commented Jan 8, 2025

github-actions bot commented Jan 8, 2025 •

edited

Loading

fmayer Jan 8, 2025

fmayer Jan 8, 2025 •

edited

Loading

delcypher left a comment

delcypher Jan 8, 2025 •

edited

Loading

fhahn Jan 8, 2025

fmayer Jan 8, 2025

Enna1 Jan 9, 2025

fmayer Jan 9, 2025

fhahn Jan 9, 2025

Enna1 left a comment

delcypher left a comment

delcypher Jan 9, 2025 •

edited

Loading

fhahn Jan 9, 2025

seanm commented Jan 9, 2025

seanm commented Jan 9, 2025

fhahn commented Jan 9, 2025

seanm commented Jan 9, 2025

[TySan] Intercept malloc_size on Apple platforms. #122133

[TySan] Intercept malloc_size on Apple platforms. #122133

Conversation

fhahn commented Jan 8, 2025

llvmbot commented Jan 8, 2025

github-actions bot commented Jan 8, 2025 • edited Loading

fmayer Jan 8, 2025

Choose a reason for hiding this comment

fmayer Jan 8, 2025 • edited Loading

Choose a reason for hiding this comment

delcypher left a comment

Choose a reason for hiding this comment

delcypher Jan 8, 2025 • edited Loading

Choose a reason for hiding this comment

fhahn Jan 8, 2025

Choose a reason for hiding this comment

fmayer Jan 8, 2025

Choose a reason for hiding this comment

Enna1 Jan 9, 2025

Choose a reason for hiding this comment

fmayer Jan 9, 2025

Choose a reason for hiding this comment

fhahn Jan 9, 2025

Choose a reason for hiding this comment

Enna1 left a comment

Choose a reason for hiding this comment

delcypher left a comment

Choose a reason for hiding this comment

delcypher Jan 9, 2025 • edited Loading

Choose a reason for hiding this comment

fhahn Jan 9, 2025

Choose a reason for hiding this comment

seanm commented Jan 9, 2025

seanm commented Jan 9, 2025

fhahn commented Jan 9, 2025

seanm commented Jan 9, 2025

github-actions bot commented Jan 8, 2025 •

edited

Loading

fmayer Jan 8, 2025 •

edited

Loading

delcypher Jan 8, 2025 •

edited

Loading

delcypher Jan 9, 2025 •

edited

Loading