[scudo] do not store size inside ring buffer

[fmayer]

No description provided.

[llvmbot]

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

Author: Florian Mayer (fmayer)

Changes

---

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

3 Files Affected:

• (modified) compiler-rt/lib/scudo/standalone/combined.h (+28-19)
• (modified) compiler-rt/lib/scudo/standalone/fuzz/get_error_info_fuzzer.cpp (+3-6)
• (modified) compiler-rt/lib/scudo/standalone/wrappers_c_bionic.cpp (+2-3)

diff --git a/compiler-rt/lib/scudo/standalone/combined.h b/compiler-rt/lib/scudo/standalone/combined.h
index 25ad11dbf7ee5..8742fa93da277 100644
--- a/compiler-rt/lib/scudo/standalone/combined.h
+++ b/compiler-rt/lib/scudo/standalone/combined.h
@@ -14,6 +14,7 @@
 #include "flags.h"
 #include "flags_parser.h"
 #include "local_cache.h"
+#include "mem_map.h"
 #include "memtag.h"
 #include "options.h"
 #include "quarantine.h"
@@ -935,8 +936,7 @@ class Allocator {
 
   uptr getRingBufferSize() {
     initThreadMaybe();
-    auto *RingBuffer = getRingBuffer();
-    return RingBuffer ? ringBufferSizeInBytes(RingBuffer->Size) : 0;
+    return RingBufferElements ? ringBufferSizeInBytes(RingBufferElements) : 0;
   }
 
   static bool setRingBufferSizeForBuffer(char *Buffer, size_t Size) {
@@ -966,8 +966,9 @@ class Allocator {
   static void getErrorInfo(struct scudo_error_info *ErrorInfo,
                            uintptr_t FaultAddr, const char *DepotPtr,
                            const char *RegionInfoPtr, const char *RingBufferPtr,
-                           const char *Memory, const char *MemoryTags,
-                           uintptr_t MemoryAddr, size_t MemorySize) {
+                           size_t RingBufferSize, const char *Memory,
+                           const char *MemoryTags, uintptr_t MemoryAddr,
+                           size_t MemorySize) {
     *ErrorInfo = {};
     if (!allocatorSupportsMemoryTagging<Config>() ||
         MemoryAddr + MemorySize < MemoryAddr)
@@ -986,7 +987,7 @@ class Allocator {
     // Check the ring buffer. For primary allocations this will only find UAF;
     // for secondary allocations we can find either UAF or OOB.
     getRingBufferErrorInfo(ErrorInfo, NextErrorReport, FaultAddr, Depot,
-                           RingBufferPtr);
+                           RingBufferPtr, RingBufferSize);
 
     // Check for OOB in the 28 blocks surrounding the 3 we checked earlier.
     // Beyond that we are likely to hit false positives.
@@ -1051,15 +1052,15 @@ class Allocator {
       atomic_u32 DeallocationTid;
     };
 
-    MemMapT MemMap;
     atomic_uptr Pos;
-    u32 Size;
     // An array of Size (at least one) elements of type Entry is immediately
     // following to this struct.
   };
   // Pointer to memory mapped area starting with AllocationRingBuffer struct,
   // and immediately followed by Size elements of type Entry.
   char *RawRingBuffer = {};
+  u32 RingBufferElements = 0;
+  MemMapT RawRingBufferMap;
 
   // The following might get optimized out by the compiler.
   NOINLINE void performSanityChecks() {
@@ -1267,7 +1268,7 @@ class Allocator {
                             u32 DeallocationTid) {
     uptr Pos = atomic_fetch_add(&getRingBuffer()->Pos, 1, memory_order_relaxed);
     typename AllocationRingBuffer::Entry *Entry =
-        getRingBufferEntry(RawRingBuffer, Pos % getRingBuffer()->Size);
+        getRingBufferEntry(RawRingBuffer, Pos % RingBufferElements);
 
     // First invalidate our entry so that we don't attempt to interpret a
     // partially written state in getSecondaryErrorInfo(). The fences below
@@ -1408,17 +1409,19 @@ class Allocator {
                                      size_t &NextErrorReport,
                                      uintptr_t FaultAddr,
                                      const StackDepot *Depot,
-                                     const char *RingBufferPtr) {
+                                     const char *RingBufferPtr,
+                                     size_t RingBufferSize) {
     auto *RingBuffer =
         reinterpret_cast<const AllocationRingBuffer *>(RingBufferPtr);
-    if (!RingBuffer || RingBuffer->Size == 0)
+    size_t RingBufferElements = ringBufferElementsFromBytes(RingBufferSize);
+    if (!RingBuffer || RingBufferElements == 0)
       return;
     uptr Pos = atomic_load_relaxed(&RingBuffer->Pos);
 
-    for (uptr I = Pos - 1;
-         I != Pos - 1 - RingBuffer->Size && NextErrorReport != NumErrorReports;
+    for (uptr I = Pos - 1; I != Pos - 1 - RingBufferElements &&
+                           NextErrorReport != NumErrorReports;
          --I) {
-      auto *Entry = getRingBufferEntry(RingBufferPtr, I % RingBuffer->Size);
+      auto *Entry = getRingBufferEntry(RingBufferPtr, I % RingBufferElements);
       uptr EntryPtr = atomic_load_relaxed(&Entry->Ptr);
       if (!EntryPtr)
         continue;
@@ -1508,9 +1511,8 @@ class Allocator {
                 getPageSizeCached()),
         "scudo:ring_buffer");
     RawRingBuffer = reinterpret_cast<char *>(MemMap.getBase());
-    auto *RingBuffer = reinterpret_cast<AllocationRingBuffer *>(RawRingBuffer);
-    RingBuffer->MemMap = MemMap;
-    RingBuffer->Size = AllocationRingBufferSize;
+    RawRingBufferMap = MemMap;
+    RingBufferElements = AllocationRingBufferSize;
     static_assert(sizeof(AllocationRingBuffer) %
                           alignof(typename AllocationRingBuffer::Entry) ==
                       0,
@@ -1526,10 +1528,17 @@ class Allocator {
     RawRingBuffer = nullptr;
   }
 
-  static constexpr size_t ringBufferSizeInBytes(u32 AllocationRingBufferSize) {
+  static constexpr size_t ringBufferSizeInBytes(u32 RingBufferElements) {
     return sizeof(AllocationRingBuffer) +
-           AllocationRingBufferSize *
-               sizeof(typename AllocationRingBuffer::Entry);
+           RingBufferElements * sizeof(typename AllocationRingBuffer::Entry);
+  }
+
+  static constexpr size_t ringBufferElementsFromBytes(size_t Bytes) {
+    if (Bytes < sizeof(AllocationRingBuffer)) {
+      return 0;
+    }
+    return (Bytes - sizeof(AllocationRingBuffer)) /
+           sizeof(typename AllocationRingBuffer::Entry);
   }
 
   inline AllocationRingBuffer *getRingBuffer() {
diff --git a/compiler-rt/lib/scudo/standalone/fuzz/get_error_info_fuzzer.cpp b/compiler-rt/lib/scudo/standalone/fuzz/get_error_info_fuzzer.cpp
index 74456450a4761..5b01ebe11c095 100644
--- a/compiler-rt/lib/scudo/standalone/fuzz/get_error_info_fuzzer.cpp
+++ b/compiler-rt/lib/scudo/standalone/fuzz/get_error_info_fuzzer.cpp
@@ -46,14 +46,11 @@ extern "C" int LLVMFuzzerTestOneInput(uint8_t *Data, size_t Size) {
   }
 
   std::string RingBufferBytes = FDP.ConsumeRemainingBytesAsString();
-  // RingBuffer is too short.
-  if (!AllocatorT::setRingBufferSizeForBuffer(RingBufferBytes.data(),
-                                              RingBufferBytes.size()))
-    return 0;
 
   scudo_error_info ErrorInfo;
   AllocatorT::getErrorInfo(&ErrorInfo, FaultAddr, StackDepot.data(),
-                           RegionInfo.data(), RingBufferBytes.data(), Memory,
-                           MemoryTags, MemoryAddr, MemorySize);
+                           RegionInfo.data(), RingBufferBytes.data(),
+                           RingBufferBytes.size(), Memory, MemoryTags,
+                           MemoryAddr, MemorySize);
   return 0;
 }
diff --git a/compiler-rt/lib/scudo/standalone/wrappers_c_bionic.cpp b/compiler-rt/lib/scudo/standalone/wrappers_c_bionic.cpp
index f203615ab3602..21694c3f17fe5 100644
--- a/compiler-rt/lib/scudo/standalone/wrappers_c_bionic.cpp
+++ b/compiler-rt/lib/scudo/standalone/wrappers_c_bionic.cpp
@@ -44,10 +44,9 @@ INTERFACE void __scudo_get_error_info(
     const char *ring_buffer, size_t ring_buffer_size, const char *memory,
     const char *memory_tags, uintptr_t memory_addr, size_t memory_size) {
   (void)(stack_depot_size);
-  (void)(ring_buffer_size);
   Allocator.getErrorInfo(error_info, fault_addr, stack_depot, region_info,
-                         ring_buffer, memory, memory_tags, memory_addr,
-                         memory_size);
+                         ring_buffer, ring_buffer_size, memory, memory_tags,
+                         memory_addr, memory_size);
 }
 
 INTERFACE const char *__scudo_get_stack_depot_addr() {

[github-actions]

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

[cferris1000]

Is there ever a case where RingBuffer is not null and RingBufferElements is 0?

If not, maybe it would be better to change this to something like:

if (!RingBuffer)
return;

size_t RingBufferElements = ...;
DCHECK(RingBufferElements != 0);

[fmayer]

This function assumes arbitrary input (and is fuzzed with it), so I did it like this to be on the safe side. Checking on the caller makes less sense, because RingBufferElements == 0 means RingBufferSize < sizeof(AllocationRingBuffer) + sizeof(AllocationRingBuffer::Entry).

[cferris1000]

Sounds reasonable to me.

[cferris1000]

LGTM.

[cferris1000]

Sounds reasonable to me.