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]") {