[ASan][Windows] Fix false positive for zero sized rtl allocations#181015
[ASan][Windows] Fix false positive for zero sized rtl allocations#181015
Conversation
|
@llvm/pr-subscribers-compiler-rt-sanitizer Author: Zack Johnson (zacklj89) ChangesThis is a follow up to #155943 On Windows, ASan's allocator internally upgrades zero-size allocation requests to size 1 (since malloc(0) must return a unique non-NULL pointer). However, when the user queries the allocation size through Windows heap APIs (RtlSizeHeap, HeapSize, _msize, GlobalSize, LocalSize), ASan reports the internal size (1) instead of the originally requested size (0). This causes false positive heap-buffer-overflow errors in a common pattern: The change adds a The 1-byte user region of a zero-size allocation is still poisoned, so any actual access to it is correctly reported as an overflow Full diff: https://github.com/llvm/llvm-project/pull/181015.diff 2 Files Affected:
diff --git a/compiler-rt/lib/asan/asan_allocator.cpp b/compiler-rt/lib/asan/asan_allocator.cpp
index 05ae3a430cabd..1b314f6879529 100644
--- a/compiler-rt/lib/asan/asan_allocator.cpp
+++ b/compiler-rt/lib/asan/asan_allocator.cpp
@@ -93,6 +93,11 @@ class ChunkHeader {
atomic_uint8_t chunk_state;
u8 alloc_type : 2;
u8 lsan_tag : 2;
+#if SANITIZER_WINDOWS
+ // True if this was a zero-size allocation upgraded to size 1.
+ // Used to report the original size (0) to the user via HeapSize/RtlSizeHeap.
+ u8 from_zero_alloc : 1;
+#endif
// align < 8 -> 0
// else -> log2(min(align, 512)) - 2
@@ -610,6 +615,9 @@ struct Allocator {
uptr chunk_beg = user_beg - kChunkHeaderSize;
AsanChunk *m = reinterpret_cast<AsanChunk *>(chunk_beg);
m->alloc_type = alloc_type;
+#if SANITIZER_WINDOWS
+ m->from_zero_alloc = upgraded_from_zero;
+#endif
CHECK(size);
m->SetUsedSize(size);
m->user_requested_alignment_log = user_requested_alignment_log;
@@ -863,8 +871,22 @@ struct Allocator {
return m->UsedSize();
}
+#if SANITIZER_WINDOWS
+ // Returns true if the allocation at p was a zero-size request that was
+ // internally upgraded to size 1.
+ bool FromZeroAllocation(uptr p) {
+ return reinterpret_cast<AsanChunk *>(p - kChunkHeaderSize)->from_zero_alloc;
+ }
+#endif
+
uptr AllocationSizeFast(uptr p) {
+#if SANITIZER_WINDOWS
+ AsanChunk *c = reinterpret_cast<AsanChunk *>(p - kChunkHeaderSize);
+ if (c->from_zero_alloc) return 0;
+ return c->UsedSize();
+#else
return reinterpret_cast<AsanChunk *>(p - kChunkHeaderSize)->UsedSize();
+#endif
}
AsanChunkView FindHeapChunkByAddress(uptr addr) {
@@ -1125,6 +1147,14 @@ uptr asan_malloc_usable_size(const void *ptr, uptr pc, uptr bp) {
GET_STACK_TRACE_FATAL(pc, bp);
ReportMallocUsableSizeNotOwned((uptr)ptr, &stack);
}
+#if SANITIZER_WINDOWS
+ // Zero-size allocations are internally upgraded to size 1, but we should
+ // report the originally requested size (0) to the user via
+ // HeapSize/RtlSizeHeap.
+ if (usable_size > 0 &&
+ instance.FromZeroAllocation(reinterpret_cast<uptr>(ptr)))
+ return 0;
+#endif
return usable_size;
}
@@ -1222,7 +1252,14 @@ void asan_delete_array_sized_aligned(void *ptr, uptr size, uptr alignment,
}
uptr asan_mz_size(const void *ptr) {
+#if SANITIZER_WINDOWS
+ uptr size = instance.AllocationSize(reinterpret_cast<uptr>(ptr));
+ if (size > 0 && instance.FromZeroAllocation(reinterpret_cast<uptr>(ptr)))
+ return 0;
+ return size;
+#else
return instance.AllocationSize(reinterpret_cast<uptr>(ptr));
+#endif
}
void asan_mz_force_lock() SANITIZER_NO_THREAD_SAFETY_ANALYSIS {
@@ -1366,6 +1403,13 @@ uptr __sanitizer_get_allocated_size(const void *p) {
GET_STACK_TRACE_FATAL_HERE;
ReportSanitizerGetAllocatedSizeNotOwned(ptr, &stack);
}
+#if SANITIZER_WINDOWS
+ // Zero-size allocations are internally upgraded to size 1, but report
+ // the originally requested size (0) to the user via
+ // HeapSize/RtlSizeHeap.
+ if (instance.FromZeroAllocation(ptr))
+ return 0;
+#endif
return allocated_size;
}
diff --git a/compiler-rt/test/asan/TestCases/Windows/rtlsizeheap_zero.cpp b/compiler-rt/test/asan/TestCases/Windows/rtlsizeheap_zero.cpp
new file mode 100644
index 0000000000000..741275c6cac02
--- /dev/null
+++ b/compiler-rt/test/asan/TestCases/Windows/rtlsizeheap_zero.cpp
@@ -0,0 +1,107 @@
+// RUN: %clang_cl_asan %s %Fe%t
+// RUN: %run %t 2>&1 | FileCheck %s
+// RUN: %clang_cl_asan %s %Fe%t /DFAIL_CHECK
+// RUN: not %run %t 2>&1 | FileCheck %s --check-prefix=CHECK-FAIL
+//
+// Verify that zero-size heap allocations report size 0 through Windows heap
+// size APIs, preventing false positives when the result is used with memset.
+
+#include <malloc.h>
+#include <stdio.h>
+#include <windows.h>
+
+using AllocateFunctionPtr = PVOID(__stdcall *)(PVOID, ULONG, SIZE_T);
+using FreeFunctionPtr = BOOL(__stdcall *)(PVOID, ULONG, PVOID);
+using SizeFunctionPtr = SIZE_T(__stdcall *)(PVOID, ULONG, PVOID);
+
+int main() {
+ HMODULE NtDllHandle = GetModuleHandle("ntdll.dll");
+ if (!NtDllHandle) {
+ puts("Couldn't load ntdll");
+ return -1;
+ }
+
+ auto RtlAllocateHeap_ptr =
+ (AllocateFunctionPtr)GetProcAddress(NtDllHandle, "RtlAllocateHeap");
+ auto RtlFreeHeap_ptr =
+ (FreeFunctionPtr)GetProcAddress(NtDllHandle, "RtlFreeHeap");
+ auto RtlSizeHeap_ptr =
+ (SizeFunctionPtr)GetProcAddress(NtDllHandle, "RtlSizeHeap");
+
+ if (!RtlAllocateHeap_ptr || !RtlFreeHeap_ptr || !RtlSizeHeap_ptr) {
+ puts("Couldn't find Rtl heap functions");
+ return -1;
+ }
+
+ // Test RtlAllocateHeap with zero size
+ {
+ char *buffer =
+ (char *)RtlAllocateHeap_ptr(GetProcessHeap(), HEAP_ZERO_MEMORY, 0);
+ if (buffer) {
+ auto size = RtlSizeHeap_ptr(GetProcessHeap(), 0, buffer);
+ memset(buffer, 0, size);
+#ifdef FAIL_CHECK
+ // heap-buffer-overflow since actual size is 0
+ memset(buffer, 0, 1);
+#endif
+ RtlFreeHeap_ptr(GetProcessHeap(), 0, buffer);
+ }
+ }
+
+ // Test malloc with zero size
+ {
+ char *buffer = (char *)malloc(0);
+ if (buffer) {
+ auto size = _msize(buffer);
+ auto rtl_size = RtlSizeHeap_ptr(GetProcessHeap(), 0, buffer);
+ memset(buffer, 0, size);
+ memset(buffer, 0, rtl_size);
+ free(buffer);
+ }
+ }
+
+ // Test operator new with zero size
+ {
+ char *buffer = new char[0];
+ auto size = _msize(buffer);
+ auto rtl_size = RtlSizeHeap_ptr(GetProcessHeap(), 0, buffer);
+ memset(buffer, 0, size);
+ memset(buffer, 0, rtl_size);
+ delete[] buffer;
+ }
+
+ // Test GlobalAlloc with zero size.
+ // GlobalAlloc calls RtlAllocateHeap internally.
+ {
+ HGLOBAL hMem = GlobalAlloc(GMEM_FIXED | GMEM_ZEROINIT, 0);
+ if (hMem) {
+ char *buffer = (char *)hMem;
+ auto size = GlobalSize(hMem);
+ auto rtl_size = RtlSizeHeap_ptr(GetProcessHeap(), 0, buffer);
+ memset(buffer, 0, size);
+ memset(buffer, 0, rtl_size);
+ GlobalFree(hMem);
+ }
+ }
+
+ // Test LocalAlloc with zero size.
+ // LocalAlloc calls RtlAllocateHeap internally.
+ {
+ HLOCAL hMem = LocalAlloc(LMEM_FIXED | LMEM_ZEROINIT, 0);
+ if (hMem) {
+ char *buffer = (char *)hMem;
+ auto size = LocalSize(hMem);
+ auto rtl_size = RtlSizeHeap_ptr(GetProcessHeap(), 0, buffer);
+ memset(buffer, 0, size);
+ memset(buffer, 0, rtl_size);
+ LocalFree(hMem);
+ }
+ }
+
+ puts("Success");
+ return 0;
+}
+
+// CHECK: Success
+// CHECK-NOT: AddressSanitizer: heap-buffer-overflow
+// CHECK-FAIL: AddressSanitizer: heap-buffer-overflow
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
🪟 Windows x64 Test Results
✅ The build succeeded and all tests passed. |
|
Looks like there's a failing test and I may have messed up the cl vs clang-cl logic. I'll fix it and the formatting tomorrow. |
thurstond
left a comment
There was a problem hiding this comment.
Thanks for the patch! General approach looks good from an ASan perspective.
I'm not as familiar with the Windows API so I'll cc your colleague @davidmrdavid
|
I must have my format hooks incorrect, apologies about that! edit: looks like the tests are also still broken and my build scripts are outdated. I think that's the problem with formatting as well. I'll make a push later with the fixes. |
293fb73 to
389fd52
Compare
davidmrdavid
left a comment
There was a problem hiding this comment.
Left some super minor feedback. Will be ready to after addressed. Thanks Zack!
| @@ -0,0 +1,107 @@ | |||
| // RUN: %clang_cl_asan %s %Fe%t | |||
| // RUN: %env_asan_opts=windows_hook_rtl_allocators=true %run %t 2>&1 | FileCheck %s | |||
| // RUN: %clang_cl_asan %s %Fe%t /DFAIL_CHECK | |||
There was a problem hiding this comment.
This change broke running the tests in mingw mode: https://github.com/mstorsjo/llvm-mingw/actions/runs/22008549964/job/63614341607
Note how the existing options are wrapped in macros, like %Fe, which expands to the right option (either /Fe or -o ) depending on the compiler driver. For /DFAIL_CHECK, we can switch to -DFAIL_CHECK, which works both with the cl.exe/clang-cl interface, and plain clang as used in mingw mode. I'll push such a commit to unbreak my tests.
There was a problem hiding this comment.
Thanks @mstorsjo, my apologies for breaking! I'll make sure to add that to my instructions for future commits.
…vm#181015) This is a follow up to llvm#155943 On Windows, ASan's allocator internally upgrades zero-size allocation requests to size 1 (since malloc(0) must return a unique non-NULL pointer). However, when the user queries the allocation size through Windows heap APIs (RtlSizeHeap, HeapSize, \_msize, GlobalSize, LocalSize), ASan reports the internal size (1) instead of the originally requested size (0). This causes false positive heap-buffer-overflow errors in a common pattern: ```c++ void *buf = HeapAlloc(GetProcessHeap(), 0, 0); SIZE_T size = HeapSize(GetProcessHeap(), 0, buf); // Returns 1, should be 0 if(size > 0) // could remove this and still be correct memset(buf, 0, size); // ASan reports heap-buffer-overflow ``` The change adds a `from_zero_alloc` bit to `ChunkHeader` that tracks whether an allocation was originally zero-size. This bit fits in the existing spare capacity of the header's bitfield byte, so the 16-byte ChunkHeader size is unchanged, but it also isn't the most elegant. The 1-byte user region of a zero-size allocation is still poisoned, so any actual access to it is correctly reported as an overflow.
This is a follow up to #155943
On Windows, ASan's allocator internally upgrades zero-size allocation requests to size 1 (since malloc(0) must return a unique non-NULL pointer). However, when the user queries the allocation size through Windows heap APIs (RtlSizeHeap, HeapSize, _msize, GlobalSize, LocalSize), ASan reports the internal size (1) instead of the originally requested size (0).
This causes false positive heap-buffer-overflow errors in a common pattern:
The change adds a
from_zero_allocbit toChunkHeaderthat tracks whether an allocation was originally zero-size. This bit fits in the existing spare capacity of the header's bitfield byte, so the 16-byte ChunkHeader size is unchanged, but it also isn't the most elegant.The 1-byte user region of a zero-size allocation is still poisoned, so any actual access to it is correctly reported as an overflow.