From be8d624f2c36c07526736324bb33b9ebbd60c504 Mon Sep 17 00:00:00 2001 From: Mike Date: Mon, 31 Aug 2026 02:31:05 -0700 Subject: [PATCH] Fix mixed-prefix drain in reuse_storage_immediate The oldest-entry drain loop applied the offsets-flavor PendingRelease::release() to every entry it drained, but the allocated/offsets outputs are only meaningful for old_inst's own redistrict entry. A ready follower swept up by the drain misbehaved: - a plain destroy (empty redistrict_tags) trips the assert(!redistrict_tags.empty()) in debug builds; in release builds split_range returns 0 and clobbers 'allocated', mis-notifying old_inst's already-placed children as ALLOC_EVENTUAL_FAILURE while their tags stay live in current_allocator (leaked ranges) - a redistrict follower with more children than old_inst overflows the offsets vector inside split_range (out-of-bounds write) Apply the offsets-flavor release only to old_inst's own entry and drain followers with the void flavor, matching the equivalent drain in release_storage_immediate. Adds two DeferredAllocBadPathTest units covering both follower shapes; on the unfixed code they reproduce the two assertion failures exactly. Adjudicated as BUG-7 in tla/allocation/bugs/BUG-7.md (checked in separately with the deferred-allocation verification campaign). Co-Authored-By: Claude Fable 5 --- src/realm/mem_impl.cc | 13 +- tests/unit_tests/deferred_alloc_test.cc | 182 ++++++++++++++++++++++++ 2 files changed, 194 insertions(+), 1 deletion(-) diff --git a/src/realm/mem_impl.cc b/src/realm/mem_impl.cc index 4707f2c0aa..539e25e96d 100644 --- a/src/realm/mem_impl.cc +++ b/src/realm/mem_impl.cc @@ -1369,7 +1369,18 @@ namespace Realm { // catch up the current state do { - allocated = it->release(current_allocator, offsets); + if(it->inst == old_inst) { + // only old_inst's own entry uses the offsets flavor - + // 'allocated'/'offsets' describe old_inst's new instances and + // must not be clobbered by drained followers + allocated = it->release(current_allocator, offsets); + } else { + // ready followers may be plain destroys or redistricts with a + // different child count - the void flavor dispatches correctly + // on both (and a redistrict follower's children were already + // notified with these same offsets when it was marked ready) + it->release(current_allocator); + } // any prior bad-path entry whose tag is now finally being removed // from current_allocator can have its dealloc notification fired if((it->inst != old_inst) && it->deferred_dealloc_notify) diff --git a/tests/unit_tests/deferred_alloc_test.cc b/tests/unit_tests/deferred_alloc_test.cc index 48d05d348e..4cfeca786e 100644 --- a/tests/unit_tests/deferred_alloc_test.cc +++ b/tests/unit_tests/deferred_alloc_test.cc @@ -273,6 +273,188 @@ TEST_F(DeferredAllocBadPathTest, ReuseStorageDeferrableReorderFailDefersRecycle) EXPECT_TRUE(slot_on_free_list(A)); } +// Regression test for the oldest-path catch-up drain in +// reuse_storage_immediate sweeping a *plain* ready follower. The drain +// walks the ready prefix of pending_releases, but only the first entry is +// old_inst's redistrict - a ready follower can be a plain destroy queued +// by release_storage_deferrable's reorder-fail pushback. The drain must +// apply the offsets-flavor release (which fills the caller's +// 'allocated'/'offsets' for old_inst's children) only to old_inst's own +// entry and the void flavor to followers; applying the offsets flavor to a +// plain follower trips assert(!redistrict_tags.empty()) in debug builds +// and, in release builds, clobbers 'allocated' so old_inst's children are +// mis-notified ALLOC_EVENTUAL_FAILURE while their tags stay in +// current_allocator. +// +// Layout: P=50 at [0,50), A=30 at [50,80), C=20 at [80,100) -> full. +// 1. redistrict P -> {P1=10} deferred on an untriggered precondition. +// 2. request D=40 - fits the future hole [10,50) -> ALLOC_DEFERRED. +// 3. destroy A with a triggered precondition - reorder can't fit D=40 in +// the 30-unit hole, so A's entry is queued ready behind P's. +// 4. fire P's precondition - reuse_storage_immediate's oldest path drains +// P's redistrict (iteration 1), satisfies D, then sweeps A's plain +// ready entry (iteration 2). +TEST_F(DeferredAllocBadPathTest, ReuseOldestDrainSweepsPlainFollower) +{ + RegionInstanceImpl *P = make_inst(50); + RegionInstanceImpl *A = make_inst(30); + RegionInstanceImpl *C = make_inst(20); + RegionInstanceImpl *D = make_inst(40); + RegionInstanceImpl *P1 = make_inst(10); + + ASSERT_EQ(MemoryImpl::ALLOC_INSTANT_SUCCESS, + mem_->allocate_storage_deferrable(P, false, Event::NO_EVENT)); + ASSERT_EQ(MemoryImpl::ALLOC_INSTANT_SUCCESS, + mem_->allocate_storage_deferrable(A, false, Event::NO_EVENT)); + ASSERT_EQ(MemoryImpl::ALLOC_INSTANT_SUCCESS, + mem_->allocate_storage_deferrable(C, false, Event::NO_EVENT)); + + // Step 1: deferred redistrict of P -> {P1}. + Event p_precondition = GenEventImpl::create_genevent()->current_event(); + std::vector p_children = {P1}; + ASSERT_EQ(MemoryImpl::ALLOC_DEFERRED, + mem_->reuse_storage_deferrable(P, p_children, p_precondition)); + ASSERT_EQ(1u, mem_->pending_releases.size()); + ASSERT_FALSE(mem_->pending_releases.front().is_ready); + + // Step 2: D=40 fits only the future hole [10,50) left by P's split. + ASSERT_EQ(MemoryImpl::ALLOC_DEFERRED, + mem_->allocate_storage_deferrable(D, false, Event::NO_EVENT)); + ASSERT_EQ(1u, mem_->pending_allocs.size()); + + // Step 3: triggered destroy of A - attempt_release_reordering can't fit + // D=40 in the 30-unit hole, so A lands ready behind P's entry. + mem_->release_storage_deferrable(A, Event::NO_EVENT); + ASSERT_EQ(2u, mem_->pending_releases.size()); + { + const LocalManagedMemory::PendingRelease &a_entry = mem_->pending_releases.back(); + ASSERT_EQ(A, a_entry.inst); + ASSERT_TRUE(a_entry.is_ready); + ASSERT_TRUE(a_entry.deferred_dealloc_notify); + ASSERT_TRUE(a_entry.redistrict_tags.empty()); // plain destroy + } + + // Step 4: fire P's precondition - the oldest-path drain must sweep both + // entries without misapplying the offsets flavor to A's plain entry. + GenEventImpl::trigger(p_precondition, /*poisoned=*/false); + + EXPECT_TRUE(mem_->pending_releases.empty()); + EXPECT_TRUE(mem_->pending_allocs.empty()); + + // P1 keeps its own successful placement at offset 0 - a clobbered + // 'allocated' would have failure-notified it instead. + EXPECT_EQ(static_cast(0), P1->metadata.inst_offset); + EXPECT_EQ(static_cast(10), D->metadata.inst_offset); + + // Both parents' slots recycle exactly once each. + EXPECT_TRUE(slot_on_free_list(P)); + EXPECT_TRUE(slot_on_free_list(A)); + + // current_allocator agrees: P1 and D live at their notified offsets, P + // and A are gone, and A's 30-unit range is genuinely free again. + size_t first, size; + EXPECT_TRUE(mem_->current_allocator.lookup(P1->me, first, size)); + EXPECT_EQ(0u, first); + EXPECT_TRUE(mem_->current_allocator.lookup(D->me, first, size)); + EXPECT_EQ(10u, first); + EXPECT_FALSE(mem_->current_allocator.lookup(P->me, first, size)); + EXPECT_FALSE(mem_->current_allocator.lookup(A->me, first, size)); + + RegionInstanceImpl *E = make_inst(30); + ASSERT_EQ(MemoryImpl::ALLOC_INSTANT_SUCCESS, + mem_->allocate_storage_deferrable(E, false, Event::NO_EVENT)); + EXPECT_EQ(static_cast(50), E->metadata.inst_offset); +} + +// Redistrict-follower variant: the ready follower is *another redistrict* +// with more children than old_inst. The offsets flavor asserts +// offsets.size() == redistrict_tags.size() in debug builds; in release +// builds the follower's split_range writes child offsets past the end of +// the caller's offsets vector (sized for old_inst's children) - an +// out-of-bounds write - and 'allocated'/'offsets' then describe the +// follower's children rather than old_inst's. +// +// Layout: P=50 at [0,50), Q=30 at [50,80), C=20 at [80,100) -> full. +// 1. redistrict P -> {P1=10} deferred on an untriggered precondition. +// 2. request D=40 -> ALLOC_DEFERRED against the future hole [10,50). +// 3. triggered redistrict Q -> {Q1,Q2,Q3} (10 each): the reorder can't +// fit D, so Q's entry is queued ready behind P's with three +// redistrict tags (vs P's one). +// 4. fire P's precondition - the drain sweeps Q's entry with the void +// flavor, which sizes its own offsets vector. +TEST_F(DeferredAllocBadPathTest, ReuseOldestDrainSweepsRedistrictFollower) +{ + RegionInstanceImpl *P = make_inst(50); + RegionInstanceImpl *Q = make_inst(30); + RegionInstanceImpl *C = make_inst(20); + RegionInstanceImpl *D = make_inst(40); + RegionInstanceImpl *P1 = make_inst(10); + RegionInstanceImpl *Q1 = make_inst(10); + RegionInstanceImpl *Q2 = make_inst(10); + RegionInstanceImpl *Q3 = make_inst(10); + + ASSERT_EQ(MemoryImpl::ALLOC_INSTANT_SUCCESS, + mem_->allocate_storage_deferrable(P, false, Event::NO_EVENT)); + ASSERT_EQ(MemoryImpl::ALLOC_INSTANT_SUCCESS, + mem_->allocate_storage_deferrable(Q, false, Event::NO_EVENT)); + ASSERT_EQ(MemoryImpl::ALLOC_INSTANT_SUCCESS, + mem_->allocate_storage_deferrable(C, false, Event::NO_EVENT)); + + Event p_precondition = GenEventImpl::create_genevent()->current_event(); + std::vector p_children = {P1}; + ASSERT_EQ(MemoryImpl::ALLOC_DEFERRED, + mem_->reuse_storage_deferrable(P, p_children, p_precondition)); + + ASSERT_EQ(MemoryImpl::ALLOC_DEFERRED, + mem_->allocate_storage_deferrable(D, false, Event::NO_EVENT)); + + // Triggered redistrict of Q -> {Q1, Q2, Q3}. The children exactly fill + // Q's [50,80) range, so the reorder still can't fit D=40 and Q's entry + // is queued ready with three redistrict tags. The children are notified + // INSTANT_SUCCESS right here, with offsets carved from Q's own interval. + std::vector q_children = {Q1, Q2, Q3}; + ASSERT_EQ(MemoryImpl::ALLOC_INSTANT_SUCCESS, + mem_->reuse_storage_deferrable(Q, q_children, Event::NO_EVENT)); + ASSERT_EQ(2u, mem_->pending_releases.size()); + { + const LocalManagedMemory::PendingRelease &q_entry = mem_->pending_releases.back(); + ASSERT_EQ(Q, q_entry.inst); + ASSERT_TRUE(q_entry.is_ready); + ASSERT_TRUE(q_entry.deferred_dealloc_notify); + ASSERT_EQ(3u, q_entry.redistrict_tags.size()); + } + EXPECT_EQ(static_cast(50), Q1->metadata.inst_offset); + EXPECT_EQ(static_cast(60), Q2->metadata.inst_offset); + EXPECT_EQ(static_cast(70), Q3->metadata.inst_offset); + + GenEventImpl::trigger(p_precondition, /*poisoned=*/false); + + EXPECT_TRUE(mem_->pending_releases.empty()); + EXPECT_TRUE(mem_->pending_allocs.empty()); + + EXPECT_EQ(static_cast(0), P1->metadata.inst_offset); + EXPECT_EQ(static_cast(10), D->metadata.inst_offset); + + // Q's children materialize in current_allocator at exactly the offsets + // they were promised when Q's entry was marked ready - child offsets are + // intrinsic to the parent's interval, which never moves. + size_t first, size; + EXPECT_TRUE(mem_->current_allocator.lookup(Q1->me, first, size)); + EXPECT_EQ(50u, first); + EXPECT_TRUE(mem_->current_allocator.lookup(Q2->me, first, size)); + EXPECT_EQ(60u, first); + EXPECT_TRUE(mem_->current_allocator.lookup(Q3->me, first, size)); + EXPECT_EQ(70u, first); + EXPECT_FALSE(mem_->current_allocator.lookup(P->me, first, size)); + EXPECT_FALSE(mem_->current_allocator.lookup(Q->me, first, size)); + + EXPECT_TRUE(slot_on_free_list(P)); + EXPECT_TRUE(slot_on_free_list(Q)); + + // The heap is exactly full again: P1+D+Q1+Q2+Q3+C = 100. + EXPECT_FALSE(mem_->current_allocator.can_allocate(D->me, 1, 1)); +} + // Exercises the bad-path branch in release_storage_immediate: a destroy is // queued with a deferred precondition (lands non-oldest in pending_releases), // then a pending alloc is queued, then the destroy's precondition fires.