[v25.3.x] net/tls: Fix concurrent put() on the socket in the OpenSSL backend#292
Merged
Merged
Conversation
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 #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>
There was a problem hiding this comment.
Pull request overview
Backport to v25.3.x of the OpenSSL TLS backend fix that prevents concurrent data_sink::put() calls on the underlying socket when OpenSSL emits write-side TLS records from within the read path (e.g., TLS 1.3 key update responses), which previously could trip SEASTAR_ASSERT(!_p) in posix_data_sink_impl.
Changes:
- Add an OpenSSL-only unit test reproducer (
test_concurrent_put_with_key_update) that detects overlappingput()calls on the underlying sink. - Fix the OpenSSL backend output-serialization logic by tracking
_output_pendingas ashared_future, so both read/write paths can await the same in-flight socketput()without falsely marking output idle. - Add a minimal OpenSSL-only, test-only hook (
tls::trigger_key_update_for_test) to drive TLS 1.3 key updates on this branch where the public API is unavailable.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
tests/unit/tls_test.cc |
Adds an OpenSSL-only regression test that deterministically detects overlapping socket put() calls during TLS 1.3 key updates. |
src/net/ossl.cc |
Switches _output_pending to shared_future and adjusts wait_for_output()/bio_write_ex() to prevent concurrent socket writes; adds test-only key-update trigger. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+2421
to
2428
| // 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. | ||
| if (!session->_output_pending.available()) { | ||
| tls_log.trace("{} bio_write_ex: nothing pending in output", *session); | ||
| tls_log.trace("{} bio_write_ex: put still in flight", *session); | ||
| BIO_set_retry_write(b); | ||
| return 0; | ||
| } |
1 task
pgellert
approved these changes
Jun 25, 2026
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.
Backport of #281 and #287 (both merged to
v26.2.x) tov25.3.x.Fix for https://redpandadata.atlassian.net/browse/CORE-16383
What this fixes
The OpenSSL TLS read and write paths use separate semaphores (
_in_semvs_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 insideSSL_read_ex. When a read emits output while a write'sput()is still in flight, the TLS layer issues a second, concurrentput()on the samedata_sink, which tripsSEASTAR_ASSERT(!_p)inposix_data_sink_impl.The root cause is that
wait_for_output()didstd::exchange(_output_pending, make_ready_future())to obtain something to await, which marked_out"idle" while theput()was still draining.bio_write_ex()'s guard then issued a second, concurrentput()from the read path. The fix makes_output_pendingashared_futuresowait_for_output()hands out an awaitable viaget_future()without consuming or replacing the in-flightput();available()/failed()then stay truthful and both paths can await the sameput(). The now-falseSEASTAR_ASSERT(_output_pending.available())write-path preconditions are removed (the decline/retry path viaBIO_set_retry_writeis the correct handling).Structure
Two commits, mirroring the combined content of the two upstream PRs:
#281reproducer +#287's lifetime fix folded in) —test_concurrent_put_with_key_update.#281production fix) insrc/net/ossl.cc.#287holds the reproducer'sgate_statebylw_shared_ptr(co-owned by the instrumented sink) instead of on the test fiber's stack. The sink decrements the gate from aput().finally()that can run on the reactor after theSEASTAR_THREAD_TEST_CASEfiber has returned and freed its stack — a use-after-return that ASan (detect_stack_use_after_return=1) traps as a SEGV. This is the bug that failed the debug+OpenSSL CI job on the original v25.3.x backport (#283), so the fix is folded directly into the reproducer here.Backport notes (v26.2.x → v25.3.x)
v26.2.xthe OpenSSL backend lives insrc/net/tls_openssl.cc; onv25.3.xit issrc/net/ossl.cc. The buggy pattern and the fix are the same.tls::force_rehandshake()onv26.2.x. Onv25.3.xforce_rehandshake()is unimplemented for the OpenSSL backend, so rather than backport that production feature this adds a minimal, test-only hook —session::trigger_key_update_for_test()plus a free functiontls::trigger_key_update_for_test(connected_socket&), neither declared in any public header — that performs just theSSL_key_update()the test needs.SEASTAR_USE_GNUTLS) rather than via the runtimetls::backend_name()used onv26.2.x's dual-backend build; it is compiled out under GnuTLS (whose read path never emits output, so the bug does not occur there).Validation
Built on current
v25.3.xwith the OpenSSL backend (--crypto-provider OpenSSL --api-level 7):test_concurrent_put_with_key_updatepasses; fulltls_testsuite passes.detect_stack_use_after_return=1):test_concurrent_put_with_key_updateand the fulltls_testsuite pass with no SEGV — confirming the#287lifetime fix on the configuration that originally failed CI.🤖 Generated with Claude Code