Repository navigation
fix: apply RELAY spec v0.2 update — ErrAlreadyExists standalone - #9
Merged
Merged
Conversation
…elay mapping RELAY spec maintainer updated §5.4 in response to issue #8 (RELAY issue #8): ErrAlreadyExists is now a standalone RCP error, not mapped to any relay sentinel. - rcp_category()::equivalent(): remove already_exists → ErrClosed mapping - test_relay.cpp: add test asserting ErrAlreadyExists != all relay sentinels Also adds the standalone ErrAlreadyExists test (24 test cases / 55 assertions). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Matt Jones <47545907+SoundMatt@users.noreply.github.com>
SoundMatt
added a commit
that referenced
this pull request
Aug 22, 2026
…177) 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<Impl>. BufferPool itself is now a thin wrapper holding that shared_ptr. loan()'s release closure captures the shared_ptr<Impl> *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<rcp::Loan> 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). Signed-off-by: Matt <47545907+SoundMatt@users.noreply.github.com> Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
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
1 participant
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
Applies the RELAY spec v0.2 update notified in issue #8.
The RELAY spec maintainer updated §5.4 in response to issues raised during cpp-RCP's RELAY conformance implementation (PR #7). The key change:
ErrAlreadyExistsis now a standalone RCP error, not mapped to any relay sentinel.Previously:
ErrAlreadyExistswrappedrelay::ErrClosed(semantically inconsistent — a uniqueness-violation error doesn't represent a closed-connection condition).Updated:
ErrAlreadyExistshas no relay mapping; it is protocol-specific with no equivalent relay sentinel.Changes
rcp_category()::equivalent():already_existscase now returnsfalsefor all relay conditionstests/test_relay.cpp: new test assertingErrAlreadyExists != all four relay sentinelsTest plan
rcp_relay