Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix socket descriptor checks on Windows #24035

Closed
wants to merge 1 commit into from

Conversation

olszomal
Copy link
Contributor

@olszomal olszomal commented Apr 4, 2024

The socket range checks introduced in e98c735 break non-blocking socket operations on Windows. This causes random WSAENOTCONN errors in subsequent read/write attempts.

  1. In the implementation of the socket() function in Windows, the nfds parameter is ignored and included only for compatibility with Berkeley sockets. On Windows, the FD_SETSIZE value contains the maximum total number of sockets in fd_set rather than the limit for for FD_SET() and FD_CLR() macros.
    https://learn.microsoft.com/en-us/windows/win32/api/winsock2/nf-winsock2-select

  2. Also, the Winsock invalid SOCKET value is INVALID_SOCKET, and not any integer < 0 as on Unix.
    https://learn.microsoft.com/en-us/windows/win32/winsock/socket-data-type-2

My PR fixes checking Windows socket value according to Microsoft documentation.

@t8m t8m added branch: master Merge to master branch approval: review pending This pull request needs review by a committer triaged: bug The issue/pr is/fixes a bug branch: 3.0 Merge to openssl-3.0 branch branch: 3.1 Merge to openssl-3.1 tests: exempted The PR is exempt from requirements for testing branch: 3.2 Merge to openssl-3.2 branch: 3.3 Merge to openssl-3.3 labels Apr 4, 2024
@olszomal
Copy link
Contributor Author

olszomal commented Apr 5, 2024

I could not reproduce the testing framework failure.

$ perl configdata.pm --dump
    /usr/bin/perl ./Configure --banner=Configured --debug -O1 -fsanitize=memory -DOSSL_SANITIZE_MEMORY -fno-optimize-sibling-calls enable-rc5 enable-md2 enable-ec_nistp_64_gcc_128
(...)

$ make test TESTS="test_tls13messages"
(...)
70-test_tls13messages.t .. ok    
All tests successful.
Files=1, Tests=17,  3 wallclock secs ( 0.23 usr  0.00 sys +  1.90 cusr  0.50 csys =  2.63 CPU)
Result: PASS

Could it be an issue with the testing framework itself?

@t8m t8m added approval: done This pull request has the required number of approvals and removed approval: review pending This pull request needs review by a committer labels Apr 5, 2024
@openssl-machine openssl-machine added approval: ready to merge The 24 hour grace period has passed, ready to merge and removed approval: done This pull request has the required number of approvals labels Apr 6, 2024
@openssl-machine
Copy link
Collaborator

This pull request is ready to merge

@t8m
Copy link
Member

t8m commented Apr 10, 2024

Merged to all the active branches. Thank you for your contribution.

@t8m t8m closed this Apr 10, 2024
openssl-machine pushed a commit that referenced this pull request Apr 10, 2024
Reviewed-by: Neil Horman <nhorman@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from #24035)
openssl-machine pushed a commit that referenced this pull request Apr 10, 2024
Reviewed-by: Neil Horman <nhorman@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from #24035)

(cherry picked from commit c89baf8)
openssl-machine pushed a commit that referenced this pull request Apr 10, 2024
Reviewed-by: Neil Horman <nhorman@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from #24035)

(cherry picked from commit c89baf8)
openssl-machine pushed a commit that referenced this pull request Apr 10, 2024
Reviewed-by: Neil Horman <nhorman@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from #24035)

(cherry picked from commit c89baf8)
openssl-machine pushed a commit that referenced this pull request Apr 10, 2024
Reviewed-by: Neil Horman <nhorman@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from #24035)

(cherry picked from commit c89baf8)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approval: ready to merge The 24 hour grace period has passed, ready to merge branch: master Merge to master branch branch: 3.0 Merge to openssl-3.0 branch branch: 3.1 Merge to openssl-3.1 branch: 3.2 Merge to openssl-3.2 branch: 3.3 Merge to openssl-3.3 tests: exempted The PR is exempt from requirements for testing triaged: bug The issue/pr is/fixes a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants