Repository navigation
Fix memory pool corruption after allocation failure in FreeObjects::newBlock() - #9250
Open
gabrieleciech wants to merge 2 commits into
Open
gabrieleciech wants to merge 2 commits into
gabrieleciech wants to merge 2 commits into
Conversation
…allocation failure When newExtent() throws (e.g. std::bad_alloc), the current extent has already been retired with decrUsage() but currentExtent still points to it, so the next call retires it again. Its usage count drops below the number of live blocks and the extent (and the medium hunk holding it) can be reclaimed while still in use, leading to crashes in unlinkBlocks()/SemiDoubleLink::remove(). Allocate the new extent first and retire the old one only after success. Add a unit test that forces the allocation failure via RLIMIT_AS.
Member
|
I see no problems with you fix - just wait for started now CI. |
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
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.
FreeObjects::newBlock()retires the current extent (decrUsage()) before callingpool->newExtent(). IfnewExtent()throws,currentExtentstill points to the retired extentand the next call retires it again, so every allocation failure drops one more usage reference.
The medium hunk's
useCountgoes below the number of live blocks; when it reaches zero the hunkis reclaimed while still in use, and
unlinkBlocks()treats live blocks as free list elements(SIGSEGV in
SemiDoubleLink::remove(), writes into unrelated memory, or use-after-free).Change
newExtent()throws, the old extentkeeps its usage reference and is retired by the next successful call.
!currentExtentis now handled in the same branch, so the separate "no current extent" blockis gone; behaviour on success is unchanged.
Test
New Boost test
CommonSuite/AllocSuite/AllocTests/MediumExtentAllocationFailure(
src/common/classes/tests/AllocTest.cpp, Linux only): it makes two allocations of the nextmedium extent fail by lowering
RLIMIT_AS, then allocates and frees normally.fb_assert(e->prev)in Debugon 3.0.14;
common_testpasses.Observed in production
Firebird 3.0.14.33856, Linux x64, Classic Server. After clients received "unable to allocate
memory from operating system", many Classic processes crashed within a few seconds of each other
during attachment cleanup (
purge_attachment→MET_clear_cache→MemoryPool::deletePool),mostly at
SemiDoubleLink::remove()with a write to NULL. In the core dumps the reclaimed mediumhunk of the attachment pool has
useCount == 0while still holding a live block; in another casea statement pool faults in
~FreeObjects()because its current extent lies in a medium hunk ofthe (still alive) attachment pool that had already been unmapped.
Notes
The same code is in
v4.0-releaseandv5.0-release(and 3.0)