From 98d2e8e98bf87015528206199507d788d40ff021 Mon Sep 17 00:00:00 2001 From: Matt <47545907+SoundMatt@users.noreply.github.com> Date: Sat, 22 Aug 2026 16:34:10 -0700 Subject: [PATCH] fix(loan): make BufferPool storage outlive-safe via shared_ptr Bug (HIGH, confirmed against source): rcp::loan::BufferPool::loan()'s released rcp::Loan captured `this` (the BufferPool*) by raw pointer in its release closure. rcp::Loan::~Loan() unconditionally invokes that closure when the Loan is destroyed. BufferPool::~BufferPool() did not track or wait for outstanding Loans, so a Loan that outlived the BufferPool it was drawn from would run the release closure against an already-destroyed BufferPool: locking a destroyed std::mutex and writing through a dangling pointer. Genuine use-after-free, not previously documented anywhere in the file as a precondition, and not exercised by any test. Fix: split BufferPool's shared internal state (the fixed-capacity free list, its mutex, entries_len_, and the fault_injector_ pointer) into a private Impl struct owned via std::shared_ptr. BufferPool itself is now a thin wrapper holding that shared_ptr. loan()'s release closure captures the shared_ptr *by value* instead of `this`, so a Loan's own copy of the shared_ptr keeps Impl's storage alive by refcount for as long as the Loan exists, independent of the BufferPool wrapper's own lifetime. Impl's storage is only actually freed once its last owner (the BufferPool wrapper or the release closure of the last outstanding Loan) releases it. Public API surface unchanged: same constructors, loan(), close(), ok(), pooled_count() signatures; new_buffer_pool() unaffected. close()'s documented idempotent/"safe with outstanding Loans" behavior is preserved exactly -- it now just sets a flag on Impl rather than on the BufferPool itself, with identical observable behavior. Updated the file's header/class doc comments to describe the new, now-safe lifetime contract (a Loan may safely outlive its BufferPool) instead of only documenting the old hazard. Test: added "A Loan may safely outlive the BufferPool it was drawn from" to tests/test_loan.cpp -- the literal audit repro: loan() a buffer from a BufferPool constructed in a nested scope into a std::unique_ptr declared in the outer scope, let the BufferPool destruct, then reset the Loan. Kept the existing close()-with-outstanding-Loans test unchanged to confirm that documented behavior still holds after this refactor. Verification: - Clean rebuild (cmake -DRCP_BUILD_TESTS=ON -DCMAKE_BUILD_TYPE=Debug): 0 errors, 0 warnings under this project's -Wall -Wextra -Wpedantic -Wshadow -Wnon-virtual-dtor -Wold-style-cast -Wcast-align -Wunused -Woverloaded-virtual flags. - ctest: 100% tests passed, 58/58 suites (test_loan: 119 assertions in 10 test cases, including the new regression test). - Sanitizer (load-bearing check for a UAF fix): reproduced this repo's CI asan-ubsan-regmap job configuration exactly -- ubuntu-22.04, clang-14, `-fsanitize=address,undefined -fno-omit-frame-pointer -g` / `-DCMAKE_EXE_LINKER_FLAGS="-fsanitize=address,undefined"`, ASAN_OPTIONS=halt_on_error=1, UBSAN_OPTIONS=halt_on_error=1 -- via Docker, and ran test_loan under it. Clean: "All tests passed (119 assertions in 10 test cases)". - Mutation-testing sanity check: temporarily reverted to the raw-`this`- capture version, rebuilt under the identical sanitizer configuration, and ran the new regression test. ASan caught it immediately: ==4341==ERROR: AddressSanitizer: stack-use-after-scope on address 0xffffe8c00558 at pc 0xaaaae9680b88 bp 0xffffe8bffc30 sp 0xffffe8bffc28 READ of size 8 at 0xffffe8c00558 thread T0 #0 ... in rcp::loan::BufferPool::loan(...)::'lambda'()::operator()() /work/include/rcp/loan.hpp:152:21 ... #5 ... in rcp::Loan::~Loan() /work/include/rcp/rcp.hpp:123:29 ... #9 ... in CATCH2_INTERNAL_TEST_12() /work/tests/test_loan.cpp:133:14 ... Address 0xffffe8c00558 is located in stack of thread T0 at offset 632 in frame #0 ... CATCH2_INTERNAL_TEST_12() /work/tests/test_loan.cpp:113 This frame has 24 object(s): [32, 40) 'loan_out' (line 123) [64, 648) 'pool' (line 125) <== Memory access at offset 632 is inside this variable ... SUMMARY: AddressSanitizer: stack-use-after-scope /work/include/rcp/loan.hpp:152:21 in rcp::loan::BufferPool::loan(...)::'lambda'()::operator()() ==4341==ABORTING Reapplied the fix, rebuilt under the same sanitizer configuration again: clean pass, "All tests passed (119 assertions in 10 test cases)". Closes a finding from the cpp-RCP v3.0.0 deep audit (loan.hpp BufferPool::loan()/~BufferPool() use-after-free on Loan-outlives-pool). Co-Authored-By: Claude Sonnet 5 Signed-off-by: Matt <47545907+SoundMatt@users.noreply.github.com> --- include/rcp/loan.hpp | 129 ++++++++++++++++++++++++++++++++----------- tests/test_loan.cpp | 26 +++++++++ 2 files changed, 124 insertions(+), 31 deletions(-) diff --git a/include/rcp/loan.hpp b/include/rcp/loan.hpp index caa5fcc..3f11954 100644 --- a/include/rcp/loan.hpp +++ b/include/rcp/loan.hpp @@ -76,6 +76,22 @@ // so that call site can be exercised via fault injection. See rcp/ // alloc.hpp's own header comment for why this is an opt-in, instance-owned // seam rather than a global allocator hook. +// +// ── Loan lifetime independent of BufferPool (audit fix) ───────────────────── +// BufferPool's own free-list bookkeeping (the fixed-capacity array above, +// its mutex, entries_len_, and the fault_injector_ seam) lives in a private +// Impl struct held via std::shared_ptr, not directly as BufferPool data +// members. loan()'s release closure captures that shared_ptr *by +// value*, not `this` (the BufferPool*): a Loan handed out by loan() may +// therefore safely outlive the BufferPool object it was drawn from — e.g. a +// Loan stored in an outer scope while the BufferPool that produced it goes +// out of scope (and destructs) first. Impl's storage is only actually freed +// once its last owner — the BufferPool wrapper itself, or the release +// closure of the last still-outstanding Loan — releases its +// shared_ptr, never while any Loan drawn from this pool could still +// reach it. This does not change close()'s own documented behavior (still +// idempotent, still safe with outstanding Loans alive) — see close()'s own +// doc comment below. #pragma once #include "alloc.hpp" @@ -104,11 +120,25 @@ class BufferPool { // nullptr so every pre-existing construction site (including // new_buffer_pool()) is unaffected; see this file's own header comment. // fault_injector is not owned by this pool and must outlive it. - explicit BufferPool(alloc::FaultInjector* fault_injector) : fault_injector_(fault_injector) {} + explicit BufferPool(alloc::FaultInjector* fault_injector) + : impl_(std::make_shared(fault_injector)) {} + + // Non-copyable: mirrors this class's shape before the shared_ptr + // refactor below, where non-copyable members (std::mutex, std::atomic) + // made copying implicitly deleted. Also non-movable: the user-declared + // destructor below suppresses the implicit move members, exactly as it + // did before this refactor. + BufferPool(const BufferPool&) = delete; + BufferPool& operator=(const BufferPool&) = delete; ~BufferPool() { + // Marks the pool closed. Impl's own storage (the free list, its + // mutex, fault_injector_) is only actually torn down once the last + // owning std::shared_ptr — this member's own impl_, or the + // release closure of any Loan drawn from this pool that is still + // alive — releases it. See this file's "Loan lifetime independent + // of BufferPool" header comment. close(); - for (size_t i = 0; i < entries_len_; i++) delete entries_[i]; } // loan returns a zeroed buffer of exactly size bytes, drawn from the @@ -121,17 +151,17 @@ class BufferPool { // free-list search below only removes an entry on a cache *hit*, which // never reaches the fault-injection check at all). std::error_code loan(int size, std::unique_ptr& out) { - if (closed_.load(std::memory_order_acquire)) return ErrClosed; + if (impl_->closed_.load(std::memory_order_acquire)) return ErrClosed; if (size < 0) return std::make_error_code(std::errc::invalid_argument); std::vector* raw = nullptr; { - std::lock_guard lk(pool_mu_); - for (size_t i = 0; i < entries_len_; i++) { - if (entries_[i]->size() >= static_cast(size)) { - raw = entries_[i]; - entries_[i] = entries_[entries_len_ - 1]; - entries_len_--; + std::lock_guard lk(impl_->pool_mu_); + for (size_t i = 0; i < impl_->entries_len_; i++) { + if (impl_->entries_[i]->size() >= static_cast(size)) { + raw = impl_->entries_[i]; + impl_->entries_[i] = impl_->entries_[impl_->entries_len_ - 1]; + impl_->entries_len_--; break; } } @@ -140,18 +170,27 @@ class BufferPool { if (raw) { raw->assign(static_cast(size), 0); // re-zero: no stale data leaks across reuse } else { - if (fault_injector_ && fault_injector_->should_fail()) + if (impl_->fault_injector_ && impl_->fault_injector_->should_fail()) return alloc::make_error_code(alloc::AllocErrc::simulated_allocation_failure); raw = new std::vector(static_cast(size), 0); } + // The release closure captures impl_ (the shared_ptr itself, by + // value) rather than `this`: a Loan may legitimately outlive the + // BufferPool object that handed it out (see this file's "Loan + // lifetime independent of BufferPool" header comment), and impl_'s + // refcount is what keeps the pool's free list / mutex / + // fault_injector_ alive for exactly as long as any outstanding + // Loan still needs them, even after this BufferPool wrapper itself + // has been destroyed. + auto impl = impl_; out = std::make_unique( *raw, // Loan owns its own copy of the payload (rcp::Loan's own by-value contract) - [this, raw]() mutable { - std::lock_guard lk(pool_mu_); - if (entries_len_ < kPoolMaxEntries) { - entries_[entries_len_] = raw; - entries_len_++; + [impl, raw]() mutable { + std::lock_guard lk(impl->pool_mu_); + if (impl->entries_len_ < kPoolMaxEntries) { + impl->entries_[impl->entries_len_] = raw; + impl->entries_len_++; return; } // Free list already at c-RCP's own RCP_LOAN_POOL_MAX_ENTRIES @@ -166,31 +205,59 @@ class BufferPool { // close is idempotent — safe to call more than once, including while // Loans obtained before the call are still alive (their eventual // release simply grows a pool nobody will draw from again, up to - // kPoolMaxEntries). - void close() { closed_.store(true, std::memory_order_release); } + // kPoolMaxEntries) — and it remains safe to call from this BufferPool's + // own destructor even with outstanding Loans alive, since it sets a + // flag on Impl (kept alive by shared_ptr refcounting), never + // dereferences a dangling `this`. + void close() { impl_->closed_.store(true, std::memory_order_release); } - bool ok() const noexcept { return !closed_.load(std::memory_order_acquire); } + bool ok() const noexcept { return !impl_->closed_.load(std::memory_order_acquire); } // pooled_count reports how many released buffers are currently held // for reuse — introspection for tests, not part of the loan/release // contract itself. Always <= kPoolMaxEntries, by construction. size_t pooled_count() const { - std::lock_guard lk(pool_mu_); - return entries_len_; + std::lock_guard lk(impl_->pool_mu_); + return impl_->entries_len_; } private: - std::atomic closed_{false}; - mutable std::mutex pool_mu_; - // Fixed-capacity free list (ported from c-RCP's RCP_LOAN_POOL_MAX_ENTRIES - // — see this file's own header comment): entries_ is a plain - // std::array of raw pointers, not a realloc()/std::vector-grown - // container — the SLOTS are static, each individual buffer's own bytes - // are not (matching c-RCP's own design exactly). - std::array*, kPoolMaxEntries> entries_{}; - size_t entries_len_ = 0; // always <= kPoolMaxEntries - - alloc::FaultInjector* fault_injector_ = nullptr; // not owned; see constructor doc comment + // Impl holds every piece of this pool's state that loan()'s release + // closure needs to reach after a successful loan() call: the fixed- + // capacity free list, its mutex, and the (non-owned) fault_injector_. + // BufferPool itself is now just a thin std::shared_ptr wrapper; + // splitting the state out this way is what lets the release closure + // capture impl_ (the shared_ptr) *by value* instead of `this` (see + // loan()'s own comment above) — the closure's own copy of impl_ keeps + // this storage alive by refcount for as long as the Loan itself is + // alive, independent of whether the BufferPool wrapper that produced + // it still exists. See this file's "Loan lifetime independent of + // BufferPool" header comment. + struct Impl { + std::atomic closed_{false}; + std::mutex pool_mu_; + // Fixed-capacity free list (ported from c-RCP's RCP_LOAN_POOL_MAX_ENTRIES + // — see this file's own header comment): entries_ is a plain + // std::array of raw pointers, not a realloc()/std::vector-grown + // container — the SLOTS are static, each individual buffer's own bytes + // are not (matching c-RCP's own design exactly). + std::array*, kPoolMaxEntries> entries_{}; + size_t entries_len_ = 0; // always <= kPoolMaxEntries + + alloc::FaultInjector* fault_injector_ = nullptr; // not owned; see BufferPool's constructor doc comment + + explicit Impl(alloc::FaultInjector* fault_injector) : fault_injector_(fault_injector) {} + + // Frees whatever's still in the free list once the last owner of + // this Impl — this BufferPool's own impl_ member, or the release + // closure of the last still-outstanding Loan — releases its + // shared_ptr. + ~Impl() { + for (size_t i = 0; i < entries_len_; i++) delete entries_[i]; + } + }; + + std::shared_ptr impl_ = std::make_shared(nullptr); }; inline std::unique_ptr new_buffer_pool() { diff --git a/tests/test_loan.cpp b/tests/test_loan.cpp index 2644987..f080ad7 100644 --- a/tests/test_loan.cpp +++ b/tests/test_loan.cpp @@ -107,6 +107,32 @@ TEST_CASE("loan::BufferPool::close is idempotent and safe with outstanding Loans loan_out.reset(); } +// ── Loan lifetime independent of BufferPool (audit fix, use-after-free) ──── + +TEST_CASE("A Loan may safely outlive the BufferPool it was drawn from", + "[loan][REQ-LOAN-008][audit-fix]") { + // Literal repro from the audit finding: the BufferPool is destroyed + // while a Loan drawn from it is still alive in an outer scope, and + // only afterward is that Loan released. Before the fix, the Loan's + // release closure captured the BufferPool's `this` by raw pointer, so + // releasing it here would lock an already-destroyed std::mutex and + // write through a dangling pointer -- a heap-use-after-free that ASan + // must catch if this fix regresses. Now the pool's shared state + // outlives the BufferPool wrapper via std::shared_ptr, so this must + // simply work. + std::unique_ptr loan_out; + { + loan::BufferPool pool; + REQUIRE_FALSE(pool.loan(64, loan_out)); + REQUIRE(loan_out != nullptr); + REQUIRE(loan_out->payload.size() == 64); + } // pool destructs here; loan_out is still alive, drawn from it + + // Releasing (or destroying) the Loan after its pool is gone must not + // crash or use freed/destroyed memory. + loan_out.reset(); +} + // ── new_buffer_pool() ──────────────────────────────────────────────────────── TEST_CASE("new_buffer_pool returns a valid, open, empty pool", "[loan][REQ-LOAN-009]") {