fix: S26.03 TlsStream Open unwinds inner transport and SSL state on failure#442
Merged
Conversation
Contributor
|
Warning Review limit reached
Your plan currently allows 1 review/hour. Refill in 51 minutes and 43 seconds. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more review capacity refills, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than trial, open-source, and free plans. In all cases, review capacity refills continuously over time. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughThis PR implements open-failure unwinding and per-failure-point error reporting for both MbedTLS and OpenSSL TLS stream implementations. When ChangesTLS stream open unwinds and per-failure-point error codes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Slice 1 of S26.03 (#441). Five new codes in TlsStreamErrors.h mirroring the S26.02 mbedTLS pattern (CONTEXT_INIT_FAILED, SESSION_INIT_FAILED, SERVER_NAME_NOT_SET, HANDSHAKE_REJECTED, HANDSHAKE_TIMEOUT) and matching protocol-level messages in TlsStreamMessages.c. No emit sites yet; subsequent slices add the per-helper emissions plus the Open failure-tail unwind.
… fails Slice 2 of S26.03 (#441). Mirrors S26.02's per-helper emit + tail unwind pattern. When TlsStream_InitSslContext returns false (covers SSL_CTX_new allocation failure plus every sub-step inside ConfigureSslContext — trust anchors, client identity, protocol floor, cipher list), InitSslContext emits TLSSTREAM_ERROR_CONTEXT_INIT_FAILED at severity ERROR. TlsStream_Open's failure tail now calls TlsStream_Close + TlsStream_ReleaseSslContext, releasing the inner transport plus any Ssl / BioMethod / Ctx allocated before the failure. Both helpers are NULL-guarded so the unwind is safe regardless of which step tripped (matches the S26.02 idempotent-Close pattern). Test side: introduces CHECK_OPEN_UNWOUND_WITH_ERROR(transport, code) macro covering the 5 universal post-failure assertions (transport closed once, ErrorHandler called once, source, code, severity). Per- mode SSL-resource-free assertions stay in the existing XxxFailureFreesCtx tests because their counts vary by failure depth. ErrorHandlerFake_Install added to setup() so every test starts with a fresh handler — unaffected tests don't observe it. Subsequent slices (3-7) extend the remaining per-failure tests with the macro and add the per-helper emissions for the other 4 codes.
… macro Slice 3 of S26.03 (#441). Each of the 8 existing per-failure-mode tests inside InitSslContext's scope (LoadVerifyLocations, MinProto, CipherListRejected, OnlyClientCert, OnlyClientKey, UseCertChainFile, UsePrivateKeyFile, CheckPrivateKey) now asserts the full unwind + error contract via CHECK_OPEN_UNWOUND_WITH_ERROR. All pass on arrival — slice 2's per-helper emit in InitSslContext + tail unwind in Open covers every CONTEXT_INIT sub-failure uniformly. Also added ReCreateStreamWithUpdatedConfig() helper to TEST_GROUP, mirroring S26.02's MbedTls equivalent. Replaces inlined Destroy/recreate-stream sequences in the six tests that need a config tweak (CipherList, ClientCertChainPath, ClientKeyPath). The helper also destroys and recreates the transport + resets OpenSslFake + reinstalls ErrorHandlerFake, so post-recreate count assertions (== 1) observe only the new Open.
Slice 4 of S26.03 (#441). Both per-helper emit sites (InitSslSession's SSL_new and AttachTransportBio's BIO_meth_new / BIO_new) now emit the shared TLSSTREAM_ERROR_SESSION_INIT_FAILED code at severity ERROR. The shared code reflects the protocol-level meaning ("TLS session could not be initialised against the configured context") — the integrator does not need to distinguish SSL allocation from BIO allocation at this level. Slice 2's tail unwind already covers the resource release on both paths. Three existing tests extended with the macro to pin the contract: OpenReturnsFalseWhenSslNewFails, WhenBioMethNewFails, WhenBioNewFails.
Slice 5 of S26.03 (#441). ConfigureExpectedHostname emits TLSSTREAM_ERROR_SERVER_NAME_NOT_SET at severity ERROR when either SSL_set_tlsext_host_name or SSL_set1_host fails. Emission stays inside the ServerName != NULL guard — a caller who never sets ServerName never reaches these calls, so no spurious error reported. Slice 2's tail unwind already covers the resource release. Two existing tests (WhenSet1HostFails, WhenSniHostnameSetupFails) extended with the macro + switched to ReCreateStreamWithUpdatedConfig for the ServerName injection.
…branch Slice 6+7 of S26.03 (#441). Combined into one commit because the production change is a single PerformHandshake refactor — splitting the combined "not-retryable OR budget-exhausted" exit branch into two distinct arms so each exit emits its own protocol-level code: - Hard SSL error (non-WANT) → TLSSTREAM_ERROR_HANDSHAKE_REJECTED. Covers peer rejection, cert chain untrusted, server name mismatch, protocol-version incompatibility — every fatal handshake outcome the client sees as a single non-retryable rc. - Budget exhausted (persistent WANT_READ/WRITE for HANDSHAKE_TIMEOUT_ MILLISECONDS) → TLSSTREAM_ERROR_HANDSHAKE_TIMEOUT. Distinct from REJECTED so the integrator can tell a wedged peer from one that actively rejected. Three tests extended with the macro: - OpenReturnsFalseWhenHandshakeFails — pinned to HANDSHAKE_REJECTED (forced via SSL_get_error returning SSL_ERROR_SSL). - OpenFailsWhenHandshakeNeverCompletes — pinned to HANDSHAKE_TIMEOUT (persistent WANT_READ). - OpenFailsImmediatelyOnHardSslError — pinned to HANDSHAKE_REJECTED; keeps its existing once-only Connect and never-Sleep assertions. Mirrors S26.02 slices 7+8.
Slice 8 of S26.03 (#441). Mirrors S26.02 slice 9. Pins the recovery contract that the per-failure-point unwinds enable: after a failed Open, the next Open is a clean Open-Close-Open cycle on the inner transport. Without the unwind tail added in slice 2, the inner transport would stay open and the next StreamSender reconnect tick would re-enter PosixTcpStream_Open, clobbering the prior fd. Arrangement uses OpenSslFake_SetConnectReturnSequence([fail, ok]) plus a SetGetErrorReturn flip to drive the first handshake to a hard error (fail-fast) and the second to success. Asserts Transport.OpenCount == 2, Transport.CloseCount == 1, second Open returns true. Passed on arrival — no production change.
Slice 9 of S26.03 (#441). TlsStream.c grew by 24 lines in slices 2-7 (per-helper emits in InitSslContext, InitSslSession, AttachTransportBio, ConfigureExpectedHostname, two PerformHandshake exit branches; unwind tail in Open). Two existing suppressions for the file moved to new line numbers: 70 -> 74 (11.3, SelfFromBase cast) 16 -> 20 (5.7, anonymous-enum opener) No new suppressions; no new MISRA violations introduced by the unwind or the emission helpers. Verified with cppcheck-misra against the full CI include set — clean exit, matches main baseline.
DavidCozens
force-pushed
the
fix/s26-03-openssl-open-unwind
branch
from
fec8b6a to
9b2437d
Compare
Contributor
☀️ Quality Summary 🚦 build-linux-gcc: 100% successful (✔️ 1301 passed, 🙈 2 skipped) Created by Quality Monitor v1.14.0 (#f3859fd). More details are shown in the GitHub Checks Result. |
This was referenced Jun 2, 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
1 participant
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.
Closes #441. OpenSSL parity of S26.02 (#420 / merged in PR #440), using the same test pattern and protocol-level error-message style.
Summary
TlsStream_Opennow unwinds on every failure path after the first allocating step: a tailif (!ok) { TlsStream_Close(base); TlsStream_ReleaseSslContext(self); }closes the inner transport and releases theSsl/BioMethod/Ctxallocations (each NULL-guarded so the unwind is safe regardless of which step tripped).SolidSyslogTlsStreamErrors.h(CONTEXT_INIT_FAILED, SESSION_INIT_FAILED, SERVER_NAME_NOT_SET, HANDSHAKE_REJECTED, HANDSHAKE_TIMEOUT) emitted viaSolidSyslog_Errorat severity ERROR. Messages are protocol-level — "TLS handshake rejected by peer or local verification; connection not established" rather than naming the OpenSSL call.else ifarms so each emits its own typed code.CHECK_OPEN_UNWOUND_WITH_ERROR(transport, code)macro that pins the full unwind + error contract. 1 new recovery test (SecondOpenAfterFailedFirstOpenSucceeds).TlsStream.cshifted to their new line numbers (70→74 for 11.3, 16→20 for 5.7). No new suppressions; clean against the full CI include set.Granularity choice (parity with S26.02)
OpenSSL has more configurable surfaces in
SSL_CTXthan mbedTLS does (SSL_CTX_load_verify_locations,SSL_CTX_use_certificate_chain_file, etc., all returningint). A literal one-code-per-failing-API expansion would have given 8-9 codes. Consolidated underCONTEXT_INIT_FAILEDto match the MbedTls adapter's diagnostic granularity. Finer codes (TRUST_ANCHORS_NOT_LOADED, CLIENT_IDENTITY_INVALID, CIPHER_LIST_REJECTED) are a future enhancement story if integrators ask for the granularity.Test plan
SolidSyslogTestsaggregate (1294 tests) +OpenSslIntegrationTests(9 live OpenSSL tests) green underclang-debugsanitize(ASan + UBSan), no leaks/UBHandshakeRejectedWhenClientDoesNotTrustServerCertunderulimit -n 32— all pass, no EMFILEOut of scope
PosixTcpStream_Open(same argument as S26.02: contract is "every Stream_Open is preceded by Stream_Close", honoured by StreamSender's disconnect-before-reconnect)🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes