fix: S26.02 MbedTlsStream Open unwinds inner transport and SSL state on failure#440
Merged
Conversation
Slice 1 of S26.02 (#420). Pure additive: new file-scope return value, Reset clears it, setter installs the value, mbedtls_ssl_config_defaults fake returns it instead of hard-coded 0. No production caller yet.
…faults fails Slice 2 of S26.02 (#420). When MbedTlsStream_Open's first allocating step (mbedtls_ssl_config_defaults) fails, the inner transport stayed open and the next StreamSender reconnect tick re-entered the transport's Open, clobbering the prior fd. Two changes close the leak: - ApplySslConfigDefaults emits MBEDTLSSTREAM_ERROR_DEFAULTS_NOT_APPLIED via SolidSyslog_Error at severity ERROR so the integrator sees a protocol-level diagnostic rather than a silent failure. - Open's failure tail calls MbedTlsStream_Close, which unwinds both the SSL state (ssl_free + ssl_config_free) and the inner transport (a no-op if Transport.Open was never reached). Close is idempotent: eager mbedtls_ssl_init / mbedtls_ssl_config_init in Initialise guarantees the *_free calls are safe even when no allocating step succeeded, and Transport.Close is guarded inside the transport. Tests 2-5 in subsequent slices pin the same shape for the remaining failure points.
Slice 3 of S26.02 (#420). Same pure-additive shape as slice 1: new file-scope return value defaulting to 0 (success), Reset clears it, setter installs the value, mbedtls_ssl_setup fake returns it instead of hard-coded 0. No production caller yet.
Slice 4 of S26.02 (#420). BindContextToConfig now emits MBEDTLSSTREAM_ERROR_SESSION_INIT_FAILED via SolidSyslog_Error at severity ERROR when mbedtls_ssl_setup returns non-zero. The unwind itself (Close on Open failure) already covers this path from slice 2 — this slice only adds the protocol-level diagnostic. Refactor: with two failure-point tests sharing an identical 8-line assertion block, extracted CHECK_OPEN_UNWOUND_WITH_ERROR(transport, expectedCode) macro near the top of the test file. The macro pins the four-resource unwind (transport closed, both mbedTLS structs freed) plus the error tuple (source, code, severity). Tests 1 and 2 collapse to two lines each; future failure-point tests reuse the same shape.
Slice 5 of S26.02 (#420). Final fake setter needed for the per-step unwind audit. Same pure-additive shape as slices 1 and 3: new file-scope return value defaulting to 0 (success), Reset clears it, setter installs the value, mbedtls_ssl_set_hostname fake returns it instead of hard-coded 0. No production caller yet.
Slice 6 of S26.02 (#420). ConfigureExpectedHostname now emits MBEDTLSSTREAM_ERROR_SERVER_NAME_NOT_SET via SolidSyslog_Error at severity ERROR when mbedtls_ssl_set_hostname returns non-zero. The emission stays inside the ServerName != NULL guard — a caller who never sets ServerName never reaches mbedtls_ssl_set_hostname, so no spurious error is reported. Unwind already in place from slice 2. Test 3 uses ReCreateHandleWithUpdatedConfig to set config.ServerName before the new Create. The helper now also re-creates the transport, re-installs ErrorHandlerFake, and resets MbedTlsFake — so tests using it (including CHECK_OPEN_UNWOUND_WITH_ERROR's == 1 assertions) observe counts from the post-ReCreate Open onwards only. The existing five ReCreate callers were unaffected: each only asserted counts produced by the new Open, so the extra reset is invisible to them.
Slice 7 of S26.02 (#420). Split PerformHandshake's combined "not-retryable OR budget-exhausted" exit branch into two distinct arms so each exit can emit its own protocol-level diagnostic. The hard-error arm (non-WANT mbedTLS return) emits MBEDTLSSTREAM_ERROR_HANDSHAKE_REJECTED — covers the common cases (peer rejected, cert chain untrusted, protocol incompatible, server name mismatch) which all surface as a single fatal handshake rc to the client side. The budget-exhausted arm is left silent for now; slice 8 fills in HANDSHAKE_TIMEOUT. Test 4 (renamed from OpenFailsImmediatelyOnHardSslError to the OpenClosesTransportAndFreesSslState convention) keeps the existing once-only handshake and never-Sleep assertions and adds CHECK_OPEN_UNWOUND_WITH_ERROR to pin the unwind and emission.
Slice 8 of S26.02 (#420). The budget-exhausted exit branch from slice 7's PerformHandshake refactor now emits MBEDTLSSTREAM_ERROR_HANDSHAKE_TIMEOUT — distinct from HANDSHAKE_REJECTED so the integrator can tell a wedged peer (no progress within 5 s) from a peer that actively rejected the handshake. Same severity (ERROR) and same unwind path (slice 2's tail Close). Test 5 (renamed from OpenFailsWhenHandshakeNeverCompletes to the OpenClosesTransportAndFreesSslState convention) gets the CHECK_OPEN_UNWOUND_WITH_ERROR assertion. Completes the audit of all five Open-failure points (DEFAULTS_NOT_APPLIED, SESSION_INIT_FAILED, SERVER_NAME_NOT_SET, HANDSHAKE_REJECTED, HANDSHAKE_TIMEOUT).
Slice 9 of S26.02 (#420). Pins the recovery contract that the per- failure-point unwinds (slices 2, 4, 6, 7, 8) 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 MbedTlsFake_SetSslHandshakeReturnSequence to make the first handshake fail (hard error, fail-fast) and the second succeed. Asserts Transport.OpenCount == 2, Transport.CloseCount == 1, second Open returns true. Passed on arrival — no production change. Completes the 6-test contract called out in the story acceptance criteria.
Slice 10 of S26.02 (#420). MbedTlsStream.c grew by 50 lines in slices 2-8 (new emissions in ApplySslConfigDefaults, BindContextToConfig, ConfigureExpectedHostname, and two PerformHandshake exit branches; unwind tail in Open). All six existing suppressions for the file moved to their new line numbers: 60 -> 64 (11.3, SelfFromBase cast) 190 -> 240 (11.5, BioSend ctx cast) 202 -> 252 (11.5, BioRecv ctx cast) 227 -> 277 (11.5, Send buffer cast) 246 -> 296 (11.5, Read buffer cast) 14 -> 18 (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.
Contributor
📝 WalkthroughWalkthroughMbedTlsStream_Open now explicitly unwinds resources (transport, SSL config, SSL context) on every failure path after allocation, emits five new protocol-level error codes via error handlers at each failure point, and is verified by test cases that inject failures at each stage and assert full unwinding and error emission. ChangesS26.02: MbedTlsStream Open() failure unwinding and error reporting
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 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 docstrings
🧪 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 |
Contributor
☀️ Quality Summary 🚦 build-linux-gcc: 100% successful (✔️ 1300 passed, 🙈 2 skipped) Created by Quality Monitor v1.14.0 (#f3859fd). More details are shown in the GitHub Checks Result. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@misra_suppressions.txt`:
- Line 47: The suppression list contains a stale entry for misra-c2012-5.7 at
line 18 that no longer corresponds to a cppcheck-reported violation; open
misra_suppressions.txt and remove or update the misra-c2012-5.7 entry (line 18)
so it no longer suppresses a non-existent issue, and while you’re there verify
that the misra-c2012-11.3 entry referencing
Platform/MbedTls/Source/SolidSyslogMbedTlsStream.c and the misra-c2012-11.5
entries (lines 240, 252, 277, 296) accurately match the current cppcheck
findings — adjust file paths or line numbers to the actual locations reported by
cppcheck or add a brief comment explaining why each suppression is still
required.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6ae79ecf-3339-4102-8550-df5b30245004
📒 Files selected for processing (8)
DEVLOG.mdPlatform/MbedTls/Interface/SolidSyslogMbedTlsStreamErrors.hPlatform/MbedTls/Source/SolidSyslogMbedTlsStream.cPlatform/MbedTls/Source/SolidSyslogMbedTlsStreamMessages.cTests/MbedTls/SolidSyslogMbedTlsStreamTest.cppTests/Support/MbedTlsFake.cTests/Support/MbedTlsFake.hmisra_suppressions.txt
7 tasks
This was referenced May 30, 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 #420.
Summary
if (!ok) MbedTlsStream_Close(base);closes the inner transport and frees both mbedTLS structs (ssl_free + ssl_config_free). Close is idempotent — the eagermbedtls_ssl_init/_config_initin Initialise guarantees the frees are safe whether or not Open reached its allocating step.SolidSyslogMbedTlsStreamErrors.h(DEFAULTS_NOT_APPLIED, 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") not C-call-level. Pattern to be reused by S26.03 (OpenSSL parity, deferred until this story merges).else ifarms so each emits its own typed code.CHECK_OPEN_UNWOUND_WITH_ERROR(transport, code)macro extracted at slice 4 once duplication became obvious. Existing testsOpenFailsImmediatelyOnHardSslErrorandOpenFailsWhenHandshakeNeverCompleteswere renamed to theOpenClosesTransportAndFreesSslStateWhen…convention while keeping their original retry/sleep assertions.Story scope
Audit of the path from
StreamSender.Sendthrough MbedTlsStream down to the socket found three resources that leaked on Open failure:mbedtls_ssl_setup— only freed bymbedtls_ssl_freeinside Close.mbedtls_ssl_config_defaults— only freed bymbedtls_ssl_config_freeinside Close.All three close with the tail
Closecall on the failure path.Test plan
HandshakeFailsWhenServerCertSignedByUntrustedCaunderulimit -n 32— all pass, no EMFILEOut of scope
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests