From d60aacfe4d4eb336f61854f067a04f26643a7918 Mon Sep 17 00:00:00 2001 From: Sebastian Blessing Date: Mon, 7 Sep 2026 17:49:25 +0200 Subject: [PATCH] Read the batched remote free ring's length word without domesticating it BatchedRemoteMessage::mk_from_freelist_builder() closes the free ring by storing a bit-packed (displacement, length) word, not a pointer, in the ring's next field. open_free_ring() and ring_size() read it back through freelist::Object::T::read_next(), which passes the decoded value through the config's domesticator. A domesticator that rejects addresses outside the heap (FixedRangeConfig::capptr_domesticate() returns nullptr for them) turns the word into 0, so every incoming ring decodes as length 0 and every batched remote deallocation is leaked. With FixedRangeConfig the fixed region is exhausted by remote frees alone even though the program frees everything it allocates. Add freelist::Object::T::read_next_raw(), which undoes the free-list encoding but does not domesticate, and define read_next() in terms of it so the difference between the two is the single domestication call. Use read_next_raw() for the ring word; the pointer derived from its displacement is still domesticated separately, as before. Add the reproducer as func/fixed_region_remote_dealloc: one allocator allocates from a FixedRangeConfig region, another frees remotely and flushes, for more rounds than the region could survive if the frees were lost. It aborts on the unpatched tree and completes with this change. Fixes #876. --- src/snmalloc/mem/freelist.h | 27 ++++-- src/snmalloc/mem/remoteallocator.h | 12 ++- .../fixed_region_remote_dealloc.cc | 87 +++++++++++++++++++ 3 files changed, 119 insertions(+), 7 deletions(-) create mode 100644 src/test/func/fixed_region_remote_dealloc/fixed_region_remote_dealloc.cc diff --git a/src/snmalloc/mem/freelist.h b/src/snmalloc/mem/freelist.h index db059c5c9..9eb8b02ed 100644 --- a/src/snmalloc/mem/freelist.h +++ b/src/snmalloc/mem/freelist.h @@ -218,6 +218,27 @@ namespace snmalloc return n_tame; } + /** + * Read the next field without domesticating it. + * + * This returns exactly the value that was stored, with only the + * free-list encoding undone: a (possibly Wild) pointer in the common + * case, or the opaque bit-packed word that BatchedRemoteMessage keeps + * in the next field of its free ring. Callers that want a pointer + * must domesticate the result (see read_next); callers reading an + * opaque word must not, as a domesticator may legitimately reject + * anything that is not an address within the heap. + */ + BQueuePtr + read_next_raw(const FreeListKey& key, address_t key_tweak) + { + return Object::decode_next( + address_cast(&this->next_object), + this->next_object, + key, + key_tweak); + } + /** * Read the next pointer */ @@ -228,11 +249,7 @@ namespace snmalloc BHeadPtr read_next( const FreeListKey& key, address_t key_tweak, Domesticator domesticate) { - return domesticate(Object::decode_next( - address_cast(&this->next_object), - this->next_object, - key, - key_tweak)); + return domesticate(read_next_raw(key, key_tweak)); } /** diff --git a/src/snmalloc/mem/remoteallocator.h b/src/snmalloc/mem/remoteallocator.h index 57d7c31e0..1b02381a9 100644 --- a/src/snmalloc/mem/remoteallocator.h +++ b/src/snmalloc/mem/remoteallocator.h @@ -112,8 +112,15 @@ namespace snmalloc address_t key_tweak, Domesticator_queue domesticate) { + /* + * The next field of the free ring holds the bit-packed (displacement, + * length) word written by mk_from_freelist_builder, not a pointer, so + * it must be read raw rather than through the domesticator (which may + * reject anything outside the heap, as FixedRangeConfig's does). The + * pointer derived from it below is domesticated separately. + */ uintptr_t encoded = - m->free_ring.read_next(key, key_tweak, domesticate).unsafe_uintptr(); + m->free_ring.read_next_raw(key, key_tweak).unsafe_uintptr(); uint16_t decoded_size = static_cast(encoded) & bits::mask_bits(MAX_CAPACITY_BITS); @@ -155,8 +162,9 @@ namespace snmalloc address_t key_tweak, Domesticator_queue domesticate) { + // See open_free_ring: this is a bit-packed word, not a pointer. uintptr_t encoded = - m->free_ring.read_next(key, key_tweak, domesticate).unsafe_uintptr(); + m->free_ring.read_next_raw(key, key_tweak).unsafe_uintptr(); uint16_t decoded_size = static_cast(encoded) & bits::mask_bits(MAX_CAPACITY_BITS); diff --git a/src/test/func/fixed_region_remote_dealloc/fixed_region_remote_dealloc.cc b/src/test/func/fixed_region_remote_dealloc/fixed_region_remote_dealloc.cc new file mode 100644 index 000000000..909ad4530 --- /dev/null +++ b/src/test/func/fixed_region_remote_dealloc/fixed_region_remote_dealloc.cc @@ -0,0 +1,87 @@ +#include "test/setup.h" + +#include +#include +#include + +#ifdef assert +# undef assert +#endif +#define assert please_use_SNMALLOC_ASSERT + +using namespace snmalloc; + +using CustomGlobals = FixedRangeConfig>; +using FixedAlloc = Allocator; + +/** + * Regression test for https://github.com/microsoft/snmalloc/issues/876. + * + * FixedRangeConfig has a range-checking capptr_domesticate, which returns + * nullptr for anything outside the fixed region. BatchedRemoteMessage stores + * a bit-packed (displacement, length) word, not a pointer, in the next field + * of its free ring; that word used to be passed through the domesticator when + * the receiving allocator opened the ring, so every ring decoded as length 0 + * and every batched remote deallocation was lost. + * + * Allocator A allocates small objects and allocator B frees them (remotely) + * and flushes. Everything allocated is freed, so the loop must be able to run + * well past the point at which the fixed region would be exhausted if the + * remote frees never made it back to A's slabs. + */ +int main() +{ + setup(); + + const size_t size = bits::one_at_bit(25); // 32 MiB region + auto base = DefaultPal::reserve(size); + DefaultPal::notify_using(base, size); + std::cout << "Allocated region " << base << " - " + << pointer_offset(base, size) << std::endl; + + CustomGlobals::init(nullptr, base, size); + + auto a = get_scoped_allocator(); + auto b = get_scoped_allocator(); + + constexpr size_t object_size = 64; + constexpr size_t batch = 64; + void* objects[batch]; + + // Enough rounds to exhaust the region twice over if the remote frees were + // being lost; the bug shows up well before the first exhaustion. + const size_t rounds = 2 * (size / (batch * object_size)); + + for (size_t round = 0; round < rounds; round++) + { + for (size_t i = 0; i < batch; i++) + { + objects[i] = a->alloc(object_size); + if (objects[i] == nullptr) + { + std::cout << "Allocator A returned nullptr in round " << round + << " after " << round * batch + << " remote frees, even though everything allocated so far " + "has been freed." + << std::endl; + abort(); + } + SNMALLOC_CHECK(snmalloc::is_owned(objects[i])); + } + + // B is not the owner of these objects, so these are remote deallocations + // that are batched into rings and pushed onto A's message queue by flush. + for (size_t i = 0; i < batch; i++) + b->dealloc(objects[i]); + b->flush(); + + if ((round % 1024) == 0) + std::cout << "." << std::flush; + } + + std::cout << std::endl + << "Completed " << rounds << " rounds of " << batch + << " remote frees of " << object_size << "-byte objects" + << std::endl; + return 0; +}