net/tls: hold the concurrent-put reproducer's gate state by shared_ptr by dotnwat · Pull Request #287 · redpanda-data/seastar

dotnwat · 2026-06-11T22:40:50Z

This fixes a UAF bug in the reproducer test that I hit while backporting.

Test-only follow-up to #281.

test_concurrent_put_with_key_update keeps its gate_state (overlap counter + arm/release promises) on the test fiber's stack and decrements gate.outstanding from the instrumented sink's put() .finally(). That finally can run on the reactor after the SEASTAR_THREAD_TEST_CASE fiber has returned and freed its stack, so the reference into the dead frame is a use-after-return. ASan (debug builds, detect_stack_use_after_return=1) traps it as a SEGV.

Whether it fires is timing-dependent, so the original merge passed CI by luck. The same latent bug then failed the debug+OpenSSL job on the v25.3.x backport of #281 while passing on v26.1.x.

Fix

Heap-allocate gate_state via lw_shared_ptr, have the sink co-own it, and capture the shared_ptr in the .finally(). Its lifetime then extends to match the sink's. Test-only; no production code is touched.

Anticipating the review questions

Q: Why does the finally run after the test body returns — didn't we close/shutdown everything first?

openssl_session::close() returns void and hands the caller no future. It runs the TLS bye-handshake + _in.close()/_out.close() as a detached reactor task (src/net/tls_openssl.cc):

void close() noexcept override {
    ...
    auto me = shared_from_this();
    engine().run_in_background(std::move(f)
        .finally([this]{ _eof = true; return _in.close(); })
        .finally([this]{ return _out.close(); })   // closes the sink
        ...
        .handle_exception([me = std::move(me)](std::exception_ptr){})
        .discard_result());
}

The session keeps itself (and the sink) alive via shared_from_this() and discard_result()s the future, so this task can run for up to bye_timeout (~10s) after close() returns. Tearing the sink down (_out.close()) can complete an in-flight put() whose .finally then runs on the reactor — after the test fiber is gone. (shutdown_input()/shutdown_output() are also void.)

Q: Then can't we just wait/drain before the test returns instead of changing the gate?

No clean way. There is deliberately no future to await (close() is void + run_in_background + discard_result) and no test-visible "fully torn down" signal. A poll like while (gate.outstanding) yield(); races the detached task: it can issue a sink put() at any point in its window, including after the poll observes zero. So a drain would re-introduce exactly the timing-dependence we're removing.

Q: If the teardown outlives the caller, isn't that inherently unsafe?

Not for seastar's own state. The session self-owns (enable_shared_from_this) for the whole detached teardown, and the reactor drains run_in_background tasks before it stops — so the session, sink and socket are freed only once the work finishes. The only thing that's unsafe is state the test reached into that teardown by raw reference (gate_state& _gate → the fiber stack), whose lifetime ends at body return. That mismatch is the bug; owning the shared state by shared_ptr (the idiomatic rule for state captured into reactor continuations) is the fix.

Q: How do we know this is the mechanism and not a guess?

Confirmed deterministically (debug + ASan, --tls-mode=openssl):

instrumenting the sink shows it is destroyed after the test body returns;
forcing a put() to be in flight at body-end reproduces the exact CI SEGV in the .finally with the stack gate, while the shared_ptr gate runs that same late finally safely.

Validation

test_concurrent_put_with_key_update and the full tls_test suite pass under debug + ASan (detect_stack_use_after_return=1), OpenSSL backend, with no SEGV.

🤖 Generated with Claude Code

Copilot

Pull request overview

This PR fixes a use-after-return bug in the OpenSSL-specific TLS reproducer test test_concurrent_put_with_key_update by ensuring the shared “gate” state outlives any late-running .finally() continuation during teardown.

Changes:

Heap-allocate the test’s gate_state using lw_shared_ptr instead of keeping it on the test stack.
Capture the shared pointer in the sink’s .finally() continuation to prevent teardown-time stack UAF.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

