Hardening region allocator#9224
Open
johningve wants to merge 3 commits into
Open
Conversation
split_block_region derived the kept region's size by subtracting the conformed remainder from the original free region, which assumes conforming the remainder leaves it unchanged. That assumption doesn't hold: the remainder is an arbitrary leftover that was never conformed as its own buffer, and Vulkan's conform sizes it from vkGetBufferMemoryRequirements (then rounds up to nearest_multiple), which may pad it beyond the arithmetic leftover — even when the incoming request was fully conformed. When that happens the subtraction sizes the kept region smaller than the request, and the VkBuffer later created for it is undersized. Size the allocated region directly from the remainder's conformed offset instead, so conform-induced padding of the remainder can no longer shrink it. Add asserts that the two regions stay within the original free region, turning the remaining unhandled case — a conformed remainder overrunning into the next region — into a loud failure rather than silent overlap. Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
RegionAllocator caches each region's VkBuffer (memory.handle) across free/reuse, on the implicit invariant that a cached handle's size equals the region's allocation size. coalesce_block_regions broke that invariant: when it merged a collected region into an available previous neighbour it grew prev_region's allocation.size but left prev_region's cached handle untouched, still sized to the old (smaller) region. A later reservation landing on the coalesced region with an exact conformed-size match takes the handle-reuse path in alloc_block_region, which reuses the cached handle with no size check, and is handed a VkBuffer smaller than the region. Deallocate the previous neighbour's handle before growing it, so the merged region falls back to a fresh, correctly-sized allocation on next use. Extract the repeated "deallocate the handle if the region is unused" guard into deallocate_block_region_if_unused and use it at the three existing sites (coalesce, split, free) plus the new prev-merge site; free_block_region now layers its status/usage reset on top of the helper. Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
RegionAllocator caches each region's handle across release/reserve and reuses it in alloc_block_region with no size check, on the invariant that a cached handle's backing allocation always matches the region's allocation.size. The test mocks couldn't see a violation: the mock handle was the constant sentinel (void *)1, carrying no size information. Encode the allocation size in the mock handle instead, and check it at every point the runtime hands a region across the callback boundary: on entry to allocate_region (a fresh allocation must never clobber a live handle), in deallocate_region (the handle must still match the region's size at death), and after every reserve() in the test cases — including the stress-test loops, where cached handles are actually re-issued under churn. The nullptr checks previously done at each reserve() site are folded into the new check_region helper. Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
alexreinking
approved these changes
Jul 20, 2026
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #9224 +/- ##
=======================================
Coverage ? 70.19%
=======================================
Files ? 255
Lines ? 78928
Branches ? 18879
=======================================
Hits ? 55407
Misses ? 17893
Partials ? 5628 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR hardens the region allocator against two ways a region's cached buffer handle (
memory.handle) could go stale relative to the region'sallocation.size.The block_allocator test is updated to always check the size of the buffer (encoded in handle) vs the size of the allocation, ensuring these are kept in sync by the allocator.
I have not been able to observe any failure due to these weaknesses in the code. They were found by Claude Fable 5 while debugging issues seen in #9223. Hence, there is no regression test either in this PR. The updated test passes also on main, but it improves the test to be able to detect invariant violations in the future.
Checklist