[v25.3.x] net/tls: Fix concurrent put() on the socket in the OpenSSL backend#283
Closed
dotnwat wants to merge 2 commits into
Closed
[v25.3.x] net/tls: Fix concurrent put() on the socket in the OpenSSL backend#283dotnwat wants to merge 2 commits into
dotnwat wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
Backports the OpenSSL TLS backend fix for a production crash caused by concurrent put() calls on the underlying socket when SSL_read_ex() emits key-update output while an application write is in flight. The change makes output-drain tracking shareable so both read and write paths can safely await the same in-flight socket write, and adds a targeted unit test reproducer.
Changes:
- Replace
_output_pendingwithshared_future<>and adjustwait_for_output()/ BIO write logic to avoid “early ready” state while a put is still draining. - Add a test-only OpenSSL hook to trigger TLS 1.3 key updates needed by the reproducer (not exposed via public headers).
- Add a new unit test
test_concurrent_put_with_key_updateto detect overlappingput()calls on the underlying sink.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/net/ossl.cc |
Switch output tracking to shared_future<>, update BIO write/await logic, and add a test-only key-update trigger hook. |
tests/unit/tls_test.cc |
Add an OpenSSL-only reproducer that detects overlapping put() calls using an instrumented loopback sink. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+2436
to
+2438
| // Give the write path time to reach the point where it would issue the | ||
| // colliding put(). The read's put stays held, so the window stays open. | ||
| seastar::sleep(std::chrono::milliseconds(200)).get(); |
Comment on lines
+2288
to
+2294
| future<> trigger_key_update_for_test(connected_socket& socket) { | ||
| auto* impl = net::get_impl::maybe_get_ptr(socket); | ||
| auto* tls_impl = dynamic_cast<tls_connected_socket_impl*>(impl); | ||
| SEASTAR_ASSERT(tls_impl); | ||
| auto* sess = static_cast<session*>(tls_impl->_session.get()); | ||
| return sess->trigger_key_update_for_test(); | ||
| } |
dotnwat
force-pushed
the
fix-openssl-tls-concurrent-put-v25.3.x
branch
from
4f261de to
0a852fa
Compare
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>
dotnwat
force-pushed
the
fix-openssl-tls-concurrent-put-v25.3.x
branch
from
0a852fa to
b4349ca
Compare
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 (merged to
v26.2.x) tov25.3.x.Fixes the production crash from https://redpandadata.atlassian.net/browse/CORE-16383:
The bug
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.wait_for_output()didstd::exchange(_output_pending, make_ready_future()), which marked_out"idle" while the put() was still draining;bio_write_ex()'s guard then issued a second, concurrentput()from the read path while a write-pathput()was in flight, trippingSEASTAR_ASSERT(!_p)inposix_data_sink_impl.The fix makes
_output_pendingashared_futuresowait_for_output()hands out an awaitable viaget_future()without consuming/replacing the in-flight put;available()/failed()then stay truthful and both paths can await the same put. The staleSEASTAR_ASSERT(_output_pending.available())precondition indo_put()is removed. Full rationale (with the interleaving) is in the commit messages.Commits
test_concurrent_put_with_key_update, which fails (overlap detected) against the current backend.shared_futurechange insrc/net/ossl.cc.Backport notes
v26.2.xthe OpenSSL backend lives insrc/net/tls_openssl.ccand splits the write path into twodo_put()overloads plusdo_put_one(); onv25.3.xit issrc/net/ossl.ccwith a singledo_put(net::packet). The buggy pattern and fix are otherwise identical.v26.2.xthe reproducer drives key-updates through the publictls::force_rehandshake()(implemented there viaSSL_key_update). Onv25.3.xthere is no rehandshake/key-update entry point at all (force_rehandshake()does not exist on this branch). Rather than add that production API, this PR 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. No public API or production behavior changes.SEASTAR_USE_GNUTLS), is compiled out under GnuTLS, and its instrumenteddata_sinkoverridesput(net::packet)to match this branch's (older)data_sinkAPI.Validation
--crypto-provider OpenSSL --api-level=7,dev): reproducer fails before the fix (overlap detected) and passes after; the fulltls_testsuite passes (49 cases).🤖 Generated with Claude Code