Revert "[scudo] Only init RingBuffer when needed."

[fmayer]

Reverts #85994

This broke Android because we don't assign the lazily-initialized ring buffer to the libc globals

[llvmbot]

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

Author: Florian Mayer (fmayer)

Changes

Reverts llvm/llvm-project#85994

This broke Android because we don't assign the lazily-initialized ring buffer to the libc globals

---

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

2 Files Affected:

• (modified) compiler-rt/lib/scudo/standalone/combined.h (+10-21)
• (modified) compiler-rt/lib/scudo/standalone/tests/combined_test.cpp (+12-66)

diff --git a/compiler-rt/lib/scudo/standalone/combined.h b/compiler-rt/lib/scudo/standalone/combined.h
index 927513dea92dab..f59295382136bc 100644
--- a/compiler-rt/lib/scudo/standalone/combined.h
+++ b/compiler-rt/lib/scudo/standalone/combined.h
@@ -18,7 +18,6 @@
 #include "local_cache.h"
 #include "mem_map.h"
 #include "memtag.h"
-#include "mutex.h"
 #include "options.h"
 #include "quarantine.h"
 #include "report.h"
@@ -182,17 +181,17 @@ class Allocator {
     Quarantine.init(
         static_cast<uptr>(getFlags()->quarantine_size_kb << 10),
         static_cast<uptr>(getFlags()->thread_local_quarantine_size_kb << 10));
+
+    mapAndInitializeRingBuffer();
   }
 
