optimize slab quarantine: unify random+FIFO into single array

Android.bp

@@ -14,8 +14,7 @@ common_cflags = [
     "-DWRITE_AFTER_FREE_CHECK=true",
     "-DSLOT_RANDOMIZE=true",
     "-DSLAB_CANARY=true",
-    "-DSLAB_QUARANTINE_RANDOM_LENGTH=1",
-    "-DSLAB_QUARANTINE_QUEUE_LENGTH=1",
+    "-DSLAB_QUARANTINE_LENGTH=2",
     "-DCONFIG_EXTENDED_SIZE_CLASSES=true",
     "-DCONFIG_LARGE_SIZE_CLASSES=true",
     "-DGUARD_SLABS_INTERVAL=1",

Makefile

@@ -99,8 +99,7 @@ CPPFLAGS += \
     -DWRITE_AFTER_FREE_CHECK=$(CONFIG_WRITE_AFTER_FREE_CHECK) \
     -DSLOT_RANDOMIZE=$(CONFIG_SLOT_RANDOMIZE) \
     -DSLAB_CANARY=$(CONFIG_SLAB_CANARY) \
-    -DSLAB_QUARANTINE_RANDOM_LENGTH=$(CONFIG_SLAB_QUARANTINE_RANDOM_LENGTH) \
-    -DSLAB_QUARANTINE_QUEUE_LENGTH=$(CONFIG_SLAB_QUARANTINE_QUEUE_LENGTH) \
+    -DSLAB_QUARANTINE_LENGTH=$(CONFIG_SLAB_QUARANTINE_LENGTH) \
     -DCONFIG_EXTENDED_SIZE_CLASSES=$(CONFIG_EXTENDED_SIZE_CLASSES) \
     -DCONFIG_LARGE_SIZE_CLASSES=$(CONFIG_LARGE_SIZE_CLASSES) \
     -DGUARD_SLABS_INTERVAL=$(CONFIG_GUARD_SLABS_INTERVAL) \

README.md

@@ -54,7 +54,7 @@ address space. There are lots of smaller differences in the implementation
 approach. It incorporates the previous extensions made to OpenBSD malloc
 including adding padding to allocations for canaries (distinct from the current
 OpenBSD malloc canaries), write-after-free detection tied to the existing
-clearing on free, queues alongside the existing randomized arrays for
+clearing on free, unified randomized quarantine arrays for
 quarantining allocations and proper double-free detection for quarantined
 allocations. The per-size-class memory regions with their own random bases were
 loosely inspired by the size and type-based partitioning in PartitionAlloc. The
@@ -282,20 +282,13 @@ The following boolean configuration options are available:
 
 The following integer configuration options are available:
 
-* `CONFIG_SLAB_QUARANTINE_RANDOM_LENGTH`: `1` (default) to control the number
-  of slots in the random array used to randomize reuse for small memory
+* `CONFIG_SLAB_QUARANTINE_LENGTH`: `2` (default) to control the number of
+  slots in the quarantine used to randomize and delay reuse for small memory
   allocations. This sets the length for the largest size class (either 16kiB
   or 128kiB based on `CONFIG_EXTENDED_SIZE_CLASSES`) and the quarantine length
   for smaller size classes is scaled to match the total memory of the
-  quarantined allocations (1 becomes 1024 for 16 byte allocations with 16kiB
-  as the largest size class, or 8192 with 128kiB as the largest).
-* `CONFIG_SLAB_QUARANTINE_QUEUE_LENGTH`: `1` (default) to control the number of
-  slots in the queue used to delay reuse for small memory allocations. This
-  sets the length for the largest size class (either 16kiB or 128kiB based on
-  `CONFIG_EXTENDED_SIZE_CLASSES`) and the quarantine length for smaller size
-  classes is scaled to match the total memory of the quarantined allocations (1
-  becomes 1024 for 16 byte allocations with 16kiB as the largest size class, or
-  8192 with 128kiB as the largest).
+  quarantined allocations (2 becomes 2048 for 16 byte allocations with 16kiB
+  as the largest size class, or 16384 with 128kiB as the largest).
 * `CONFIG_GUARD_SLABS_INTERVAL`: `1` (default) to control the number of slabs
   before a slab is skipped and left as an unused memory protected guard slab.
   The default of `1` leaves a guard slab between every slab. This feature does
@@ -445,7 +438,7 @@ was a bit less important and if a core goal was finding latent bugs.
 * Slab allocations are zeroed on free
 * Detection of write-after-free for slab allocations by verifying zero filling
   is intact at allocation time
-* Delayed free via a combination of FIFO and randomization for slab allocations
+* Delayed free via randomized quarantine arrays for slab allocations
 * Large allocations are purged and memory protected on free with the memory
   mapping kept reserved in a quarantine to detect use-after-free
     * The quarantine is primarily based on a FIFO ring buffer, with the oldest
@@ -741,8 +734,8 @@ This ensures the following properties:
 
 - Linear overflows are deterministically detected.
 - Use-after-free are deterministically detected until the freed slot goes through
-  both the random and FIFO quarantines, gets allocated again, goes through both
-  quarantines again and then finally gets allocated again for a 2nd time.
+  the quarantine, gets allocated again, goes through the quarantine again and
+  then finally gets allocated again for a 2nd time.
 - Since the default `0` tag is reserved, untagged pointers can't access slab
   allocations and vice versa.
 

config/default.mk

@@ -7,8 +7,7 @@ CONFIG_ZERO_ON_FREE := true
 CONFIG_WRITE_AFTER_FREE_CHECK := true
 CONFIG_SLOT_RANDOMIZE := true
 CONFIG_SLAB_CANARY := true
-CONFIG_SLAB_QUARANTINE_RANDOM_LENGTH := 1
-CONFIG_SLAB_QUARANTINE_QUEUE_LENGTH := 1
+CONFIG_SLAB_QUARANTINE_LENGTH := 2
 CONFIG_EXTENDED_SIZE_CLASSES := true
 CONFIG_LARGE_SIZE_CLASSES := true
 CONFIG_GUARD_SLABS_INTERVAL := 1

config/light.mk

@@ -7,8 +7,7 @@ CONFIG_ZERO_ON_FREE := true
 CONFIG_WRITE_AFTER_FREE_CHECK := false
 CONFIG_SLOT_RANDOMIZE := false
 CONFIG_SLAB_CANARY := true
-CONFIG_SLAB_QUARANTINE_RANDOM_LENGTH := 0
-CONFIG_SLAB_QUARANTINE_QUEUE_LENGTH := 0
+CONFIG_SLAB_QUARANTINE_LENGTH := 0
 CONFIG_EXTENDED_SIZE_CLASSES := true
 CONFIG_LARGE_SIZE_CLASSES := true
 CONFIG_GUARD_SLABS_INTERVAL := 8

h_malloc.c

@@ -24,18 +24,16 @@
 #include <sys/mman.h>
 #endif
 
-#define SLAB_QUARANTINE (SLAB_QUARANTINE_RANDOM_LENGTH > 0 || SLAB_QUARANTINE_QUEUE_LENGTH > 0)
+#define SLAB_QUARANTINE (SLAB_QUARANTINE_LENGTH > 0)
 #define REGION_QUARANTINE (REGION_QUARANTINE_RANDOM_LENGTH > 0 || REGION_QUARANTINE_QUEUE_LENGTH > 0)
 #define MREMAP_MOVE_THRESHOLD ((size_t)32 * 1024 * 1024)
 
 static_assert(sizeof(void *) == 8, "64-bit only");
 
 static_assert(!WRITE_AFTER_FREE_CHECK || ZERO_ON_FREE, "WRITE_AFTER_FREE_CHECK depends on ZERO_ON_FREE");
 
-static_assert(SLAB_QUARANTINE_RANDOM_LENGTH >= 0 && SLAB_QUARANTINE_RANDOM_LENGTH <= 65536,
-    "invalid slab quarantine random length");
-static_assert(SLAB_QUARANTINE_QUEUE_LENGTH >= 0 && SLAB_QUARANTINE_QUEUE_LENGTH <= 65536,
-    "invalid slab quarantine queue length");
+static_assert(SLAB_QUARANTINE_LENGTH >= 0 && SLAB_QUARANTINE_LENGTH <= 65536,
+    "invalid slab quarantine length");
 static_assert(REGION_QUARANTINE_RANDOM_LENGTH >= 0 && REGION_QUARANTINE_RANDOM_LENGTH <= 65536,
     "invalid region quarantine random length");
 static_assert(REGION_QUARANTINE_QUEUE_LENGTH >= 0 && REGION_QUARANTINE_QUEUE_LENGTH <= 65536,
@@ -309,13 +307,8 @@ struct __attribute__((aligned(CACHELINE_SIZE))) size_class {
     size_t metadata_count;
     size_t metadata_count_unguarded;
 
-#if SLAB_QUARANTINE_QUEUE_LENGTH > 0
-    size_t quarantine_queue_index;
-    void *quarantine_queue[SLAB_QUARANTINE_QUEUE_LENGTH << (MAX_SLAB_SIZE_CLASS_SHIFT - MIN_SLAB_SIZE_CLASS_SHIFT)];
-#endif
-
-#if SLAB_QUARANTINE_RANDOM_LENGTH > 0
-    void *quarantine_random[SLAB_QUARANTINE_RANDOM_LENGTH << (MAX_SLAB_SIZE_CLASS_SHIFT - MIN_SLAB_SIZE_CLASS_SHIFT)];
+#if SLAB_QUARANTINE_LENGTH > 0
+    void *quarantine[SLAB_QUARANTINE_LENGTH << (MAX_SLAB_SIZE_CLASS_SHIFT - MIN_SLAB_SIZE_CLASS_SHIFT)];
 #endif
 };
 
@@ -856,12 +849,12 @@ static inline void deallocate_small(void *p, const size_t *expected_size) {
 
     size_t quarantine_shift = clz64(size) - (63 - MAX_SLAB_SIZE_CLASS_SHIFT);
 
-#if SLAB_QUARANTINE_RANDOM_LENGTH > 0
-    size_t slab_quarantine_random_length = SLAB_QUARANTINE_RANDOM_LENGTH << quarantine_shift;
+#if SLAB_QUARANTINE_LENGTH > 0
+    size_t slab_quarantine_length = SLAB_QUARANTINE_LENGTH << quarantine_shift;
 
-    size_t random_index = get_random_u16_uniform(&c->rng, slab_quarantine_random_length);
-    void *random_substitute = c->quarantine_random[random_index];
-    c->quarantine_random[random_index] = p;
+    size_t random_index = get_random_u16_uniform(&c->rng, slab_quarantine_length);
+    void *random_substitute = c->quarantine[random_index];
+    c->quarantine[random_index] = p;
 
     if (random_substitute == NULL) {
         mutex_unlock(&c->lock);
@@ -871,24 +864,6 @@ static inline void deallocate_small(void *p, const size_t *expected_size) {
     p = random_substitute;
 #endif
 
-#if SLAB_QUARANTINE_QUEUE_LENGTH > 0
-    size_t slab_quarantine_queue_length = SLAB_QUARANTINE_QUEUE_LENGTH << quarantine_shift;
-
-    void *queue_substitute = c->quarantine_queue[c->quarantine_queue_index];
-    c->quarantine_queue[c->quarantine_queue_index] = p;
-
-    // Modulo here is costly so we're using an increment and an if instead.
-    size_t next_queue_index = c->quarantine_queue_index + 1;
-    c->quarantine_queue_index = next_queue_index < slab_quarantine_queue_length ? next_queue_index : 0;
-
-    if (queue_substitute == NULL) {
-        mutex_unlock(&c->lock);
-        return;
-    }
-
-    p = queue_substitute;
-#endif
-
     metadata = get_metadata(c, p);
     slab = get_slab(c, slab_size, metadata);
     slot = libdivide_u32_do((char *)p - (char *)slab, &c->size_divisor);
@@ -2002,20 +1977,10 @@ EXPORT int h_malloc_trim(UNUSED size_t pad) {
             if (size >= min_extended_size_class) {
                 size_t quarantine_shift = clz64(size) - (63 - MAX_SLAB_SIZE_CLASS_SHIFT);
 
-#if SLAB_QUARANTINE_RANDOM_LENGTH > 0
-                size_t slab_quarantine_random_length = SLAB_QUARANTINE_RANDOM_LENGTH << quarantine_shift;
-                for (size_t i = 0; i < slab_quarantine_random_length; i++) {
-                    void *p = c->quarantine_random[i];
-                    if (p != NULL) {
-                        memory_purge(p, size);
-                    }
-                }
-#endif
-
-#if SLAB_QUARANTINE_QUEUE_LENGTH > 0
-                size_t slab_quarantine_queue_length = SLAB_QUARANTINE_QUEUE_LENGTH << quarantine_shift;
-                for (size_t i = 0; i < slab_quarantine_queue_length; i++) {
-                    void *p = c->quarantine_queue[i];
+#if SLAB_QUARANTINE_LENGTH > 0
+                size_t slab_quarantine_length = SLAB_QUARANTINE_LENGTH << quarantine_shift;
+                for (size_t i = 0; i < slab_quarantine_length; i++) {
+                    void *p = c->quarantine[i];
                     if (p != NULL) {
                         memory_purge(p, size);
                     }

test/Makefile

@@ -75,7 +75,12 @@ EXECUTABLES := \
     impossibly_large_malloc \
     realloc_init \
     malloc_zero_different \
-    malloc_noreuse
+    malloc_noreuse \
+    quarantine_double_free_extended \
+    quarantine_double_free_extended_delayed \
+    quarantine_invalid_malloc_usable_size_extended \
+    quarantine_invalid_malloc_object_size_extended \
+    quarantine_write_after_free_extended_reuse
 
 all: $(EXECUTABLES)
 

test/quarantine_double_free_extended.c

@@ -0,0 +1,13 @@
+#include <stdlib.h>
+
+#include "test_util.h"
+
+OPTNONE int main(void) {
+    void *p = malloc(32768);
+    if (!p) {
+        return 1;
+    }
+    free(p);
+    free(p);
+    return 0;
+}

test/quarantine_double_free_extended_delayed.c

@@ -0,0 +1,23 @@
+#include <stdlib.h>
+
+#include "test_util.h"
+
+OPTNONE int main(void) {
+    void *p = malloc(65536);
+    if (!p) {
+        return 1;
+    }
+    void *allocs[100];
+    for (int i = 0; i < 100; i++) {
+        allocs[i] = malloc(65536);
+        if (!allocs[i]) {
+            return 1;
+        }
+    }
+    free(p);
+    for (int i = 0; i < 100; i++) {
+        free(allocs[i]);
+    }
+    free(p);
+    return 0;
+}

test/quarantine_invalid_malloc_object_size_extended.c

@@ -0,0 +1,14 @@
+#include <stdlib.h>
+
+#include "test_util.h"
+#include "../include/h_malloc.h"
+
+OPTNONE int main(void) {
+    void *p = malloc(32768);
+    if (!p) {
+        return 1;
+    }
+    free(p);
+    h_malloc_object_size(p);
+    return 0;
+}

test/quarantine_invalid_malloc_usable_size_extended.c

@@ -0,0 +1,14 @@
+#include <stdlib.h>
+#include <malloc.h>
+
+#include "test_util.h"
+
+OPTNONE int main(void) {
+    void *p = malloc(32768);
+    if (!p) {
+        return 1;
+    }
+    free(p);
+    malloc_usable_size(p);
+    return 0;
+}

test/quarantine_write_after_free_extended_reuse.c

@@ -0,0 +1,17 @@
+#include <stdlib.h>
+
+#include "test_util.h"
+#include "../util.h"
+
+OPTNONE int main(void) {
+    char *p = malloc(32768);
+    if (!p) {
+        return 1;
+    }
+    free(p);
+    p[100] = 'a';
+    for (size_t i = 0; i < 10000; i++) {
+        free(malloc(32768));
+    }
+    return 0;
+}

test/test_smc.py

@@ -287,5 +287,43 @@ def test_malloc_noreuse(self):
             "malloc_noreuse")
         self.assertEqual(returncode, 0)
 
+    def test_quarantine_double_free_extended(self):
+        _stdout, stderr, returncode = self.run_test(
+            "quarantine_double_free_extended")
+        self.assertEqual(returncode, -6)
+        self.assertEqual(stderr.decode("utf-8"),
+                         "fatal allocator error: double free (quarantine)\n")
+
+    def test_quarantine_double_free_extended_delayed(self):
+        _stdout, stderr, returncode = self.run_test(
+            "quarantine_double_free_extended_delayed")
+        self.assertEqual(returncode, -6)
+        # with quarantine length 4 for 65536 bytes (default L=2, shift=1), freeing 100 allocations
+        # almost certainly displaces the first pointer from quarantine
+        self.assertIn(stderr.decode("utf-8"),
+                      ["fatal allocator error: double free\n",
+                       "fatal allocator error: double free (quarantine)\n"])
+
+    def test_quarantine_invalid_malloc_usable_size_extended(self):
+        _stdout, stderr, returncode = self.run_test(
+            "quarantine_invalid_malloc_usable_size_extended")
+        self.assertEqual(returncode, -6)
+        self.assertEqual(stderr.decode("utf-8"),
+                         "fatal allocator error: invalid malloc_usable_size (quarantine)\n")
+
+    def test_quarantine_invalid_malloc_object_size_extended(self):
+        _stdout, stderr, returncode = self.run_test(
+            "quarantine_invalid_malloc_object_size_extended")
+        self.assertEqual(returncode, -6)
+        self.assertEqual(stderr.decode("utf-8"),
+                         "fatal allocator error: invalid malloc_object_size (quarantine)\n")
+
+    def test_quarantine_write_after_free_extended_reuse(self):
+        _stdout, stderr, returncode = self.run_test(
+            "quarantine_write_after_free_extended_reuse")
+        self.assertEqual(returncode, -6)
+        self.assertEqual(stderr.decode("utf-8"),
+                         "fatal allocator error: detected write after free\n")
+
 if __name__ == '__main__':
     unittest.main()