Conversation
Use the SO_ERROR result for retry classification and error reporting instead of a stale thread-local socket error. Add a regression test covering the false-success sequence.
No functional change. Pass the -1 explicitly to bio_socket_should_retry and document that it classifies the thread's last socket error rather than its argument, so the preceding bio_socket_set_error call is clearly load-bearing. Fold WaitForConnect into WaitForSocket as a kConnect wait type, and give that wait a longer timeout: Windows takes ~2s to report a refused loopback connection, which left little margin against the previous 5s. Explain why the SYS reason code is only checked off Windows, and sort the socket_helper.c include block.
Reading SO_ERROR to diagnose the failure also clears it, so the connect state machine stayed in BIO_CONN_S_BLOCKED_CONNECT and the next BIO_do_connect saw a clear socket error and transitioned to BIO_CONN_S_OK. A caller polling BIO_do_connect would see 0 once and then 1, on a connection that never completed and with an empty error queue. Add a terminal BIO_CONN_S_ERROR state so the failure sticks until BIO_CTRL_RESET. Repeat attempts, and reads and writes, now keep reporting failure. No system error is pushed on the repeat path because errno no longer describes the original failure. Extend the regression test to cover the repeated call and a subsequent write.
Contributor
|
🔒 Security Review — View Report Please review before merging. |
prasden
self-requested a review
August 31, 2026 17:57
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3459 +/- ##
==========================================
+ Coverage 78.06% 78.08% +0.01%
==========================================
Files 700 700
Lines 124705 124743 +38
Branches 17326 17326
==========================================
+ Hits 97352 97403 +51
+ Misses 26484 26469 -15
- Partials 869 871 +2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
prasden
reviewed
Sep 9, 2026
Comment on lines
+214
to
+217
| // |bio_socket_should_retry| classifies the thread's last socket | ||
| // error, not its argument, so publish |i| first. | ||
| bio_socket_set_error(i); | ||
| if (bio_socket_should_retry(-1)) { |
Contributor
There was a problem hiding this comment.
NP: seems a bit awkward to pass in -1 into bio_socket_should_retry just to get the errno right after setting bio_socket_set_error(i) above.
Another internal method like bio_errno_is_retryable to distinguish from failures like ECONNREFUSED could help.
Comment on lines
191
to
195
| OPENSSL_PUT_SYSTEM_ERROR(); | ||
| OPENSSL_PUT_ERROR(BIO, BIO_R_CONNECT_ERROR); | ||
| ERR_add_error_data(4, "host=", c->param_hostname, ":", | ||
| c->param_port); | ||
| } |
Contributor
There was a problem hiding this comment.
Should we set c-> state = BIO_CONN_S_ERROR here as well?
prasden
approved these changes
Sep 9, 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
3 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.
Context and motivation
A non-blocking
BIO_s_connectcould report success after the peer had already refused the connection.BIO_do_connectclassified the completed attempt using the thread-local socket error left by the originalEINPROGRESS/WSAEWOULDBLOCKconnect(), so a realSO_ERRORfailure was treated as "try again." ReadingSO_ERRORalso clears it, so the nextBIO_do_connectsaw a clean socket and transitioned toBIO_CONN_S_OK. A caller pollingBIO_do_connectwould observe 0 once and then 1, on a socket that never connected and with an empty error queue.Description of changes
Classify the blocked-connect path from the
SO_ERRORvalue, not the leftover thread-local error.getsockoptfailure is now a hard error (-1) rather than a fake errno 1.Add a terminal
BIO_CONN_S_ERRORstate. Once the failure has been reported, furtherBIO_do_connect/ read / write calls keep failing untilBIO_reset. Repeat attempts only pushBIO_R_NBIO_CONNECT_ERROR: the originalSO_ERRORwas consumed anderrnono longer describes it.Testing
BIOTest.SocketNonBlockingConnectFailuredrives a non-blocking connect at a just-released loopback port, plants a stale retryable socket error after the attempt completes, and checks the first hard failure, a secondBIO_do_connect(must not flip to success), and a subsequent write.Off Windows the test also checks
ECONNREFUSEDas theSYSreason. Winsock codes do not surviveERR_GET_REASON's 12-bit pack, so that assert is POSIX-only.Review considerations
No public API or ABI change. The new state is internal;
bio_info_cbmay now observe it. Not in the FIPS module boundary.WaitForSocketnow has akConnectmode that also watchesPOLLERR/POLLHUP(and the Windows exception set) and uses a 30s timeout. Windows can take ~2s to refuse a loopback connect.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and the ISC license.