Fix UBSan downcast, logstore UAF, and CacheBuffer m_mem data races - #935
Closed
shosseinimotlagh wants to merge 16 commits into
Closed
shosseinimotlagh wants to merge 16 commits into
shosseinimotlagh wants to merge 16 commits into
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
- 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.
…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.
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.
Summary
This PR consolidates several correctness fixes for
stable/v3.8.x:1. blkstore: UBSan downcast fix (
cec0b674)to_blkstore_requsedstatic_pointer_cast, which is undefined behaviour when the dynamic type does not matchblkstore_req<Buffer>. Switched todynamic_pointer_castso mismatches returnnullptrcleanly instead of silently invoking UB that UBSan now catches with-fsanitize=undefined.2. logstore: fix use-after-free in
on_write_completion(3528e52f)Fixes a UAF where a callback captured
thisafter the owning object could be destroyed.3. cache: fix
CacheBuffer::m_memdata races (0e3f2f12–c4fb3865)flush_buffers()readwb_req->m_memvia a raw pointer while a concurrent CP1 writer calledrefresh_buf()→set_memvec()dropping the refcount to zero and freeing theMemVector. Fixed by taking anintrusive_ptrbefore dropping the lock.WriteBackCache::write()else-branch updatedwb_req->m_memoutsidewb_req->mtxwhileflush_buffers()read it. Fixed by takingwb_req->mtxin both paths.77341239):at_offset()returned a blob backed by aMemVectorraw pointer that could be freed mid-use. Fixed by keeping theMemVectoralive through the blob's lifetime.folly::SharedMutexReadPriorityreplacesstd::shared_mutexform_mem_mtx(4 bytes vs. 56 bytes).4. Integration test (
test_wb_cache_integration) (788aaf1e,630b8112)New GTest suite that exercises the Race B path on a live homestore instance (real SSD btree, real blkstore, real CP machinery). Runs with ASan+UBSan:
ConcurrentWriteAndFlushIsRaceFree: concurrent btree writes + CP flushes, 20 000 keys × 3 iterationsCrossCpMemvecRaceDetectedByFlip(#ifdef _PRERELEASE): uses FLIP injection to make Race A deterministic5. Dependency updates (
8e431eeb,571a553f)nlohmann_jsonrange corrected; openssl pinned to avoid Conan resolver conflictsTest results (ASan + UBSan on Linux)
Built with
-o homestore:sanitize=True -s build_type=Debug. 24/25 CTest targets pass with zero sanitizer violations. The one failing test (BlkCacheQueue) is a pre-existing parallel-startup conflict where 25 ASan-instrumented processes compete for ASan's fixed shadow-memory region at startup; it passes 4/4 in isolation and is unrelated to these changes.