Fix locking and lifetime races in CLFUS compress_entries#13382
Open
phongn wants to merge 1 commit into
Open
Conversation
Collaborator
Author
|
Tracking issue: #13381 |
The fastlz minimum-length check ran in the region where the stripe mutex is dropped and jumped to Lfailed, writing to the entry and advancing the compression cursor without the lock and skipping the relock, so the rest of the pass ran unlocked. The compression-failure check similarly ran before revalidating that the entry survived the unlocked compression, so a failure could mark a freed entry; it now runs after revalidation. Also guard against the compression cursor having been invalidated while the lock was dropped, which dereferenced null. Observable behavior for live entries is unchanged: undersized or uncompressible entries are marked incompressible and skipped. The races are only observable under concurrency, so the added test pins the single-threaded behavior of the seam; the class declaration moves into RamCacheCLFUS.h so the test can drive compress_entries() directly. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
1932b21 to
328e026
Compare
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
RamCacheCLFUS::compress_entries()drops the stripe mutex around the compression call and retakes it afterward. Three bugs sat on that lock seam:Lfailed, which writes the entry's flags, advances the compression cursor, and continues the loop — all without the lock, and skipping the relock. If another thread takes the stripe lock during that window, the pass's final unlock tripsink_assert(t == m->thread_holding)in debug builds and releases the other thread's lock in release builds. Reachable withcompress = 1and any resident entry under 16 bytes.incompressibleon an entry a concurrent_destroyhad already freed and possibly recycled.e = this->_compressedcan be null and the loop tail dereferences it.Changes
defaultsets the failure flag instead of jumping (unreachable today, but no unlocked jump remains).After this patch the invariant is simple to audit: every write to the entry and every
Lfailed/Lcontinuejump happens with the stripe mutex held; the unlocked region contains only the pure compression call on snapshotted data.Testing
New Catch2 test (
test_RamCacheCompressEntries) drivescompress_entries()synchronously and pins the seam's behavior: undersized payloads are marked incompressible under the lock, skipped on subsequent passes, and remain readable; eligible payloads still compress and round-trip. The races themselves are only observable under concurrency (unlocking an unheldProxyMutexis a silent no-op), which is also why they went unnoticed — so this pins behavior through the restructure rather than reproducing the crashes.To make the class testable, its declaration moves verbatim into
RamCacheCLFUS.h. This mirrors the extraction already reviewed in #13257; whichever lands second rebases trivially, and the test will be folded into the shared compression test suite in the follow-up refactor.Notes for reviewers