From a4e45ad1c6efb821504371e3800f9ecf925aaa22 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lodri=20P=C3=A9ter?= <267054113+peterlodri-sec@users.noreply.github.com> Date: Mon, 11 May 2026 12:45:53 +0200 Subject: [PATCH] optimize slab quarantine: unify random+FIFO into single array Combine the two-stage slab quarantine (random array + FIFO queue) into a single random-replacement array. This reduces per-free operations from 2 pointer swaps to 1, improving performance while keeping the total quarantine slot count identical. - Replace CONFIG_SLAB_QUARANTINE_RANDOM_LENGTH and CONFIG_SLAB_QUARANTINE_QUEUE_LENGTH with unified CONFIG_SLAB_QUARANTINE_LENGTH - Default: 2 (same total slots as old 1+1) - Light: 0 (unchanged) - Remove stale FIFO references from README - Add extended-size-class quarantine tests Fixes #179 --- Android.bp | 3 +- Makefile | 3 +- README.md | 23 +++---- config/default.mk | 3 +- config/light.mk | 3 +- h_malloc.c | 63 +++++-------------- test/Makefile | 7 ++- test/quarantine_double_free_extended.c | 13 ++++ .../quarantine_double_free_extended_delayed.c | 23 +++++++ ...tine_invalid_malloc_object_size_extended.c | 14 +++++ ...tine_invalid_malloc_usable_size_extended.c | 14 +++++ ...arantine_write_after_free_extended_reuse.c | 17 +++++ test/test_smc.py | 38 +++++++++++ 13 files changed, 151 insertions(+), 73 deletions(-) create mode 100644 test/quarantine_double_free_extended.c create mode 100644 test/quarantine_double_free_extended_delayed.c create mode 100644 test/quarantine_invalid_malloc_object_size_extended.c create mode 100644 test/quarantine_invalid_malloc_usable_size_extended.c create mode 100644 test/quarantine_write_after_free_extended_reuse.c diff --git a/Android.bp b/Android.bp index 3926aac6..a6c73624 100644 --- a/Android.bp +++ b/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", diff --git a/Makefile b/Makefile index de8d82c0..c3b7670a 100644 --- a/Makefile +++ b/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) \ diff --git a/README.md b/README.md index 089dd1ab..93895e2b 100644 --- a/README.md +++ b/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. diff --git a/config/default.mk b/config/default.mk index d8f03a78..c6c9f898 100644 --- a/config/default.mk +++ b/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 diff --git a/config/light.mk b/config/light.mk index da8676d9..5849a392 100644 --- a/config/light.mk +++ b/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 diff --git a/h_malloc.c b/h_malloc.c index c6c58e03..b0912f7f 100644 --- a/h_malloc.c +++ b/h_malloc.c @@ -24,7 +24,7 @@ #include #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) @@ -32,10 +32,8 @@ 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); } diff --git a/test/Makefile b/test/Makefile index 41b48a42..e9274b6b 100644 --- a/test/Makefile +++ b/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) diff --git a/test/quarantine_double_free_extended.c b/test/quarantine_double_free_extended.c new file mode 100644 index 00000000..15b81f94 --- /dev/null +++ b/test/quarantine_double_free_extended.c @@ -0,0 +1,13 @@ +#include + +#include "test_util.h" + +OPTNONE int main(void) { + void *p = malloc(32768); + if (!p) { + return 1; + } + free(p); + free(p); + return 0; +} diff --git a/test/quarantine_double_free_extended_delayed.c b/test/quarantine_double_free_extended_delayed.c new file mode 100644 index 00000000..0330e5b3 --- /dev/null +++ b/test/quarantine_double_free_extended_delayed.c @@ -0,0 +1,23 @@ +#include + +#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; +} diff --git a/test/quarantine_invalid_malloc_object_size_extended.c b/test/quarantine_invalid_malloc_object_size_extended.c new file mode 100644 index 00000000..7a4a03d8 --- /dev/null +++ b/test/quarantine_invalid_malloc_object_size_extended.c @@ -0,0 +1,14 @@ +#include + +#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; +} diff --git a/test/quarantine_invalid_malloc_usable_size_extended.c b/test/quarantine_invalid_malloc_usable_size_extended.c new file mode 100644 index 00000000..a755d16b --- /dev/null +++ b/test/quarantine_invalid_malloc_usable_size_extended.c @@ -0,0 +1,14 @@ +#include +#include + +#include "test_util.h" + +OPTNONE int main(void) { + void *p = malloc(32768); + if (!p) { + return 1; + } + free(p); + malloc_usable_size(p); + return 0; +} diff --git a/test/quarantine_write_after_free_extended_reuse.c b/test/quarantine_write_after_free_extended_reuse.c new file mode 100644 index 00000000..c6fe6611 --- /dev/null +++ b/test/quarantine_write_after_free_extended_reuse.c @@ -0,0 +1,17 @@ +#include + +#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; +} diff --git a/test/test_smc.py b/test/test_smc.py index 0d5665b1..d00ac427 100644 --- a/test/test_smc.py +++ b/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()