[v26.1.x] net/tls: Fix concurrent put() on the socket in the OpenSSL backend#291
Merged
Merged
Conversation
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 #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>
There was a problem hiding this comment.
Pull request overview
Backport to v26.1.x that fixes a crash in the OpenSSL TLS backend caused by two concurrent data_sink::put() calls (one from the write path and one emitted from SSL_read_ex() during TLS 1.3 key-update handling). The fix ensures both paths correctly serialize on the same in-flight socket write.
Changes:
- Add a deterministic unit test reproducer (
test_concurrent_put_with_key_update) using an instrumented loopback sink to detect overlappingput()calls. - Fix the OpenSSL backend by changing
_output_pendingfromfuture<>toshared_future<>and awaiting viaget_future()to avoid “consuming” the in-flight put. - Add a minimal OpenSSL-only, test-only hook (
trigger_key_update_for_test) to drive TLS 1.3 key updates on this branch.
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 reproducer test that detects concurrent underlying-socket put() calls during key updates. |
src/net/ossl.cc |
Fixes the concurrent put() bug by making _output_pending a shared_future<>, updating wait_for_output(), and adding a test-only key-update trigger. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
2503
to
+2506
| try { | ||
| size_t n; | ||
|
|
||
| // Skip issuing if a previous put() failed; the failed _output_pending is |
1 task
dotnwat
requested review from
StephanDollberg,
joe-redpanda,
pgellert and
travisdowns
pgellert
approved these changes
Jun 24, 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) tov26.1.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.#287(holding the reproducer'sgate_statebylw_shared_ptrinstead of on the test fiber's stack, fixing a use-after-return that ASan traps) is folded directly into the reproducer here, so the test is correct from the first commit.Backport notes (v26.2.x → v26.1.x)
v26.2.xthe OpenSSL backend lives insrc/net/tls_openssl.cc; onv26.1.xit issrc/net/ossl.cc. The buggy pattern and the fix are the same.tls::force_rehandshake()onv26.2.x, which is implemented there. Onv26.1.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
v26.1.xwith the OpenSSL backend (--crypto-provider OpenSSL --api-level 8):test_concurrent_put_with_key_updatepasses.tls_testsuite passes with no regressions.🤖 Generated with Claude Code