Fix: [Network] Error handling on Windows broken #9116
Merged
Conversation
This file contains 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
rubidium42
added
the
backport requested
TrueBrain
reviewed
Apr 27, 2021
rubidium42
force-pushed
the
winsock-uses-no-errno
branch
2 times, most recently
from
5e4f1c1
to
51ba093
Compare
TrueBrain
reviewed
Apr 27, 2021
PeterN
reviewed
Apr 27, 2021
…numbers that need to be looked up
…twork errors They are likely not working as expected on Windows, so prevent their usage. Winsock does not set errno and strerror does not return anything useful for Winsock error numbers.
rubidium42
force-pushed
the
winsock-uses-no-errno
branch
from
51ba093
to
52a6fb9
Compare
TrueBrain
approved these changes
Apr 27, 2021
LordAro
reviewed
Apr 27, 2021
LordAro
added
backported
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Motivation / Problem
Testing #9112 on Windows I found out it did not work. Looking at the specs I found out that errno is not useful for errors from Winsock; that's where GET_LAST_ERROR() was supposed to be used. However, strerror is not useful for errors from Winsock either and that has no abstraction, so most error messages on Windows from the network would be useless.
Description
Use GET_LAST_ERROR(), although renamed to NetworkGetLastError() to make it clearer it is only to be used for network errors, where errno was used in the network code.
Introduce NetworkGetLastErrorString which is strerror(NetworkGetLastError()) for everything but Windows; note that errno on OS/2 is also not used for socket errors, so also for OS/2 GET_LAST_ERROR() should have been used.
For Windows introduce NetworkGetLastErrorString that returns an error string using Windows' FormatMessage.
To prevent issues in the future, safeguards were added to not use strerror or errno in the networking code. "In the networking code" is everything that includes network/core/os_abstraction.h. That should generally be safe as, in theory, nothing outside the network could should touch that (with proper encapsulation ofcourse). However, it was leaking out through some header files, e.g. fios.h so some small changes were made to prevent leaking os_abstraction.h.
Limitations
Cannot use strerror/errno anymore when including os_abstraction.h (and safeguards.h), though if you need to there is probably some issue with encapsulation and the place that (indirectly) includes that knows way too much of the internal state of the network code.
Checklist for review
Some things are not automated, and forgotten often. This list is a reminder for the reviewers.