test_concurrent_put_with_key_update (added in #281) keeps its gate_state (the overlap-detection counter and the arm/release promises) on the test fiber's stack and decrements gate.outstanding from the instrumented sink's put() .finally(). That finally can run on the reactor AFTER the SEASTAR_THREAD_TEST_CASE fiber has returned and freed its stack, so the reference into the dead frame is a use-after-return. ASan (debug builds, detect_stack_use_after_return=1) traps it as a SEGV. Whether it fires is timing-dependent, so the original merge passed CI; the same latent bug then failed the debug+OpenSSL job on the v25.3.x backport of #281 while passing on v26.1.x. Why the finally outlives the test body (and why a drain cannot fix it) openssl_session::close() does not close synchronously and hands the caller no future. It runs the TLS bye-handshake plus _in.close()/ _out.close() as a DETACHED reactor task (src/net/tls_openssl.cc): void close() noexcept override { ... auto me = shared_from_this(); engine().run_in_background(std::move(f) .finally([this]{ _eof = true; return _in.close(); }) .finally([this]{ return _out.close(); }) // closes the sink ... .handle_exception([me = std::move(me)](std::exception_ptr){}) .discard_result()); } The session keeps itself (and therefore this sink) alive via shared_from_this() and discards the future, so the task can run for up to bye_timeout (~10s) after close() returns, with no handle for anyone to await. Tearing the sink down (_out.close()) can complete an in-flight put() whose .finally then runs on the reactor -- after the test fiber is long gone. This is safe for seastar's own state: the session self-owns for the whole detached teardown and the reactor drains run_in_background tasks before it stops, so the session, sink and socket are freed only once the work finishes. It is unsafe only for state the test reached into that teardown by raw reference -- here, gate_state on the fiber stack, whose lifetime ends when the test body returns. Because there is deliberately no future to await and no "fully torn down" signal, a poll such as `while (gate.outstanding) yield()` cannot close the race: the detached task may issue a sink put() at any point in its window, including after a poll has observed zero. shutdown_input()/shutdown_output() are likewise void, so they cannot be awaited either. The fix is lifetime-matching -- the idiomatic seastar rule that state shared with a reactor continuation must be owned (shared_ptr), not referenced from a stack frame. Heap-allocate gate_state via lw_shared_ptr, have the sink co-own it, and capture the shared_ptr in the .finally(). Its lifetime then extends to match the sink's, i.e. as long as anything can still touch it. Test-only change; no production code is touched. The mechanism was confirmed deterministically (not just reasoned): an instrumented build shows the sink destroyed after the test body returns, and forcing a put() to be in flight at body-end reproduces the exact CI SEGV in the .finally with the stack gate, while the shared_ptr gate runs that same late finally safely. Verified under debug + ASan (detect_stack_use_after_return=1) with the OpenSSL backend (--tls-mode=openssl): test_concurrent_put_with_key_update and the full tls_test suite pass with no SEGV. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Backport of the reproducer from #281 (merged to v26.2.x) to v25.3.x. The OpenSSL TLS read and write paths use separate semaphores (_in_sem vs _out_sem) and both can write to the underlying socket: a write encrypts application data, while a read can emit a TLS key-update/renegotiation response produced inside SSL_read_ex. When a read emits output while a write's put() is still in flight, the TLS layer issues a second, concurrent put() on the same data_sink, which trips SEASTAR_ASSERT(!_p) in posix_data_sink_impl. Add test_concurrent_put_with_key_update, which installs an instrumented data_sink under the client that flags overlapping put()s (what the posix assert detects) and holds one put to open a deterministic window. It drives two server key-updates -- OpenSSL only flushes the response to the first while reading the second -- and that read-path put is held in flight while a concurrent client write tries to issue the colliding put. This test fails (overlap detected) against the current OpenSSL backend; it is fixed by the following commit. GnuTLS re-runs the handshake while holding both semaphores, so it never issues the concurrent put; the test is compiled out under the GnuTLS backend. Backport notes: on v26.2.x the OpenSSL backend lives in src/net/tls_openssl.cc and the reproducer drives key-updates through the public tls::force_rehandshake(), implemented there via SSL_key_update(). On v25.3.x the backend is src/net/ossl.cc and has no rehandshake / key-update entry point at all (force_rehandshake() does not exist on this branch), so rather than add that production API this commit adds a minimal, test-only hook -- session::trigger_key_update_for_test() plus a free function tls::trigger_key_update_for_test(connected_socket&), neither declared in any public header -- that performs just the SSL_key_update() the test needs. The test also selects the backend at compile time (SEASTAR_USE_GNUTLS) rather than via the runtime tls::backend_name() used on v26.2.x, and its instrumented data_sink overrides put(net::packet) to match this branch's data_sink API. Gate lifetime (why gate_state is shared, not on the stack): the instrumented sink decrements gate_state.outstanding from a put() .finally(), and that finally can run on the reactor AFTER this SEASTAR_THREAD_TEST_CASE fiber has returned and freed its stack -- so a gate referenced from the fiber stack would be a use-after-return (an intermittent SEGV under ASan, which is exactly what failed the debug+ OpenSSL CI job for this backport). The reason the finally outlives the body is that session::close() (src/net/ossl.cc) does not close synchronously and returns no future: it runs the bye-handshake plus _in.close()/_out.close() as a DETACHED reactor task via engine().run_in_background(...), keeps the session (and this sink) alive with shared_from_this(), and discards the future. So the caller has nothing to await, the task can run for up to bye_timeout (~10s) after close() returns, and tearing the sink down can complete a put() whose finally then runs once the test fiber is gone. This is safe for seastar's own state (the session self-owns for the whole teardown and the reactor drains run_in_background tasks before it stops); it is unsafe only for state the test reached into that teardown by raw reference. Because there is no awaitable handle and no "fully torn down" signal, a drain/poll such as `while (gate.outstanding) yield()` cannot fix it (the detached task may issue a put after the poll observes zero). The fix is lifetime-matching: gate_state is held by lw_shared_ptr, the sink co-owns it, and the .finally() captures the shared_ptr, so its lifetime extends to match the sink's. Confirmed deterministically -- forcing a put in flight at body-end reproduces the SEGV in the finally with a stack gate and runs safely with the shared_ptr gate. (Same lifetime fix as #287, which applies it to the already-merged v26.2.x reproducer.) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Backport of the reproducer from #281 (merged to v26.2.x) to v26.1.x. The OpenSSL TLS read and write paths use separate semaphores (_in_sem vs _out_sem) and both can write to the underlying socket: a write encrypts application data, while a read can emit a TLS key-update/renegotiation response produced inside SSL_read_ex. When a read emits output while a write's put() is still in flight, the TLS layer issues a second, concurrent put() on the same data_sink, which trips SEASTAR_ASSERT(!_p) in posix_data_sink_impl. Add test_concurrent_put_with_key_update, which installs an instrumented data_sink under the client that flags overlapping put()s (what the posix assert detects) and holds one put to open a deterministic window. It drives two server key-updates -- OpenSSL only flushes the response to the first while reading the second -- and that read-path put is held in flight while a concurrent client write tries to issue the colliding put. This test fails (overlap detected) against the current OpenSSL backend; it is fixed by the following commit. GnuTLS re-runs the handshake while holding both semaphores, so it never issues the concurrent put; the test is compiled out under the GnuTLS backend. Backport notes: on v26.2.x the OpenSSL backend lives in src/net/tls_openssl.cc and the reproducer drives key-updates through the public tls::force_rehandshake(), which is implemented there via SSL_key_update(). On v26.1.x the backend is src/net/ossl.cc and force_rehandshake() is unimplemented for OpenSSL, so rather than backport that production feature this commit adds a minimal, test-only hook -- session::trigger_key_update_for_test() plus a free function tls::trigger_key_update_for_test(connected_socket&), neither declared in any public header -- that performs just the SSL_key_update() the test needs. The test also selects the backend at compile time (SEASTAR_USE_GNUTLS) rather than via the runtime tls::backend_name() used on v26.2.x's dual-backend build. Gate lifetime (why gate_state is shared, not on the stack): the instrumented sink decrements gate_state.outstanding from a put() .finally(), and that finally can run on the reactor AFTER this SEASTAR_THREAD_TEST_CASE fiber has returned and freed its stack -- so a gate referenced from the fiber stack would be a use-after-return (an intermittent SEGV under ASan; this is the same class of bug that failed the debug+OpenSSL CI job on the v25.3.x backport). The reason the finally outlives the body is that session::close() (src/net/ossl.cc) does not close synchronously and returns no future: it runs the bye-handshake plus _in.close()/_out.close() as a DETACHED reactor task via engine().run_in_background(...), keeps the session (and this sink) alive with shared_from_this(), and discards the future. So the caller has nothing to await, the task can run for up to bye_timeout (~10s) after close() returns, and tearing the sink down can complete a put() whose finally then runs once the test fiber is gone. This is safe for seastar's own state (the session self-owns for the whole teardown and the reactor drains run_in_background tasks before it stops); it is unsafe only for state the test reached into that teardown by raw reference. Because there is no awaitable handle and no "fully torn down" signal, a drain/poll such as `while (gate.outstanding) yield()` cannot fix it (the detached task may issue a put after the poll observes zero). The fix is lifetime-matching: gate_state is held by lw_shared_ptr, the sink co-owns it, and the .finally() captures the shared_ptr, so its lifetime extends to match the sink's. Confirmed deterministically -- forcing a put in flight at body-end reproduces the SEGV in the finally with a stack gate and runs safely with the shared_ptr gate. (Same lifetime fix as #287, which applies it to the already-merged v26.2.x reproducer.) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

dotnwat · 2026-06-12T02:46:02Z

Anticipating the review questions

Haha Claude is out here repeating my narrative

Backport of the reproducer from #281 (merged to v26.2.x) to v26.1.x. The OpenSSL TLS read and write paths use separate semaphores (_in_sem vs _out_sem) and both can write to the underlying socket: a write encrypts application data, while a read can emit a TLS key-update/renegotiation response produced inside SSL_read_ex. When a read emits output while a write's put() is still in flight, the TLS layer issues a second, concurrent put() on the same data_sink, which trips SEASTAR_ASSERT(!_p) in posix_data_sink_impl. Add test_concurrent_put_with_key_update, which installs an instrumented data_sink under the client that flags overlapping put()s (what the posix assert detects) and holds one put to open a deterministic window. It drives two server key-updates -- OpenSSL only flushes the response to the first while reading the second -- and that read-path put is held in flight while a concurrent client write tries to issue the colliding put. This test fails (overlap detected) against the current OpenSSL backend; it is fixed by the following commit. GnuTLS re-runs the handshake while holding both semaphores, so it never issues the concurrent put; the test is compiled out under the GnuTLS backend. Backport notes: on v26.2.x the OpenSSL backend lives in src/net/tls_openssl.cc and the reproducer drives key-updates through the public tls::force_rehandshake(), which is implemented there via SSL_key_update(). On v26.1.x the backend is src/net/ossl.cc and force_rehandshake() is unimplemented for OpenSSL, so rather than backport that production feature this commit adds a minimal, test-only hook -- session::trigger_key_update_for_test() plus a free function tls::trigger_key_update_for_test(connected_socket&), neither declared in any public header -- that performs just the SSL_key_update() the test needs. The test also selects the backend at compile time (SEASTAR_USE_GNUTLS) rather than via the runtime tls::backend_name() used on v26.2.x's dual-backend build. Gate lifetime (why gate_state is shared, not on the stack): the instrumented sink decrements gate_state.outstanding from a put() .finally(), and that finally can run on the reactor AFTER this SEASTAR_THREAD_TEST_CASE fiber has returned and freed its stack -- so a gate referenced from the fiber stack would be a use-after-return (an intermittent SEGV under ASan; this is the same class of bug that failed the debug+OpenSSL CI job on the v25.3.x backport). The reason the finally outlives the body is that session::close() (src/net/ossl.cc) does not close synchronously and returns no future: it runs the bye-handshake plus _in.close()/_out.close() as a DETACHED reactor task via engine().run_in_background(...), keeps the session (and this sink) alive with shared_from_this(), and discards the future. So the caller has nothing to await, the task can run for up to bye_timeout (~10s) after close() returns, and tearing the sink down can complete a put() whose finally then runs once the test fiber is gone. This is safe for seastar's own state (the session self-owns for the whole teardown and the reactor drains run_in_background tasks before it stops); it is unsafe only for state the test reached into that teardown by raw reference. Because there is no awaitable handle and no "fully torn down" signal, a drain/poll such as `while (gate.outstanding) yield()` cannot fix it (the detached task may issue a put after the poll observes zero). The fix is lifetime-matching: gate_state is held by lw_shared_ptr, the sink co-owns it, and the .finally() captures the shared_ptr, so its lifetime extends to match the sink's. Confirmed deterministically -- forcing a put in flight at body-end reproduces the SEGV in the finally with a stack gate and runs safely with the shared_ptr gate. (Same lifetime fix as #287, which applies it to the already-merged v26.2.x reproducer.) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Backport of the reproducer from #281 (merged to v26.2.x) to v25.3.x. The OpenSSL TLS read and write paths use separate semaphores (_in_sem vs _out_sem) and both can write to the underlying socket: a write encrypts application data, while a read can emit a TLS key-update/renegotiation response produced inside SSL_read_ex. When a read emits output while a write's put() is still in flight, the TLS layer issues a second, concurrent put() on the same data_sink, which trips SEASTAR_ASSERT(!_p) in posix_data_sink_impl. Add test_concurrent_put_with_key_update, which installs an instrumented data_sink under the client that flags overlapping put()s (what the posix assert detects) and holds one put to open a deterministic window. It drives two server key-updates -- OpenSSL only flushes the response to the first while reading the second -- and that read-path put is held in flight while a concurrent client write tries to issue the colliding put. This test fails (overlap detected) against the current OpenSSL backend; it is fixed by the following commit. GnuTLS re-runs the handshake while holding both semaphores, so it never issues the concurrent put; the test is compiled out under the GnuTLS backend. Backport notes: on v26.2.x the OpenSSL backend lives in src/net/tls_openssl.cc and the reproducer drives key-updates through the public tls::force_rehandshake(), implemented there via SSL_key_update(). On v25.3.x the backend is src/net/ossl.cc and has no rehandshake / key-update entry point at all (force_rehandshake() does not exist on this branch), so rather than add that production API this commit adds a minimal, test-only hook -- session::trigger_key_update_for_test() plus a free function tls::trigger_key_update_for_test(connected_socket&), neither declared in any public header -- that performs just the SSL_key_update() the test needs. The test also selects the backend at compile time (SEASTAR_USE_GNUTLS) rather than via the runtime tls::backend_name() used on v26.2.x, and its instrumented data_sink overrides put(net::packet) to match this branch's data_sink API. Gate lifetime (why gate_state is shared, not on the stack): the instrumented sink decrements gate_state.outstanding from a put() .finally(), and that finally can run on the reactor AFTER this SEASTAR_THREAD_TEST_CASE fiber has returned and freed its stack -- so a gate referenced from the fiber stack would be a use-after-return (an intermittent SEGV under ASan, which is exactly what failed the debug+ OpenSSL CI job for this backport). The reason the finally outlives the body is that session::close() (src/net/ossl.cc) does not close synchronously and returns no future: it runs the bye-handshake plus _in.close()/_out.close() as a DETACHED reactor task via engine().run_in_background(...), keeps the session (and this sink) alive with shared_from_this(), and discards the future. So the caller has nothing to await, the task can run for up to bye_timeout (~10s) after close() returns, and tearing the sink down can complete a put() whose finally then runs once the test fiber is gone. This is safe for seastar's own state (the session self-owns for the whole teardown and the reactor drains run_in_background tasks before it stops); it is unsafe only for state the test reached into that teardown by raw reference. Because there is no awaitable handle and no "fully torn down" signal, a drain/poll such as `while (gate.outstanding) yield()` cannot fix it (the detached task may issue a put after the poll observes zero). The fix is lifetime-matching: gate_state is held by lw_shared_ptr, the sink co-owns it, and the .finally() captures the shared_ptr, so its lifetime extends to match the sink's. Confirmed deterministically -- forcing a put in flight at body-end reproduces the SEGV in the finally with a stack gate and runs safely with the shared_ptr gate. (Same lifetime fix as #287, which applies it to the already-merged v26.2.x reproducer.) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Copilot AI review requested due to automatic review settings June 11, 2026 22:40

Copilot started reviewing on behalf of dotnwat June 11, 2026 22:41 View session

Copilot AI reviewed Jun 11, 2026

View reviewed changes

dotnwat force-pushed the fix-tls-reproducer-uaf-v26.2.x branch from a153159 to a15ec2e Compare June 11, 2026 23:36

dotnwat requested review from joe-redpanda and travisdowns June 12, 2026 02:47

travisdowns approved these changes Jun 12, 2026

View reviewed changes

dotnwat merged commit 5d474c8 into v26.2.x Jun 12, 2026
39 checks passed

This was referenced Jun 23, 2026

[v26.1.x] net/tls: Fix concurrent put() on the socket in the OpenSSL backend #291

Merged

[v25.3.x] net/tls: Fix concurrent put() on the socket in the OpenSSL backend #292

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

net/tls: hold the concurrent-put reproducer's gate state by shared_ptr#287

net/tls: hold the concurrent-put reproducer's gate state by shared_ptr#287
dotnwat merged 1 commit into
v26.2.xfrom
fix-tls-reproducer-uaf-v26.2.x

dotnwat commented Jun 11, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

dotnwat commented Jun 12, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

dotnwat commented Jun 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Fix

Anticipating the review questions

Validation

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

dotnwat commented Jun 12, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

dotnwat commented Jun 11, 2026 •

edited

Loading