Fix openssl tls concurrent put v26.2.x v2#281
Merged
Conversation
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 skipped there. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR addresses an OpenSSL-specific TLS backend crash caused by concurrent data_sink::put() calls originating from the read path (key-update flush inside SSL_read_ex) racing with an application write, which can violate the one-in-flight-put invariant enforced by posix_data_sink_impl.
Changes:
- Replace the
_output_pendingtracking in the OpenSSL TLS session withshared_future<>and updatewait_for_output()to avoid prematurely marking output as idle. - Ensure the write path drains any in-flight output before entering
SSL_write_ex, preventing read/write interleavings from issuing overlapping puts. - Add a regression unit test that reproduces the concurrent-put scenario (skipped for GnuTLS).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| tests/unit/tls_test.cc | Adds an OpenSSL-only regression test that detects overlapping put() calls by instrumenting the client socket sink. |
| src/net/tls_openssl.cc | Fixes output serialization by keeping _output_pending truthful via shared_future<> and adjusting the draining/guard logic around BIO writes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+2350
to
+2352
| auto fR = cin.read(); | ||
| gate.entered.get_future().get(); | ||
|
|
Comment on lines
2381
to
+2384
| try { | ||
| size_t n; | ||
|
|
||
| // Skip issuing if a previous put() failed; the failed _output_pending is |
Comment on lines
+2372
to
+2374
| // A put() is still draining (or never started): refuse, ask OpenSSL to | ||
| // retry later. This is the mutual-exclusion check -- it is honest only | ||
| // because wait_for_output() does not mark _output_pending ready early. |
pgellert
previously approved these changes
Jun 4, 2026
Comment on lines
+2267
to
+2278
| ++_gate.outstanding; | ||
| future<> hold = make_ready_future<>(); | ||
| if (_gate.arm) { | ||
| _gate.arm = false; // one-shot | ||
| _gate.entered.set_value(); // announce the held put() | ||
| hold = _gate.release.get_future(); | ||
| } | ||
| return hold.then([this, bufs = std::move(bufs)] () mutable { | ||
| return _next.put(std::move(bufs)); | ||
| }).finally([this] { | ||
| --_gate.outstanding; | ||
| }); |
| // server key-updates: OpenSSL only flushes the response to the first one while | ||
| // reading the second, and that flush is the read-path put() we hold in flight | ||
| // while a concurrent client write tries to issue the colliding put(). | ||
| SEASTAR_THREAD_TEST_CASE(test_concurrent_put_with_key_update) { |
There was a problem hiding this comment.
Do we have any proof/indication(e.g. broker logs) showing that this specific scenario (key exchange) is what led to the assertion failure in CORE-16383? It does look like the scenario is a "writing to the socket from session::do_get()", so key exchange seems like a reasonable assumption based on that, but I am just wondering if you have gathered any further context here.
Member
Author
There was a problem hiding this comment.
Hmm, good question. I'll take a look at the incident report and see if there are logs. AFAIK we've never been able to reproduce this ourselves.
Member
Author
There was a problem hiding this comment.
From what I've read the put from the read path can basically happen at anytime and its transparent and either side can initiate it. I guess there are other situations too when it can happen. Anyway, I don't expect that we'd see anything in the logs.
joe-redpanda
previously approved these changes
Jun 4, 2026
travisdowns
reviewed
Jun 8, 2026
| // tears down. Resetting it here would mutate _output_pending from a | ||
| // completion continuation, which would have to be shown not to race with a | ||
| // put() issued in the meantime and clobber a live one; leaving it failed | ||
| // avoids that, and is harmless since an output failure is terminal. |
Member
There was a problem hiding this comment.
The LLMs like to write these historical type comments, which explain the logic between of a certain change, i.e., why we dont' use std::exchange, because in this commit we are removing std::exchange, but for a future reader I don't think this makes much sense because they probably never considered std::exchange in the first place, as they are looking at the current code.
I think that's better in the commit message (which is associated with the "diff") than the comment.
Member
Author
There was a problem hiding this comment.
Agree, you should have seen the comment before I asked Claude to chill out haha. I'll move it
| // inside handle_do_put_ssl_err() before it returns stop_iteration::no). | ||
| // Draining up front just avoids that round-trip and keeps the loop's | ||
| // "drain before each SSL_write_ex" shape uniform. | ||
| co_await wait_for_output(); |
Member
There was a problem hiding this comment.
If this is only an "optimization" for an extra roundtrip in a particular edge case (like TLS protocols generating its own writes during application reads), then I think we should not do it? Because this function is on the CPU-hot path, invoked once for every small buf. So it would be a pessimization overall. If it's needed for correctness that's another story.
|
Not needed imo for this pr but thoughts on an ssl / network stack chaos test? |
travisdowns
previously approved these changes
Jun 10, 2026
A data_sink permits only one put() in flight at a time
(posix_data_sink_impl asserts !_p). That contract was being violated
because wait_for_output() did
std::exchange(_output_pending, make_ready_future())
to obtain something to await, which marked _out "idle" while the put()
was still draining. bio_write_ex()'s guard then issued a second,
concurrent put() from the read path while a write-path put() was in
flight, tripping the assert (reproduced by the preceding commit).
This is not a missing lock: the reactor is single-threaded and the
bio_write_ex() guard is already atomic. It was the guard reading
falsified state. Make _output_pending a shared_future so that
wait_for_output() can hand out an awaitable via get_future() without
consuming or replacing the in-flight put; available()/failed() then stay
truthful and both the read and write paths can await the same put.
The interleaving, with a write fiber W (do_put, holding _out_sem) and a
read fiber R (do_get, holding _in_sem); R reaches bio_write_ex because
SSL_read_ex emits a key-update response while processing an incoming
record ("|" marks a reactor switch between the fibers):
BEFORE, when wait_for_output() did
std::exchange(_output_pending, make_ready_future()):
W SSL_write_ex -> bio_write_ex: _output_pending idle, so issue put_W;
the sink sets _p and starts write_all() (put_W now UNRESOLVED).
W co_await wait_for_output() evaluates the exchange synchronously: it
swaps _output_pending to a *ready* future, then parks W on put_W.
| put_W's write_all() is still draining, yet _output_pending now
reads "available".
R SSL_read_ex -> bio_write_ex: _output_pending available, so issue
put_R -> posix_data_sink_impl::put() trips SEASTAR_ASSERT(!_p),
because _p still holds put_W's packet. *** assertion failure ***
AFTER, with wait_for_output() == _output_pending.get_future() (no swap):
W bio_write_ex issues put_W; _output_pending = shared_future(put_W),
which stays UNRESOLVED while write_all() drains.
W co_await wait_for_output() takes a get_future() on put_W and parks
W on it, leaving _output_pending untouched.
| put_W still draining; _output_pending correctly reads "not
available".
R SSL_read_ex -> bio_write_ex: _output_pending not available, so
decline (BIO_set_retry_write) -- no second put(). SSL_read_ex
returns WANT_WRITE; do_get co_awaits wait_for_output(), a SECOND
get_future() on the SAME put_W, and parks R on it too.
| write_all() completes, the sink clears _p, put_W resolves, and the
shared state resolves BOTH waiters.
R resumes, retries SSL_read_ex -> bio_write_ex now idle -> issues
put_R into a clear sink. W resumes and continues. One put() at a
time.
A write can also enter do_put_one() with a put already in flight, for the
same reason R does above: the read path emits a key-update put while the
write holds the disjoint _out_sem. That needs no up-front drain --
bio_write_ex() declines while a put is in flight (BIO_set_retry_write), so
the first SSL_write_ex reports WANT_WRITE and handle_do_put_ssl_err()
drains and retries, exactly as later loop iterations do.
Remove the SEASTAR_ASSERT(_output_pending.available()) preconditions at
the start of the two do_put() overloads and do_put_one(). They asserted
"no put is in flight when a write begins", which is false for the reason
above: _in_sem and _out_sem are disjoint, so a write taking _out_sem does
not exclude a read-path put. The assert only held before because the
std::exchange() swap left _output_pending spuriously available -- the
same falsification behind the bug -- so with the swap gone it would now
fire on exactly the scenario being fixed. The decline/retry path is the
correct handling; the two overloads just delegate to do_put_one(), so
their asserts are simply dropped. The destructor's available() assert is
kept -- it is still true once close() has drained output, and a failed
_output_pending is also available(). (GnuTLS keeps its copies of these
asserts: its read path never emits output, so the precondition still
holds there.)
On an output failure _output_pending is intentionally left failed and
acts as a circuit breaker -- it is not reset. The _output_pending data
member carries a detailed comment covering the locking model, the
serialization protocol, why the future->shared_future change is the
minimal correct fix (rather than adding a lock), how handed-out waiters
still resolve after reassignment, and the failure behavior.
With this change test_concurrent_put_with_key_update passes, and the full
tls_test suite passes under both the OpenSSL and GnuTLS providers.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
dotnwat
dismissed stale reviews from travisdowns, joe-redpanda, and pgellert
via
1cc4713
dotnwat
force-pushed
the
fix-openssl-tls-concurrent-put-v26.2.x-v2
branch
from
3f8a3d3 to
1cc4713
Compare
Member
Author
Yeh, it would be great. It's unclear how easily this particular issue would surface in a basic chaos test, but maybe if we included OpenSSL configuration tweaking and some kind of timing injections into the harness we could surface some good stuff. |
Member
Author
|
Thanks for the reviews. @travisdowns I removed that optimization and moved the historical parts of the comments into the commit message. This branch is live in upstream CI here redpanda-data/redpanda#30692. |
pgellert
approved these changes
Jun 11, 2026
travisdowns
approved these changes
Jun 11, 2026
This was referenced Jun 11, 2026
dotnwat
added a commit
that referenced
this pull request
Jun 11, 2026
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. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
dotnwat
added a commit
that referenced
this pull request
Jun 11, 2026
Backport of #281 (merged to v26.2.x) to v25.3.x. On v26.2.x the OpenSSL backend lives in src/net/tls_openssl.cc and splits the write path into two do_put() overloads plus do_put_one(); on this branch it is src/net/ossl.cc with a single do_put(net::packet). The buggy pattern and the fix are otherwise the same. A data_sink permits only one put() in flight at a time (posix_data_sink_impl asserts !_p). That contract was being violated because wait_for_output() did std::exchange(_output_pending, make_ready_future()) to obtain something to await, which marked _out "idle" while the put() was still draining. bio_write_ex()'s guard then issued a second, concurrent put() from the read path while a write-path put() was in flight, tripping the assert (reproduced by the preceding commit). This is not a missing lock: the reactor is single-threaded and the bio_write_ex() guard is already atomic. It was the guard reading falsified state. Make _output_pending a shared_future so that wait_for_output() can hand out an awaitable via get_future() without consuming or replacing the in-flight put; available()/failed() then stay truthful and both the read and write paths can await the same put. The interleaving, with a write fiber W (do_put, holding _out_sem) and a read fiber R (do_get, holding _in_sem); R reaches bio_write_ex because SSL_read_ex emits a key-update response while processing an incoming record ("|" marks a reactor switch between the fibers): BEFORE, when wait_for_output() did std::exchange(_output_pending, make_ready_future()): W SSL_write_ex -> bio_write_ex: _output_pending idle, so issue put_W; the sink sets _p and starts write_all() (put_W now UNRESOLVED). W co_await wait_for_output() evaluates the exchange synchronously: it swaps _output_pending to a *ready* future, then parks W on put_W. | put_W's write_all() is still draining, yet _output_pending now reads "available". R SSL_read_ex -> bio_write_ex: _output_pending available, so issue put_R -> posix_data_sink_impl::put() trips SEASTAR_ASSERT(!_p), because _p still holds put_W's packet. *** assertion failure *** AFTER, with wait_for_output() == _output_pending.get_future() (no swap): W bio_write_ex issues put_W; _output_pending = shared_future(put_W), which stays UNRESOLVED while write_all() drains. W co_await wait_for_output() takes a get_future() on put_W and parks W on it, leaving _output_pending untouched. | put_W still draining; _output_pending correctly reads "not available". R SSL_read_ex -> bio_write_ex: _output_pending not available, so decline (BIO_set_retry_write) -- no second put(). SSL_read_ex returns WANT_WRITE; do_get co_awaits wait_for_output(), a SECOND get_future() on the SAME put_W, and parks R on it too. | write_all() completes, the sink clears _p, put_W resolves, and the shared state resolves BOTH waiters. R resumes, retries SSL_read_ex -> bio_write_ex now idle -> issues put_R into a clear sink. W resumes and continues. One put() at a time. A write can also enter do_put() with a put already in flight, for the same reason R does above: the read path emits a key-update put while the write holds the disjoint _out_sem. That needs no up-front drain -- bio_write_ex() declines while a put is in flight (BIO_set_retry_write), so the first SSL_write_ex reports WANT_WRITE and handle_do_put_ssl_err() drains and retries, exactly as later loop iterations do. Remove the SEASTAR_ASSERT(_output_pending.available()) precondition at the start of do_put(). It asserted "no put is in flight when a write begins", which is false for the reason above: _in_sem and _out_sem are disjoint, so a write taking _out_sem does not exclude a read-path put. The assert only held before because the std::exchange() swap left _output_pending spuriously available -- the same falsification behind the bug -- so with the swap gone it would now fire on exactly the scenario being fixed. The decline/retry path is the correct handling. The destructor's available() assert is kept -- it is still true once close() has drained output, and a failed _output_pending is also available(). The GnuTLS backend (src/net/tls.cc) is unaffected: its read path never emits output, so the concurrent put cannot occur there. On an output failure _output_pending is intentionally left failed and acts as a circuit breaker -- it is not reset. The _output_pending data member carries a detailed comment covering the locking model, the serialization protocol, why the future->shared_future change is the minimal correct fix (rather than adding a lock), how handed-out waiters still resolve after reassignment, and the failure behavior. With this change test_concurrent_put_with_key_update passes, and the full tls_test suite passes under the OpenSSL backend (the new test is compiled out under GnuTLS). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
dotnwat
added a commit
that referenced
this pull request
Jun 11, 2026
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. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
dotnwat
added a commit
that referenced
this pull request
Jun 11, 2026
Backport of #281 (merged to v26.2.x) to v26.1.x. On v26.2.x the OpenSSL backend lives in src/net/tls_openssl.cc; on this branch it is src/net/ossl.cc, but the buggy pattern and the fix are the same. A data_sink permits only one put() in flight at a time (posix_data_sink_impl asserts !_p). That contract was being violated because wait_for_output() did std::exchange(_output_pending, make_ready_future()) to obtain something to await, which marked _out "idle" while the put() was still draining. bio_write_ex()'s guard then issued a second, concurrent put() from the read path while a write-path put() was in flight, tripping the assert (reproduced by the preceding commit). This is not a missing lock: the reactor is single-threaded and the bio_write_ex() guard is already atomic. It was the guard reading falsified state. Make _output_pending a shared_future so that wait_for_output() can hand out an awaitable via get_future() without consuming or replacing the in-flight put; available()/failed() then stay truthful and both the read and write paths can await the same put. The interleaving, with a write fiber W (do_put, holding _out_sem) and a read fiber R (do_get, holding _in_sem); R reaches bio_write_ex because SSL_read_ex emits a key-update response while processing an incoming record ("|" marks a reactor switch between the fibers): BEFORE, when wait_for_output() did std::exchange(_output_pending, make_ready_future()): W SSL_write_ex -> bio_write_ex: _output_pending idle, so issue put_W; the sink sets _p and starts write_all() (put_W now UNRESOLVED). W co_await wait_for_output() evaluates the exchange synchronously: it swaps _output_pending to a *ready* future, then parks W on put_W. | put_W's write_all() is still draining, yet _output_pending now reads "available". R SSL_read_ex -> bio_write_ex: _output_pending available, so issue put_R -> posix_data_sink_impl::put() trips SEASTAR_ASSERT(!_p), because _p still holds put_W's packet. *** assertion failure *** AFTER, with wait_for_output() == _output_pending.get_future() (no swap): W bio_write_ex issues put_W; _output_pending = shared_future(put_W), which stays UNRESOLVED while write_all() drains. W co_await wait_for_output() takes a get_future() on put_W and parks W on it, leaving _output_pending untouched. | put_W still draining; _output_pending correctly reads "not available". R SSL_read_ex -> bio_write_ex: _output_pending not available, so decline (BIO_set_retry_write) -- no second put(). SSL_read_ex returns WANT_WRITE; do_get co_awaits wait_for_output(), a SECOND get_future() on the SAME put_W, and parks R on it too. | write_all() completes, the sink clears _p, put_W resolves, and the shared state resolves BOTH waiters. R resumes, retries SSL_read_ex -> bio_write_ex now idle -> issues put_R into a clear sink. W resumes and continues. One put() at a time. A write can also enter do_put_one() with a put already in flight, for the same reason R does above: the read path emits a key-update put while the write holds the disjoint _out_sem. That needs no up-front drain -- bio_write_ex() declines while a put is in flight (BIO_set_retry_write), so the first SSL_write_ex reports WANT_WRITE and handle_do_put_ssl_err() drains and retries, exactly as later loop iterations do. Remove the SEASTAR_ASSERT(_output_pending.available()) preconditions at the start of the two do_put() overloads and do_put_one(). They asserted "no put is in flight when a write begins", which is false for the reason above: _in_sem and _out_sem are disjoint, so a write taking _out_sem does not exclude a read-path put. The assert only held before because the std::exchange() swap left _output_pending spuriously available -- the same falsification behind the bug -- so with the swap gone it would now fire on exactly the scenario being fixed. The decline/retry path is the correct handling; the two overloads just delegate to do_put_one(), so their asserts are simply dropped. The destructor's available() assert is kept -- it is still true once close() has drained output, and a failed _output_pending is also available(). (The GnuTLS backend in src/net/tls.cc keeps its copies of these asserts: its read path never emits output, so the precondition still holds there.) On an output failure _output_pending is intentionally left failed and acts as a circuit breaker -- it is not reset. The _output_pending data member carries a detailed comment covering the locking model, the serialization protocol, why the future->shared_future change is the minimal correct fix (rather than adding a lock), how handed-out waiters still resolve after reassignment, and the failure behavior. With this change test_concurrent_put_with_key_update passes, and the full tls_test suite passes under the OpenSSL backend (the new test is compiled out under GnuTLS). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
dotnwat
added a commit
that referenced
this pull request
Jun 11, 2026
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>
dotnwat
added a commit
that referenced
this pull request
Jun 12, 2026
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>
dotnwat
added a commit
that referenced
this pull request
Jun 12, 2026
Backport of #281 (merged to v26.2.x) to v25.3.x. On v26.2.x the OpenSSL backend lives in src/net/tls_openssl.cc and splits the write path into two do_put() overloads plus do_put_one(); on this branch it is src/net/ossl.cc with a single do_put(net::packet). The buggy pattern and the fix are otherwise the same. A data_sink permits only one put() in flight at a time (posix_data_sink_impl asserts !_p). That contract was being violated because wait_for_output() did std::exchange(_output_pending, make_ready_future()) to obtain something to await, which marked _out "idle" while the put() was still draining. bio_write_ex()'s guard then issued a second, concurrent put() from the read path while a write-path put() was in flight, tripping the assert (reproduced by the preceding commit). This is not a missing lock: the reactor is single-threaded and the bio_write_ex() guard is already atomic. It was the guard reading falsified state. Make _output_pending a shared_future so that wait_for_output() can hand out an awaitable via get_future() without consuming or replacing the in-flight put; available()/failed() then stay truthful and both the read and write paths can await the same put. The interleaving, with a write fiber W (do_put, holding _out_sem) and a read fiber R (do_get, holding _in_sem); R reaches bio_write_ex because SSL_read_ex emits a key-update response while processing an incoming record ("|" marks a reactor switch between the fibers): BEFORE, when wait_for_output() did std::exchange(_output_pending, make_ready_future()): W SSL_write_ex -> bio_write_ex: _output_pending idle, so issue put_W; the sink sets _p and starts write_all() (put_W now UNRESOLVED). W co_await wait_for_output() evaluates the exchange synchronously: it swaps _output_pending to a *ready* future, then parks W on put_W. | put_W's write_all() is still draining, yet _output_pending now reads "available". R SSL_read_ex -> bio_write_ex: _output_pending available, so issue put_R -> posix_data_sink_impl::put() trips SEASTAR_ASSERT(!_p), because _p still holds put_W's packet. *** assertion failure *** AFTER, with wait_for_output() == _output_pending.get_future() (no swap): W bio_write_ex issues put_W; _output_pending = shared_future(put_W), which stays UNRESOLVED while write_all() drains. W co_await wait_for_output() takes a get_future() on put_W and parks W on it, leaving _output_pending untouched. | put_W still draining; _output_pending correctly reads "not available". R SSL_read_ex -> bio_write_ex: _output_pending not available, so decline (BIO_set_retry_write) -- no second put(). SSL_read_ex returns WANT_WRITE; do_get co_awaits wait_for_output(), a SECOND get_future() on the SAME put_W, and parks R on it too. | write_all() completes, the sink clears _p, put_W resolves, and the shared state resolves BOTH waiters. R resumes, retries SSL_read_ex -> bio_write_ex now idle -> issues put_R into a clear sink. W resumes and continues. One put() at a time. A write can also enter do_put() with a put already in flight, for the same reason R does above: the read path emits a key-update put while the write holds the disjoint _out_sem. That needs no up-front drain -- bio_write_ex() declines while a put is in flight (BIO_set_retry_write), so the first SSL_write_ex reports WANT_WRITE and handle_do_put_ssl_err() drains and retries, exactly as later loop iterations do. Remove the SEASTAR_ASSERT(_output_pending.available()) precondition at the start of do_put(). It asserted "no put is in flight when a write begins", which is false for the reason above: _in_sem and _out_sem are disjoint, so a write taking _out_sem does not exclude a read-path put. The assert only held before because the std::exchange() swap left _output_pending spuriously available -- the same falsification behind the bug -- so with the swap gone it would now fire on exactly the scenario being fixed. The decline/retry path is the correct handling. The destructor's available() assert is kept -- it is still true once close() has drained output, and a failed _output_pending is also available(). The GnuTLS backend (src/net/tls.cc) is unaffected: its read path never emits output, so the concurrent put cannot occur there. On an output failure _output_pending is intentionally left failed and acts as a circuit breaker -- it is not reset. The _output_pending data member carries a detailed comment covering the locking model, the serialization protocol, why the future->shared_future change is the minimal correct fix (rather than adding a lock), how handed-out waiters still resolve after reassignment, and the failure behavior. With this change test_concurrent_put_with_key_update passes, and the full tls_test suite passes under the OpenSSL backend (the new test is compiled out under GnuTLS). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
dotnwat
added a commit
that referenced
this pull request
Jun 12, 2026
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
added a commit
that referenced
this pull request
Jun 12, 2026
Backport of #281 (merged to v26.2.x) to v26.1.x. On v26.2.x the OpenSSL backend lives in src/net/tls_openssl.cc; on this branch it is src/net/ossl.cc, but the buggy pattern and the fix are the same. A data_sink permits only one put() in flight at a time (posix_data_sink_impl asserts !_p). That contract was being violated because wait_for_output() did std::exchange(_output_pending, make_ready_future()) to obtain something to await, which marked _out "idle" while the put() was still draining. bio_write_ex()'s guard then issued a second, concurrent put() from the read path while a write-path put() was in flight, tripping the assert (reproduced by the preceding commit). This is not a missing lock: the reactor is single-threaded and the bio_write_ex() guard is already atomic. It was the guard reading falsified state. Make _output_pending a shared_future so that wait_for_output() can hand out an awaitable via get_future() without consuming or replacing the in-flight put; available()/failed() then stay truthful and both the read and write paths can await the same put. The interleaving, with a write fiber W (do_put, holding _out_sem) and a read fiber R (do_get, holding _in_sem); R reaches bio_write_ex because SSL_read_ex emits a key-update response while processing an incoming record ("|" marks a reactor switch between the fibers): BEFORE, when wait_for_output() did std::exchange(_output_pending, make_ready_future()): W SSL_write_ex -> bio_write_ex: _output_pending idle, so issue put_W; the sink sets _p and starts write_all() (put_W now UNRESOLVED). W co_await wait_for_output() evaluates the exchange synchronously: it swaps _output_pending to a *ready* future, then parks W on put_W. | put_W's write_all() is still draining, yet _output_pending now reads "available". R SSL_read_ex -> bio_write_ex: _output_pending available, so issue put_R -> posix_data_sink_impl::put() trips SEASTAR_ASSERT(!_p), because _p still holds put_W's packet. *** assertion failure *** AFTER, with wait_for_output() == _output_pending.get_future() (no swap): W bio_write_ex issues put_W; _output_pending = shared_future(put_W), which stays UNRESOLVED while write_all() drains. W co_await wait_for_output() takes a get_future() on put_W and parks W on it, leaving _output_pending untouched. | put_W still draining; _output_pending correctly reads "not available". R SSL_read_ex -> bio_write_ex: _output_pending not available, so decline (BIO_set_retry_write) -- no second put(). SSL_read_ex returns WANT_WRITE; do_get co_awaits wait_for_output(), a SECOND get_future() on the SAME put_W, and parks R on it too. | write_all() completes, the sink clears _p, put_W resolves, and the shared state resolves BOTH waiters. R resumes, retries SSL_read_ex -> bio_write_ex now idle -> issues put_R into a clear sink. W resumes and continues. One put() at a time. A write can also enter do_put_one() with a put already in flight, for the same reason R does above: the read path emits a key-update put while the write holds the disjoint _out_sem. That needs no up-front drain -- bio_write_ex() declines while a put is in flight (BIO_set_retry_write), so the first SSL_write_ex reports WANT_WRITE and handle_do_put_ssl_err() drains and retries, exactly as later loop iterations do. Remove the SEASTAR_ASSERT(_output_pending.available()) preconditions at the start of the two do_put() overloads and do_put_one(). They asserted "no put is in flight when a write begins", which is false for the reason above: _in_sem and _out_sem are disjoint, so a write taking _out_sem does not exclude a read-path put. The assert only held before because the std::exchange() swap left _output_pending spuriously available -- the same falsification behind the bug -- so with the swap gone it would now fire on exactly the scenario being fixed. The decline/retry path is the correct handling; the two overloads just delegate to do_put_one(), so their asserts are simply dropped. The destructor's available() assert is kept -- it is still true once close() has drained output, and a failed _output_pending is also available(). (The GnuTLS backend in src/net/tls.cc keeps its copies of these asserts: its read path never emits output, so the precondition still holds there.) On an output failure _output_pending is intentionally left failed and acts as a circuit breaker -- it is not reset. The _output_pending data member carries a detailed comment covering the locking model, the serialization protocol, why the future->shared_future change is the minimal correct fix (rather than adding a lock), how handed-out waiters still resolve after reassignment, and the failure behavior. With this change test_concurrent_put_with_key_update passes, and the full tls_test suite passes under the OpenSSL backend (the new test is compiled out under GnuTLS). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
dotnwat
added a commit
that referenced
this pull request
Jun 12, 2026
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>
This was referenced Jun 23, 2026
dotnwat
added a commit
that referenced
this pull request
Jun 24, 2026
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
added a commit
that referenced
this pull request
Jun 24, 2026
Backport of #281 (merged to v26.2.x) to v26.1.x. On v26.2.x the OpenSSL backend lives in src/net/tls_openssl.cc; on this branch it is src/net/ossl.cc, but the buggy pattern and the fix are the same. A data_sink permits only one put() in flight at a time (posix_data_sink_impl asserts !_p). That contract was being violated because wait_for_output() did std::exchange(_output_pending, make_ready_future()) to obtain something to await, which marked _out "idle" while the put() was still draining. bio_write_ex()'s guard then issued a second, concurrent put() from the read path while a write-path put() was in flight, tripping the assert (reproduced by the preceding commit). This is not a missing lock: the reactor is single-threaded and the bio_write_ex() guard is already atomic. It was the guard reading falsified state. Make _output_pending a shared_future so that wait_for_output() can hand out an awaitable via get_future() without consuming or replacing the in-flight put; available()/failed() then stay truthful and both the read and write paths can await the same put. The interleaving, with a write fiber W (do_put, holding _out_sem) and a read fiber R (do_get, holding _in_sem); R reaches bio_write_ex because SSL_read_ex emits a key-update response while processing an incoming record ("|" marks a reactor switch between the fibers): BEFORE, when wait_for_output() did std::exchange(_output_pending, make_ready_future()): W SSL_write_ex -> bio_write_ex: _output_pending idle, so issue put_W; the sink sets _p and starts write_all() (put_W now UNRESOLVED). W co_await wait_for_output() evaluates the exchange synchronously: it swaps _output_pending to a *ready* future, then parks W on put_W. | put_W's write_all() is still draining, yet _output_pending now reads "available". R SSL_read_ex -> bio_write_ex: _output_pending available, so issue put_R -> posix_data_sink_impl::put() trips SEASTAR_ASSERT(!_p), because _p still holds put_W's packet. *** assertion failure *** AFTER, with wait_for_output() == _output_pending.get_future() (no swap): W bio_write_ex issues put_W; _output_pending = shared_future(put_W), which stays UNRESOLVED while write_all() drains. W co_await wait_for_output() takes a get_future() on put_W and parks W on it, leaving _output_pending untouched. | put_W still draining; _output_pending correctly reads "not available". R SSL_read_ex -> bio_write_ex: _output_pending not available, so decline (BIO_set_retry_write) -- no second put(). SSL_read_ex returns WANT_WRITE; do_get co_awaits wait_for_output(), a SECOND get_future() on the SAME put_W, and parks R on it too. | write_all() completes, the sink clears _p, put_W resolves, and the shared state resolves BOTH waiters. R resumes, retries SSL_read_ex -> bio_write_ex now idle -> issues put_R into a clear sink. W resumes and continues. One put() at a time. A write can also enter do_put_one() with a put already in flight, for the same reason R does above: the read path emits a key-update put while the write holds the disjoint _out_sem. That needs no up-front drain -- bio_write_ex() declines while a put is in flight (BIO_set_retry_write), so the first SSL_write_ex reports WANT_WRITE and handle_do_put_ssl_err() drains and retries, exactly as later loop iterations do. Remove the SEASTAR_ASSERT(_output_pending.available()) preconditions at the start of the two do_put() overloads and do_put_one(). They asserted "no put is in flight when a write begins", which is false for the reason above: _in_sem and _out_sem are disjoint, so a write taking _out_sem does not exclude a read-path put. The assert only held before because the std::exchange() swap left _output_pending spuriously available -- the same falsification behind the bug -- so with the swap gone it would now fire on exactly the scenario being fixed. The decline/retry path is the correct handling; the two overloads just delegate to do_put_one(), so their asserts are simply dropped. The destructor's available() assert is kept -- it is still true once close() has drained output, and a failed _output_pending is also available(). (The GnuTLS backend in src/net/tls.cc keeps its copies of these asserts: its read path never emits output, so the precondition still holds there.) On an output failure _output_pending is intentionally left failed and acts as a circuit breaker -- it is not reset. The _output_pending data member carries a detailed comment covering the locking model, the serialization protocol, why the future->shared_future change is the minimal correct fix (rather than adding a lock), how handed-out waiters still resolve after reassignment, and the failure behavior. With this change test_concurrent_put_with_key_update passes, and the full tls_test suite passes under the OpenSSL backend (the new test is compiled out under GnuTLS). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
dotnwat
added a commit
that referenced
this pull request
Jun 30, 2026
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>
dotnwat
added a commit
that referenced
this pull request
Jun 30, 2026
Backport of #281 (merged to v26.2.x) to v25.3.x. On v26.2.x the OpenSSL backend lives in src/net/tls_openssl.cc and splits the write path into two do_put() overloads plus do_put_one(); on this branch it is src/net/ossl.cc with a single do_put(net::packet). The buggy pattern and the fix are otherwise the same. A data_sink permits only one put() in flight at a time (posix_data_sink_impl asserts !_p). That contract was being violated because wait_for_output() did std::exchange(_output_pending, make_ready_future()) to obtain something to await, which marked _out "idle" while the put() was still draining. bio_write_ex()'s guard then issued a second, concurrent put() from the read path while a write-path put() was in flight, tripping the assert (reproduced by the preceding commit). This is not a missing lock: the reactor is single-threaded and the bio_write_ex() guard is already atomic. It was the guard reading falsified state. Make _output_pending a shared_future so that wait_for_output() can hand out an awaitable via get_future() without consuming or replacing the in-flight put; available()/failed() then stay truthful and both the read and write paths can await the same put. The interleaving, with a write fiber W (do_put, holding _out_sem) and a read fiber R (do_get, holding _in_sem); R reaches bio_write_ex because SSL_read_ex emits a key-update response while processing an incoming record ("|" marks a reactor switch between the fibers): BEFORE, when wait_for_output() did std::exchange(_output_pending, make_ready_future()): W SSL_write_ex -> bio_write_ex: _output_pending idle, so issue put_W; the sink sets _p and starts write_all() (put_W now UNRESOLVED). W co_await wait_for_output() evaluates the exchange synchronously: it swaps _output_pending to a *ready* future, then parks W on put_W. | put_W's write_all() is still draining, yet _output_pending now reads "available". R SSL_read_ex -> bio_write_ex: _output_pending available, so issue put_R -> posix_data_sink_impl::put() trips SEASTAR_ASSERT(!_p), because _p still holds put_W's packet. *** assertion failure *** AFTER, with wait_for_output() == _output_pending.get_future() (no swap): W bio_write_ex issues put_W; _output_pending = shared_future(put_W), which stays UNRESOLVED while write_all() drains. W co_await wait_for_output() takes a get_future() on put_W and parks W on it, leaving _output_pending untouched. | put_W still draining; _output_pending correctly reads "not available". R SSL_read_ex -> bio_write_ex: _output_pending not available, so decline (BIO_set_retry_write) -- no second put(). SSL_read_ex returns WANT_WRITE; do_get co_awaits wait_for_output(), a SECOND get_future() on the SAME put_W, and parks R on it too. | write_all() completes, the sink clears _p, put_W resolves, and the shared state resolves BOTH waiters. R resumes, retries SSL_read_ex -> bio_write_ex now idle -> issues put_R into a clear sink. W resumes and continues. One put() at a time. A write can also enter do_put() with a put already in flight, for the same reason R does above: the read path emits a key-update put while the write holds the disjoint _out_sem. That needs no up-front drain -- bio_write_ex() declines while a put is in flight (BIO_set_retry_write), so the first SSL_write_ex reports WANT_WRITE and handle_do_put_ssl_err() drains and retries, exactly as later loop iterations do. Remove the SEASTAR_ASSERT(_output_pending.available()) precondition at the start of do_put(). It asserted "no put is in flight when a write begins", which is false for the reason above: _in_sem and _out_sem are disjoint, so a write taking _out_sem does not exclude a read-path put. The assert only held before because the std::exchange() swap left _output_pending spuriously available -- the same falsification behind the bug -- so with the swap gone it would now fire on exactly the scenario being fixed. The decline/retry path is the correct handling. The destructor's available() assert is kept -- it is still true once close() has drained output, and a failed _output_pending is also available(). The GnuTLS backend (src/net/tls.cc) is unaffected: its read path never emits output, so the concurrent put cannot occur there. On an output failure _output_pending is intentionally left failed and acts as a circuit breaker -- it is not reset. The _output_pending data member carries a detailed comment covering the locking model, the serialization protocol, why the future->shared_future change is the minimal correct fix (rather than adding a lock), how handed-out waiters still resolve after reassignment, and the failure behavior. With this change test_concurrent_put_with_key_update passes, and the full tls_test suite passes under the OpenSSL backend (the new test is compiled out under GnuTLS). Co-Authored-By: Claude Opus 4.8 (1M context) <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
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.
Fix for https://redpandadata.atlassian.net/browse/CORE-16383