Improve error handling in SecureAccept and SsecureConnect - #3468
Merged
Conversation
14 tasks
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughSummary by CodeRabbit
WalkthroughChangesSSL handshake error propagation
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant SecureConnect
participant SSL_Connect
participant MbedTLS
participant ExceptionMapper
SecureConnect->>SSL_Connect: start handshake with mbedtlsCode
SSL_Connect->>MbedTLS: perform TLS handshake
MbedTLS-->>SSL_Connect: handshake status and error code
SSL_Connect-->>SecureConnect: return SslError
SecureConnect->>ExceptionMapper: map non-success SslError
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
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 |
![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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/PAL/COM/sockets/Sockets_debugger.cpp (1)
462-482: 🩺 Stability & Availability | 🔴 Critical | 🏗️ Heavy liftDo not remove the non-blocking retry loop.
By removing the
do/whileloop that checked for blocking states (SOCK_EWOULDBLOCK/SOCK_TRY_AGAIN), the SSL handshake for the debugger will immediately fail and abruptly close the socket if it cannot complete in a single pass.TLS handshakes require multiple round trips, so non-blocking sockets will invariably return a "would block" state. You must restore a retry loop to allow the handshake to continue, ensuring that blocking states are properly intercepted rather than treated as a fatal
sslErr != SslError_None.🤖 Prompt for 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. In `@src/PAL/COM/sockets/Sockets_debugger.cpp` around lines 462 - 482, Restore the non-blocking retry loop around SSL_Connect in the debugger SSL handshake, continuing while the result represents SOCK_EWOULDBLOCK or SOCK_TRY_AGAIN and only treating other non-success results as fatal. Keep socket/context cleanup after the retry loop and preserve m_usingSSL assignment only for a successful handshake.
🤖 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
`@src/DeviceInterfaces/System.Net/sys_net_native_System_Net_Security_SslNative.cpp`:
- Around line 187-190: Remove the stale sslErr < 0 check immediately following
WaitEvents in the retry loop, since sslErr is not updated there and the
condition can prematurely terminate retries. Preserve the loop’s existing retry
behavior for SOCK_EWOULDBLOCK and SOCK_TRY_AGAIN.
In `@src/PAL/COM/sockets/ssl/MbedTLS/ssl_accept_internal.cpp`:
- Line 30: Update the context initialization and sanity check in the SSL accept
function to validate both context and context->ssl before dereferencing either.
Return SslError_HandshakeBadContext when either pointer is NULL, then assign ssl
only after the combined check, matching the safe flow used by
ssl_connect_internal.cpp.
- Around line 51-59: Restore non-blocking mode before handshake failure returns:
in ssl_accept_internal.cpp lines 51-59 and ssl_connect_internal.cpp lines 58-66,
set nonblock to 1 and call SOCK_ioctl in each failure path, including
certificate verification failure, before returning the corresponding SslError.
---
Outside diff comments:
In `@src/PAL/COM/sockets/Sockets_debugger.cpp`:
- Around line 462-482: Restore the non-blocking retry loop around SSL_Connect in
the debugger SSL handshake, continuing while the result represents
SOCK_EWOULDBLOCK or SOCK_TRY_AGAIN and only treating other non-success results
as fatal. Keep socket/context cleanup after the retry loop and preserve
m_usingSSL assignment only for a successful handshake.
🪄 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: Repository YAML (base), Central YAML (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7b773199-86e7-4eba-9164-42b54a9e2834
📒 Files selected for processing (9)
src/DeviceInterfaces/System.Net/sys_net_native.hsrc/DeviceInterfaces/System.Net/sys_net_native_System_Net_Security_SslNative.cppsrc/PAL/COM/sockets/Sockets_debugger.cppsrc/PAL/COM/sockets/ssl/MbedTLS/ssl_accept_internal.cppsrc/PAL/COM/sockets/ssl/MbedTLS/ssl_connect_internal.cppsrc/PAL/COM/sockets/ssl/ssl.cppsrc/PAL/COM/sockets/ssl/ssl_functions.hsrc/PAL/COM/sockets/ssl/ssl_stubs.cppsrc/PAL/Include/nanoPAL_Sockets.h
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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
`@src/DeviceInterfaces/System.Net/sys_net_native_System_Net_Security_SslNative.cpp`:
- Around line 91-97: The blocking handshake paths no longer use timeout stack
state. In SecureAccept at
src/DeviceInterfaces/System.Net/sys_net_native_System_Net_Security_SslNative.cpp
lines 91-97, remove the timeout_ms/hbTimeout setup, the unused timeout handling,
and the Timeout stack pop; apply the same removal of timeout/hbTimeout setup,
unused timeout handling, and stack pop in SecureConnect at lines 164-170,
leaving the blocking SSL handshake calls intact.
- Around line 531-533: Update the SslError switch in the SSL initialization
function so the default branch explicitly returns failure instead of breaking
and continuing to stack.SetResult_I4(sslContext). Preserve the existing handling
for recognized SslError values and ensure unrecognized or future errors cannot
return the invalid SSL context as success.
🪄 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: Repository YAML (base), Central YAML (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: a5099a69-baec-4cbb-9f1c-2820bf7b6e0c
📒 Files selected for processing (2)
src/DeviceInterfaces/System.Net/sys_net_native_System_Net_Security_SslNative.cppsrc/PAL/COM/sockets/ssl/MbedTLS/ssl_accept_internal.cpp
- Now returning detailed SSL error and mbedTLS error code. - Update declarations and callers. - Update return checks to use the new codes. - Update enum declaration in System.Net.
josesimoes
force-pushed
the
fix-tls-error-parsing
branch
from
July 15, 2026 11:06
17c0c2a to
a5b5252
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@src/PAL/COM/sockets/Sockets_debugger.cpp`:
- Around line 462-482: Remove the outer do/while retry loop and its accompanying
non-blocking socket comment around SSL_Connect. Invoke SSL_Connect once,
preserving mbedtlsCode and sslErr for the existing terminal error handling
because ssl_connect_internal now performs the handshake retries internally.
In `@src/PAL/COM/sockets/ssl/MbedTLS/ssl_connect_internal.cpp`:
- Around line 29-32: Extend the initial validation in ssl_connect_internal to
also check context->ssl for NULL before calling mbedtls_ssl_set_hostname. Return
SslError_HandshakeBadContext for either invalid context or SSL state, matching
the safety behavior of ssl_accept_internal.
🪄 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: Repository YAML (base), Central YAML (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: 930339ea-d878-4aa3-85fe-6a94a303f6b7
📒 Files selected for processing (9)
src/DeviceInterfaces/System.Net/sys_net_native.hsrc/DeviceInterfaces/System.Net/sys_net_native_System_Net_Security_SslNative.cppsrc/PAL/COM/sockets/Sockets_debugger.cppsrc/PAL/COM/sockets/ssl/MbedTLS/ssl_accept_internal.cppsrc/PAL/COM/sockets/ssl/MbedTLS/ssl_connect_internal.cppsrc/PAL/COM/sockets/ssl/ssl.cppsrc/PAL/COM/sockets/ssl/ssl_functions.hsrc/PAL/COM/sockets/ssl/ssl_stubs.cppsrc/PAL/Include/nanoPAL_Sockets.h
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
2 participants
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.
Description
Motivation and Context
How Has This Been Tested?
Screenshots
Types of changes
Checklist