Fix heap corruption in FTS query path when scans race committing writers - #845
Merged
Conversation
Fixes three distinct memory-safety bugs hit by concurrent QUERY_FTS_INDEX scans, committing writers, and periodic CHECKPOINTs (reported in #840, heap corruption manifesting as "malloc(): invalid next size (unsorted)" at arbitrary allocation sites, e.g. SparseFrontier::addNode under fts tableFunc): 1. DenseFrontier out-of-bounds write (confirmed by ASAN): DenseFrontier buffers are sized from getMaxOffsetMap() -> NodeTable::getNumTotalRows(), which reads the current committed row count and is not snapshot-isolated. The parallel init vertex compute re-fetches a fresh max offset; a concurrent commit that grows the table between the two reads makes the morsel range exceed the allocated frontier, and DenseFrontierInitVertexCompute writes 2 bytes past the end of the atomic<iteration_t> array. Clamp the init ranges to the allocated size. 2. Checkpoint write gate released before the storage phase: TransactionManager::checkpointNoLock released the write gate after WAL rotation but before checkpointStoragePhase(), letting new write transactions mutate persistent chunks' UpdateInfo (e.g. the FTS terms-table df update on every insert) while the checkpointer was concurrently scanning and resetVersionAndUpdateInfo()-ing those same chunks. Writers then committed or rolled back against a wiped UpdateInfo (InternalException in getUpdateNode, null-UpdateNode SEGV in rollback, and heap corruption in release builds). Hold the write gate for the entire checkpoint. 3. Unsynchronized unordered_map mutations in the GDS frontier: SparseFrontier / SparseFrontierReference::addNode mutate the per-table map with no lock while parallel frontier tasks can reach the sparse write path; serialize them with a mutex inside GDSSpareObjectManager. Dense frontiers keep lock-free atomic stores. Also fixes the vendored zstd ASan poison declarations to have C linkage, which is required now that the zstd sources are compiled as C++ (otherwise ASan builds fail to load liblbug with an undefined mangled __asan_unpoison_memory_region symbol). Bumps the extensions submodule to include the matching FTS fix that serializes QFTSEdgeCompute's shared scores map. Remaining known issue (needs a design follow-up): concurrent read-transaction scans can still race the checkpointer's storage phase when it rewrites/reclaims node-group chunk structures that in-flight scans reference (issue #840's original SIGSEGV family).
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.
Summary
Fixes three memory-safety bugs hit when concurrent
QUERY_FTS_INDEXscans race committing writers and periodicCHECKPOINTs (investigated via the repro from #840 — heap corruption manifesting asmalloc(): invalid next size (unsorted)at arbitrary allocation sites, e.g.SparseFrontier::addNodeunder the FTStableFunc).All three were confirmed with an ASAN build (
BM_MALLOCbuffer manager) running the repro workload (3 scanner threads + writer + checkpointer on an on-disk DB with an FTS index):1.
DenseFrontierout-of-bounds writeASAN:
heap-buffer-overflow WRITE of size 2inDenseFrontier::addNodefromDenseFrontierInitVertexCompute.DenseFrontierbuffers are sized fromgetMaxOffsetMap()→NodeTable::getNumTotalRows(), which returns the current committed row count (NodeGroupCollection::numTotalRows) and is not snapshot-isolated. The parallel init vertex compute re-fetches a fresh max offset; a concurrent commit (the FTS index appends to the docs/terms tables on everyCREATE) grows the table between the two reads, so the morsel range exceeds the allocation and the init writes past the end of theatomic<iteration_t>array.Fix: clamp the init/visited ranges to the frontier's allocated size.
2. Checkpoint write gate released before the storage phase
ASAN:
InternalExceptionfromUpdateInfo::getUpdateNodeduring writer commit, and a null-UpdateNodederef inUpdateInfo::rollback(updates[vectorIdx]wiped) — heap corruption in release builds.TransactionManager::checkpointNoLockreleased the write gate after WAL rotation but beforecheckpointStoragePhase(). New write transactions could then mutate persistent chunks'UpdateInfo(the FTS insert path updates the terms-tabledfcolumn) while the checkpointer was concurrently scanning andresetVersionAndUpdateInfo()-ing those same chunks.Fix: hold the write gate for the entire checkpoint. (This also removes the acknowledged hash-index snapshot limitation noted in the old comment, since writers can no longer start mid-checkpoint.)
3. Unsynchronized
unordered_mapmutations in the GDS frontierSparseFrontier/SparseFrontierReference::addNodemutate the per-table map with no lock while parallel frontier tasks can reach the sparse write path; concurrent rehash corrupts the heap. Fix: serialize via a mutex inGDSSpareObjectManager(shared between a sparse frontier and its references). Dense frontiers keep lock-free atomic stores.Build fix
The vendored zstd declares the ASan poison functions without
extern "C"; compiled as C++ it emits an unresolvable mangled__asan_unpoison_memory_regionsymbol, so ASan builds ofliblbug.sofail todlopen. Add proper C-linkage guards.Extensions submodule
Bumped to include LadybugDB/extensions#69 —
QFTSEdgeComputecopies (one per worker thread) share thescoresmap by reference and mutated it without synchronization; now guarded by a shared mutex mirroring the existingMatchTermsVertexCompute::resDfsMutexpattern.Testing
UpdateInfocorruption no longer reproduce; the original release-build crash signature (malloc(): invalid next size (unsorted)insideSparseFrontier::addNode) is resolved.NodeTableScanState::scanNextSIGSEGV family). Repro crash rate on release builds dropped from ~always to ~1/3 with these fixes; the residual crashes are this remaining race.TODO before merge: run
make testandmake extension-test.