-  void enableRingBuffer() NO_THREAD_SAFETY_ANALYSIS {
+  void enableRingBuffer() {
     AllocationRingBuffer *RB = getRingBuffer();
     if (RB)
       RB->Depot->enable();
-    RingBufferInitLock.unlock();
   }
 
-  void disableRingBuffer() NO_THREAD_SAFETY_ANALYSIS {
-    RingBufferInitLock.lock();
+  void disableRingBuffer() {
     AllocationRingBuffer *RB = getRingBuffer();
     if (RB)
       RB->Depot->disable();
@@ -919,11 +918,9 @@ class Allocator {
       DCHECK(!Primary.Options.load().get(OptionBit::TrackAllocationStacks));
       return;
     }
-
-    if (Track) {
-      initRingBufferMaybe();
+    if (Track)
       Primary.Options.set(OptionBit::TrackAllocationStacks);
-    } else
+    else
       Primary.Options.clear(OptionBit::TrackAllocationStacks);
   }
 
@@ -1098,9 +1095,6 @@ class Allocator {
                     0,
                 "invalid alignment");
 
-  // Lock to initialize the RingBuffer
-  HybridMutex RingBufferInitLock;
-
   // Pointer to memory mapped area starting with AllocationRingBuffer struct,
   // and immediately followed by Size elements of type Entry.
   atomic_uptr RingBufferAddress = {};
@@ -1555,16 +1549,11 @@ class Allocator {
         RBEntryStart)[N];
   }
 
-  void initRingBufferMaybe() {
-    ScopedLock L(RingBufferInitLock);
-    if (getRingBuffer() != nullptr)
+  void mapAndInitializeRingBuffer() {
+    if (getFlags()->allocation_ring_buffer_size <= 0)
       return;
-
-    int ring_buffer_size = getFlags()->allocation_ring_buffer_size;
-    if (ring_buffer_size <= 0)
-      return;
-
-    u32 AllocationRingBufferSize = static_cast<u32>(ring_buffer_size);
+    u32 AllocationRingBufferSize =
+        static_cast<u32>(getFlags()->allocation_ring_buffer_size);
 
     // We store alloc and free stacks for each entry.
     constexpr u32 kStacksPerRingBufferEntry = 2;
diff --git a/compiler-rt/lib/scudo/standalone/tests/combined_test.cpp b/compiler-rt/lib/scudo/standalone/tests/combined_test.cpp
index 1a36155bcd423a..6a311adc55e4bd 100644
--- a/compiler-rt/lib/scudo/standalone/tests/combined_test.cpp
+++ b/compiler-rt/lib/scudo/standalone/tests/combined_test.cpp
@@ -867,86 +867,32 @@ SCUDO_TYPED_TEST(ScudoCombinedTest, ReallocateInPlaceStress) {
   }
 }
 
-SCUDO_TYPED_TEST(ScudoCombinedTest, RingBufferDefaultDisabled) {
-  // The RingBuffer is not initialized until tracking is enabled for the
-  // first time.
-  auto *Allocator = this->Allocator.get();
-  EXPECT_EQ(0u, Allocator->getRingBufferSize());
-  EXPECT_EQ(nullptr, Allocator->getRingBufferAddress());
-}
-
-SCUDO_TYPED_TEST(ScudoCombinedTest, RingBufferInitOnce) {
-  auto *Allocator = this->Allocator.get();
-  Allocator->setTrackAllocationStacks(true);
-
-  auto RingBufferSize = Allocator->getRingBufferSize();
-  ASSERT_GT(RingBufferSize, 0u);
-  auto *RingBufferAddress = Allocator->getRingBufferAddress();
-  EXPECT_NE(nullptr, RingBufferAddress);
-
-  // Enable tracking again to verify that the initialization only happens once.
-  Allocator->setTrackAllocationStacks(true);
-  ASSERT_EQ(RingBufferSize, Allocator->getRingBufferSize());
-  EXPECT_EQ(RingBufferAddress, Allocator->getRingBufferAddress());
-}
-
 SCUDO_TYPED_TEST(ScudoCombinedTest, RingBufferSize) {
   auto *Allocator = this->Allocator.get();
-  Allocator->setTrackAllocationStacks(true);
-
-  auto RingBufferSize = Allocator->getRingBufferSize();
-  ASSERT_GT(RingBufferSize, 0u);
-  EXPECT_EQ(Allocator->getRingBufferAddress()[RingBufferSize - 1], '\0');
+  auto Size = Allocator->getRingBufferSize();
+  ASSERT_GT(Size, 0u);
+  EXPECT_EQ(Allocator->getRingBufferAddress()[Size - 1], '\0');
 }
 
 SCUDO_TYPED_TEST(ScudoCombinedTest, RingBufferAddress) {
   auto *Allocator = this->Allocator.get();
-  Allocator->setTrackAllocationStacks(true);
-
-  auto *RingBufferAddress = Allocator->getRingBufferAddress();
-  EXPECT_NE(RingBufferAddress, nullptr);
-  EXPECT_EQ(RingBufferAddress, Allocator->getRingBufferAddress());
-}
-
-SCUDO_TYPED_TEST(ScudoCombinedTest, StackDepotDefaultDisabled) {
-  // The StackDepot is not initialized until tracking is enabled for the
-  // first time.
-  auto *Allocator = this->Allocator.get();
-  EXPECT_EQ(0u, Allocator->getStackDepotSize());
-  EXPECT_EQ(nullptr, Allocator->getStackDepotAddress());
-}
-
-SCUDO_TYPED_TEST(ScudoCombinedTest, StackDepotInitOnce) {
-  auto *Allocator = this->Allocator.get();
-  Allocator->setTrackAllocationStacks(true);
-
-  auto StackDepotSize = Allocator->getStackDepotSize();
-  EXPECT_GT(StackDepotSize, 0u);
-  auto *StackDepotAddress = Allocator->getStackDepotAddress();
-  EXPECT_NE(nullptr, StackDepotAddress);
-
-  // Enable tracking again to verify that the initialization only happens once.
-  Allocator->setTrackAllocationStacks(true);
-  EXPECT_EQ(StackDepotSize, Allocator->getStackDepotSize());
-  EXPECT_EQ(StackDepotAddress, Allocator->getStackDepotAddress());
+  auto *Addr = Allocator->getRingBufferAddress();
+  EXPECT_NE(Addr, nullptr);
+  EXPECT_EQ(Addr, Allocator->getRingBufferAddress());
 }
 
 SCUDO_TYPED_TEST(ScudoCombinedTest, StackDepotSize) {
   auto *Allocator = this->Allocator.get();
-  Allocator->setTrackAllocationStacks(true);
-
-  auto StackDepotSize = Allocator->getStackDepotSize();
-  EXPECT_GT(StackDepotSize, 0u);
-  EXPECT_EQ(Allocator->getStackDepotAddress()[StackDepotSize - 1], '\0');
+  auto Size = Allocator->getStackDepotSize();
+  ASSERT_GT(Size, 0u);
+  EXPECT_EQ(Allocator->getStackDepotAddress()[Size - 1], '\0');
 }
 
 SCUDO_TYPED_TEST(ScudoCombinedTest, StackDepotAddress) {
   auto *Allocator = this->Allocator.get();
-  Allocator->setTrackAllocationStacks(true);
-
-  auto *StackDepotAddress = Allocator->getStackDepotAddress();
-  EXPECT_NE(StackDepotAddress, nullptr);
-  EXPECT_EQ(StackDepotAddress, Allocator->getStackDepotAddress());
+  auto *Addr = Allocator->getStackDepotAddress();
+  EXPECT_NE(Addr, nullptr);
+  EXPECT_EQ(Addr, Allocator->getStackDepotAddress());
 }
 
 SCUDO_TYPED_TEST(ScudoCombinedTest, StackDepot) {

[cferris1000]

Broke what? This has been in for a while, so why not fix forward?

[fmayer]

Broke what? This has been in for a while, so why not fix forward?

Broke the ring buffer for all apps.

[cferris1000]

Broke what? This has been in for a while, so why not fix forward?

Broke the ring buffer for all apps.

Are you saying that trying to enable it on Android doesn't work? What happens, it doesn't enable or something

Broke what? This has been in for a while, so why not fix forward?

Broke the ring buffer for all apps.

Give a day to try and figure out if there is an easy way to fix this. Is there a particular test I can run to verify if this is still broken?

[fmayer]

OK, let's fix forward instead.