Skip to content

Fix locking and lifetime races in CLFUS compress_entries#13382

Open
phongn wants to merge 1 commit into
apache:masterfrom
phongn:fix-clfus-compress-entries-races
Open

Fix locking and lifetime races in CLFUS compress_entries#13382
phongn wants to merge 1 commit into
apache:masterfrom
phongn:fix-clfus-compress-entries-races

Conversation

@phongn

@phongn phongn commented Jul 14, 2026

Copy link
Copy Markdown
Collaborator

Summary

RamCacheCLFUS::compress_entries() drops the stripe mutex around the compression call and retakes it afterward. Three bugs sat on that lock seam:

  1. Unlocked pass continuation. The fastlz minimum-length check ran in the unlocked region and jumped to 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 trips ink_assert(t == m->thread_holding) in debug builds and releases the other thread's lock in release builds. Reachable with compress = 1 and any resident entry under 16 bytes.
  2. Write after free. The compression-failure check ran before revalidating that the entry survived the unlocked window, so a failed compression could mark incompressible on an entry a concurrent _destroy had already freed and possibly recycled.
  3. Null cursor dereference. When revalidation fails and the cursor was concurrently invalidated (e.g. by put-side victimization), e = this->_compressed can be null and the loop tail dereferences it.

Changes

  • The fastlz minimum-length decision moves into the locked bound phase (same outcome: the entry is marked incompressible and skipped).
  • The unlocked codec switch's default sets the failure flag instead of jumping (unreachable today, but no unlocked jump remains).
  • Failure marking runs only after the entry has been revalidated under the lock.
  • A null cursor after failed revalidation ends the pass instead of dereferencing.

After this patch the invariant is simple to audit: every write to the entry and every Lfailed/Lcontinue jump happens with the stripe mutex held; the unlocked region contains only the pure compression call on snapshotted data.

Testing

New Catch2 test (test_RamCacheCompressEntries) drives compress_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 unheld ProxyMutex is 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

  • Observable behavior for live entries is unchanged: undersized or uncompressible entries are marked incompressible and the cursor advances, exactly as before.
  • Backport candidate: the same code ships in all release branches.

@phongn

phongn commented Jul 14, 2026

Copy link
Copy Markdown
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>
@phongn phongn force-pushed the fix-clfus-compress-entries-races branch from 1932b21 to 328e026 Compare July 14, 2026 19:15
@JosiahWI JosiahWI added the Cache label Jul 14, 2026
@JosiahWI JosiahWI added this to the 11.0.0 milestone Jul 14, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants