From a18e42ea9c8a2fc7aca10349227a569ab103c543 Mon Sep 17 00:00:00 2001 From: Doug Torrance Date: Sat, 29 Aug 2026 18:54:05 -0400 Subject: [PATCH 1/2] Deallocate the blocks that restoreToGuardPoint frees restoreToGuardPoint emptied every block that was allocated after the guard point was taken, but never deallocated them. guardPoint() and freeTopFromOldBlock() both require that a block behind the front block is not empty, so this left the arena in a state that they mishandle. It takes two blocks inside the guard to show up, as with only one the sole empty block is the front block, which is allowed: memt::Arena arena; void* a = arena.alloc(1); { memt::Arena::Guard guard(arena); arena.alloc(arena.getMemoryUse() + 1); arena.alloc(arena.getMemoryUse() + 1); } arena.freeTop(a); // a is the only allocation that is still live That aborts in freeTopFromOldBlock when MEMT_DEBUG is defined. Without it the position of one block is set to an address inside another, and getAllocatedMemoryUse() then wraps around to a huge value. Free them as freeAndAllAfterFromOldBlock already does. The front block is kept, as it is kept there too. Drop the record of the allocations in the blocks being freed before freeing them, so that no freed address is used afterwards. Note that this is not the only way to get an empty block behind the front block. growCapacity does it as well, by chaining an empty front block behind a newly allocated one, and that is not fixed here. Co-Authored-By: Claude Opus 5 --- src/memtailor/Arena.h | 25 +++++++++++++++++++------ src/test/ArenaTest.cpp | 29 +++++++++++++++++++++++++++++ 2 files changed, 48 insertions(+), 6 deletions(-) diff --git a/src/memtailor/Arena.h b/src/memtailor/Arena.h index 29fa053..017c04e 100644 --- a/src/memtailor/Arena.h +++ b/src/memtailor/Arena.h @@ -433,15 +433,28 @@ namespace memt { return; } + // The blocks allocated since the guard point have to be deallocated, + // as guardPoint() and freeTopFromOldBlock() both rely on a block + // behind the front block never being empty. Block* b = &block(); - while (!(b->begin() < ptr && ptr <= b->end())) { + if (!(b->begin() < ptr && ptr <= b->end())) { b->setPosition(b->begin()); - b = b->previousBlock(); + do { + // If you get an assert here then most likely the guard point was + // invalidated by deallocating memory that was live at the point + // the guard point was created. + MEMT_ASSERT(b->hasPreviousBlock()); + b = b->previousBlock(); + } while (!(b->begin() < ptr && ptr <= b->end())); - // If you get an assert here then most likely the guard point was - // invalidated by deallocating memory that was live at the point - // the guard point was created. - MEMT_ASSERT(b != 0); +#ifdef MEMT_DEBUG + // Drop the record of the allocations in the blocks that are about to + // be freed while their addresses are still valid. + while (!_debugAllocs.empty() && !b->isInBlock(_debugAllocs.back())) + _debugAllocs.pop_back(); +#endif + while (block().previousBlock() != b) + _blocks.freePreviousBlock(); } MEMT_ASSERT(b->begin() < ptr && ptr <= b->end()); b->setPosition(ptr); diff --git a/src/test/ArenaTest.cpp b/src/test/ArenaTest.cpp index 6887659..6f0c76d 100644 --- a/src/test/ArenaTest.cpp +++ b/src/test/ArenaTest.cpp @@ -306,3 +306,32 @@ TEST(Arena, GuardAfterEmptyRestore) { #endif memt::Arena::Guard guard2(arena); // must be possible on an empty arena } + +TEST(Arena, GuardFreesNewerBlocks) { + memt::Arena arena; + void* a = arena.alloc(1); + size_t memoryUse = 0; + { + memt::Arena::Guard guard(arena); + // Two new blocks, not one: with one, the only block left empty is + // the front block, which is allowed. With two there is also an empty + // block behind it, which is not. + arena.alloc(arena.getMemoryUse() + 1); + arena.alloc(arena.getMemoryUse() + 1); + memoryUse = arena.getMemoryUse(); + } + // the block in the middle is deallocated. As for freeAndAllAfter, the + // front block is kept, so not all of the memory is released. + ASSERT_LT(arena.getMemoryUse(), memoryUse); + + { + memt::Arena::Guard guard(arena); // requires no empty block behind the front + } + + arena.freeTop(a); // a is the only allocation that is still live + ASSERT_TRUE(arena.isEmpty()); + ASSERT_EQ(arena.getAllocatedMemoryUse(), 0u); +#ifdef MEMT_DEBUG + ASSERT_EQ(arena.debugAllocCount(), 0u); +#endif +} From f7f39f9b49514e2b1690804f70ae8f026a9b2518 Mon Sep 17 00:00:00 2001 From: Doug Torrance Date: Sat, 29 Aug 2026 19:46:38 -0400 Subject: [PATCH 2/2] Deallocate an empty block when growing the capacity alloc() grows the capacity whenever a request does not fit in what is left of the front block, including when the front block is empty. The old front block was then chained behind the new one regardless, so an empty block ended up behind the front block. guardPoint() and freeTopFromOldBlock() both require that this does not happen. No guard point is needed to reach it: memt::Arena a; void* p = a.alloc(1); // block A void* q = a.alloc(a.getMemoryUse() + 1); // block B, A behind it a.freeTop(q); // B is empty now void* r = a.alloc(a.getMemoryUse() + 1); // block C, B empty behind it a.freeTop(r); a.freeTop(p); // p is in A, but B is examined Every call there is allowed. It aborts in freeTopFromOldBlock when MEMT_DEBUG is defined, and otherwise sets the position of one block to an address inside another, after which getAllocatedMemoryUse() wraps around to a huge value. Deallocate the old front block when it is empty. This is done in growCapacity rather than in MemoryBlocks, as BufferPool shares the latter and has no such requirement. Co-Authored-By: Claude Opus 5 --- src/memtailor/Arena.cpp | 5 +++++ src/test/ArenaTest.cpp | 16 ++++++++++++++++ 2 files changed, 21 insertions(+) diff --git a/src/memtailor/Arena.cpp b/src/memtailor/Arena.cpp index e9549be..940f674 100644 --- a/src/memtailor/Arena.cpp +++ b/src/memtailor/Arena.cpp @@ -52,6 +52,11 @@ namespace memt { MEMT_ASSERT(size >= needed); MEMT_ASSERT(size % MemoryAlignment == 0); _blocks.allocBlock(size); + + // guardPoint() and freeTopFromOldBlock() both require that a block + // behind the front block is not empty. + if (block().hasPreviousBlock() && block().previousBlock()->empty()) + _blocks.freePreviousBlock(); } void Arena::freeTopFromOldBlock(void* ptr) { diff --git a/src/test/ArenaTest.cpp b/src/test/ArenaTest.cpp index 6f0c76d..f4a9926 100644 --- a/src/test/ArenaTest.cpp +++ b/src/test/ArenaTest.cpp @@ -335,3 +335,19 @@ TEST(Arena, GuardFreesNewerBlocks) { ASSERT_EQ(arena.debugAllocCount(), 0u); #endif } + +TEST(Arena, EmptyBlockIsNotKeptBehindFrontBlock) { + memt::Arena arena; + void* p = arena.alloc(1); + void* q = arena.alloc(arena.getMemoryUse() + 1); + arena.freeTop(q); + void* r = arena.alloc(arena.getMemoryUse() + 1); + arena.freeTop(r); + + arena.freeTop(p); // p is in the first block + ASSERT_TRUE(arena.isEmpty()); + ASSERT_EQ(arena.getAllocatedMemoryUse(), 0u); +#ifdef MEMT_DEBUG + ASSERT_EQ(arena.debugAllocCount(), 0u); +#endif +}