[scudo] Fix c wrappers double free test.

[cferris1000]

The previous test simply tried to double free the pointer in the EXPECT_DEATH macro. Unfortunately, the gtest infrastructure can allocate a pointer that happens to be the previously freed pointer. Thus the free doesn't fail since the spawned process does not attempt to free all of the pointers allocated in the original test.

NOTE: Scudo should be checked to make sure that the TSD is not always returning pointers in the same order they are freed. Although this appears to be a problem with a program that only does a small number of allocations.

[llvmbot]

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

Author: Christopher Ferris (cferris1000)

Changes

The previous test simply tried to double free the pointer in the EXPECT_DEATH macro. Unfortunately, the gtest infrastructure can allocate a pointer that happens to be the previously freed pointer. Thus the free doesn't fail since the spawned process does not attempt to free all of the pointers allocated in the original test.

NOTE: Scudo should be checked to make sure that the TSD is not always returning pointers in the same order they are freed. Although this appears to be a problem with a program that only does a small number of allocations.

---

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

1 Files Affected:

• (modified) compiler-rt/lib/scudo/standalone/tests/wrappers_c_test.cpp (+13-1)

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 f5e17d7214863..05065444a70c5 100644
--- a/compiler-rt/lib/scudo/standalone/tests/wrappers_c_test.cpp
+++ b/compiler-rt/lib/scudo/standalone/tests/wrappers_c_test.cpp
@@ -175,7 +175,19 @@ TEST_F(ScudoWrappersCDeathTest, Malloc) {
 
   free(P);
   verifyDeallocHookPtr(P);
-  EXPECT_DEATH(free(P), "");
+
+  // Verify a double free causes an abort.
+  // Don't simply free(P) since EXPECT_DEATH will do a number of
+  // allocations before creating a new process. There is a possibility
+  // that the previously freed P is reused, therefore, in the new
+  // process doing free(P) is not a double free.
+  EXPECT_DEATH(
+      {
+        void *Ptr = malloc(Size);
+        free(Ptr);
+        free(Ptr);
+      },
+      "");
 
   P = malloc(0U);
   EXPECT_NE(P, nullptr);

[ChiaHungDuan]

Was this making the unittest flaky?

[ChiaHungDuan]

NOTE: Scudo should be checked to make sure that the TSD is not always returning pointers in the same order they are freed. Although this appears to be a problem with a program that only does a small number of allocations.
Currently, the order is FIFO and one reason behind this can be the locality. For small blocks, I guess it could be fine if we change the order a little bit. Like LIFO (make the cache work like a circular buffer). If we want something more complicated, it will add some latency to free and I'm not sure how big the impact is.