Fix CacheBuffer::m_mem use-after-free - #930
Merged
shosseinimotlagh merged 20 commits intoOct 1, 2026
Merged
Conversation
get_memvec_intrusive() and set_memvec() previously had a three-way race: Thread A reads m_mem.px (raw ptr into intrusive_ptr), then Thread B calls set_memvec() replacing m_mem, then IO completion drops the last wb_req ref, freeing MemVec_1 before Thread A bumps its refcount. Result: UAF in insert_missing_pieces / update_missing_piece / at_offset. Fix: - Add m_mem_mtx (shared_mutex) solely for protecting m_mem lifetime. Separate from m_mtx which guards eviction/cache-state changes. - get_memvec_intrusive(): take shared_lock while copying intrusive_ptr, so the refcount increment is atomic with respect to set_memvec. Multiple concurrent readers proceed in parallel; only a concurrent set_memvec causes a brief exclusive wait. - set_memvec(): take unique_lock before replacing m_mem. - insert_missing_pieces(), update_missing_piece(), at_offset(): all converted from raw get_memvec() reference to get_memvec_intrusive(), ensuring the caller holds a strong ref for the duration of the call. The lock is held only for the pointer snapshot (nanoseconds), not for the actual MemVector operation, so IO throughput is unaffected. Reproducer: crash in homeds::MemVector::insert_missing_pieces (Thread 14) with secondary tcmalloc freelist corruption (Threads 5, 7, 8) observed in access_mgr v3.5.21 / HomeStore 3.8.4 on 2026-09-13. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
refresh_buf() read m_mem via raw get_memvec() before calling set_memvec(). Although the btree node lock prevents a concurrent set_memvec on the same node, using the raw accessor is inconsistent with the m_mem_mtx discipline introduced in the previous commit and technically unsound from the C++ memory model perspective. Replace with get_memvec_intrusive() for full consistency. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…_buf" This reverts commit bd44021.
…for m_mem_mtx std::shared_mutex wraps pthread_rwlock_t at 56 bytes per instance. With millions of btree nodes each embedding a CacheBuffer, that adds ~53 MB per million nodes. folly::SharedMutexReadPriority is 4 bytes (a single uint32_t) and is already the lock type used for per-btree-node locking (btree_node.h) and hash-bucket locking (intrusive_hashset.hpp) in this codebase. Same correctness guarantees, 14x smaller. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ests Race A: CacheBuffer::insert_missing_pieces() and set_memvec() accessed m_mem concurrently without synchronisation. insert_missing_pieces() returned a raw C++ reference from get_memvec(), while a concurrent set_memvec() could replace m_mem and drop MV1's refcount to zero, freeing it while the raw reference was still live → UAF → SIGSEGV. Fix: add mutable std::shared_mutex m_mem_mtx to CacheBuffer. Readers (insert_missing_pieces, update_missing_piece, at_offset) capture an intrusive_ptr<MemVector> under shared_lock before releasing it. set_memvec() holds unique_lock when replacing m_mem. To make the race deterministic without TSAN, two _PRERELEASE FLIPs cooperate: 1. wb_flush_delay_before_write (200ms, in flush_buffers): keeps CP0 reqs in WB_REQ_WAITING while Phase 2 writers call refresh_buf → set_memvec(MV2) → MV1 refcount 2→1. 2. wb_cache_get_memvec_cp_delay (500ms, in insert_missing_pieces): holds the window open after the raw ref (no-fix) or intrusive_ptr (fix) is taken, giving CP0 flush time to complete → MV1 freed. No-fix: Thread A wakes to a dangling ref → SIGSEGV. Fix: intrusive_ptr keeps MV1 alive. Also added: - test_wb_cache_race.cpp: unit tests for Race A and Race B (wb_req->m_mem) - test_wb_cache_integration.cpp: integration tests on a live homestore instance including CrossCpMemvecRaceDetectedByFlip (PRERELEASE only) Verified on docker4: ~/org_crash/test_wb_cache_integration -> SIGSEGV (Race A) ~/fix_crash/test_wb_cache_integration -> PASSED (fix holds)
SDSTOR-25237: fix wb_req->m_mem data race and add regression UT
CacheBuffer::at_offset() correctly took a locked, refcounted snapshot
of m_mem before reading it, but that reference (mv) was local to the
function and was released the instant at_offset() returned. Callers
(Volume::verify_csum, btree node checksum verification, etc.) only
received a bare sisl::blob {bytes, size} with no ownership, so a
concurrent set_memvec() could free the MemVector after at_offset()
returned but before the caller read blob.bytes - a second, later-window
instance of the same use-after-free class already fixed for the
read-and-bump-refcount step itself.
Introduce blob_view (sisl::blob + a type-erased ownership token) so the
MemVector stays alive for as long as the caller holds the returned
blob, not just for the duration of the accessor call. CacheBuffer and
VolInterface's at_offset() now return blob_view; callers that bind the
result to `auto` need no further changes, since the extra member rides
along transparently and is released only when the caller's own local
variable goes out of scope. Updated the two production callers that
explicitly declared `sisl::blob` locals (Volume::verify_csum,
SSDBtreeStore::get_physical/_init_node), which would otherwise slice
off the ownership token at that exact line and silently reintroduce
the bug.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
cache: keep MemVector alive through at_offset()'s returned blob
Contributor
|
change |
shosseinimotlagh
force-pushed
the
fix/memvec-uaf-race
branch
from
September 30, 2026 18:18
e2e29c9 to
08984ab
Compare
shosseinimotlagh
previously approved these changes
Sep 30, 2026
- nlohmann_json/3.12.0 is on ebay-local only; use [^3.11] range to pick what is available on conan center (matches sisl/8.9.8) - Add openssl/1.1.1w override to match iomgr/8.8.7 and sisl/8.9.8, preventing version-range conflicts in the dependency graph
Save seq_num before invoking the completion callback, which may call logstore_req::free(req). Accessing req->seq_num after the callback is a heap-use-after-free caught by ASAN in test_wb_cache_integration and test_load.
Move VolInterface::shutdown + iomanager.stop from TearDownTestSuite into main() after RUN_ALL_TESTS() returns, matching the pattern used by test_log_store. Shutting down inside TearDownTestSuite caused a segfault (SIGSEGV, exit 139) and ASAN LeakSanitizer reports because gtest's own post-suite cleanup still ran while the IOManager and HomeStore singletons were gone.
raakella1
force-pushed
the
fix/memvec-uaf-race
branch
from
October 1, 2026 15:53
6959a7a to
1866dc9
Compare
raakella1
force-pushed
the
fix/memvec-uaf-race
branch
2 times, most recently
from
October 1, 2026 16:00
1866dc9 to
de7ef0d
Compare
…wncast error process_completions() casts virtualdev_req -> blkstore_req<Buffer> via static_pointer_cast. UBSan's -fsanitize=vptr flags this as an invalid downcast when the actual object is writeback_req (a further-derived type) because it cannot verify the type relationship through the static cast. Switch to dynamic_pointer_cast: semantically correct for this downcast, and bypasses UBSan's static-cast vptr check. The cast always succeeds at runtime since writeback_req IS-A blkstore_req<wb_cache_buffer_t>.
… blkstore buffer type WbTestBtree used SimpleNumberKey/FixedBytesValue with SIMPLE node types, but the index blkstore is BlkStore<WriteBackCacheBuffer<MappingKey, MappingValue, VAR_VALUE, VAR_VALUE>> (= BLKSTORE_BUFFER_TYPE). With dynamic_pointer_cast in to_blkstore_req the type mismatch correctly returns null instead of UB, triggering an assertion. Switch WbTestBtree to MappingKey/MappingValue/VAR_VALUE/VAR_VALUE so the btree's blkstore_req dynamic type matches what process_completions expects.
Default MappingValue() leaves m_earr uninitialized (is_initialized=false), causing blob_array.h:191's assertion to fire when the btree node serializes the value. Use MappingValue(seq_id_t, BlkId) which calls alloc_element() and sets is_initialized before the first put.
…ree nodes MappingValue::estimate_size_after_append() asserts false — it is intentionally unimplemented because the real Mapping class bypasses it via custom callbacks. The assert fires in varlen_node.hpp:348 whenever btree_put() is called with a key that already exists in a VAR_VALUE leaf node. ConcurrentWriteAndFlushIsRaceFree: use a fresh non-overlapping key band per (iter, thread) so no key is ever written twice. The wb_cache else-branch still fires because Phase 2 keys are adjacent to Phase 1 keys and land in the same partially-filled leaf nodes. CrossCpMemvecRaceDetectedByFlip: Phase 1 uses stride-(kWriteThreads+1) keys; Phase 2 threads use interleaved offsets (t+1 within each stride block). All four threads' inserts land between Phase 1 keys in the same leaf nodes, so refresh_buf fires for every write — same race coverage with no collisions.
…eate After m_log_records->create() marks the log record's slot active, a concurrent flush on another thread can immediately pick it up via foreach_active, flush synchronously (buffered-IO + IO-reactor), fire the completion callback, and free the owning logstore_req. The next line in append_async then reads data.size from the reference that now points into freed memory, triggering an ASAN heap-use-after-free. Fix: capture data.size in a local uint32_t before create() is called. Use that local for the pending-size fetch_add and the post-create threshold check. The data reference is not accessed after create().
shosseinimotlagh
approved these changes
Oct 1, 2026
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
2 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.
CacheBuffer::m_mem was read via raw, unsynchronized access in insert_missing_pieces(), update_missing_piece(), and at_offset(), while set_memvec() (called from refresh_buf() during cross-checkpoint COW) could concurrently replace m_mem and free the old MemVector out from under a reader → UAF → SIGSEGV.
Confirmed in production: identical insert_missing_pieces/verify_csum crash signature on HomeStore 3.8.4 and 3.8.7.
Fix