set CLOEXEC on diagnostic server FDs #55850
Merged
Conversation
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
stephentoub
approved these changes
Jul 16, 2021
lateralusX
added a commit
to lateralusX/runtime
that referenced
this pull request
Jul 22, 2021
…ort. Up until dotnet#55850 it was possible to build using diagnostic server socket PAL on Windows for local testing. NOTE, this is not a config we build on CI since it is not used by any products (diagnostic server PAL on Windows uses NamedPipes), but useful for local testing. The same PR also included calls to fcntl using FD_CLOEXEC in case platform doesn't define SOCK_CLOEXEC. Since this is a Linux specific flag, it might not be defined on several targeted platforms. Failures calling fcntl using FD_CLOEXEC was also triggering an EP_ASSERT but that is only on debug builds. Most calls to fcntl using FD_CLOEXEC in runtime PAL layers (both CoreCLR and MonoVM) doesn't check for errors in this case and sees the operation as best effort, like done here https://github.com/dotnet/runtime/blob/b937677e8f8601848d29bc072a93cc0c6e21576d/src/libraries/Native/Unix/System.Native/pal_networking.c#L2499. We should do the same in ds-ipc-pal-socket.c making sure it won't break any potential platform not fully supporting it or if we are really concerned about this error, handle it as real error failing creation of socket, but that needs to be tested on mobile platforms (not running TCP/IP PAL on CI).
lateralusX
added a commit
that referenced
this pull request
Jul 26, 2021
…ort. (#56141) Up until #55850 it was possible to build using diagnostic server socket PAL on Windows for local testing. NOTE, this is not a config we build on CI since it is not used by any products (diagnostic server PAL on Windows uses NamedPipes), but useful for local testing. The same PR also included calls to fcntl using FD_CLOEXEC in case platform doesn't define SOCK_CLOEXEC. Since this is a Linux specific flag, it might not be defined on several targeted platforms. Failures calling fcntl using FD_CLOEXEC was also triggering an EP_ASSERT but that is only on debug builds. Most calls to fcntl using FD_CLOEXEC in runtime PAL layers (both CoreCLR and MonoVM) doesn't check for errors in this case and sees the operation as best effort, like done here https://github.com/dotnet/runtime/blob/b937677e8f8601848d29bc072a93cc0c6e21576d/src/libraries/Native/Unix/System.Native/pal_networking.c#L2499. We should do the same in ds-ipc-pal-socket.c making sure it won't break any potential platform not fully supporting it or if we are really concerned about this error, handle it as real error failing creation of socket, but that needs to be tested on mobile platforms (not running TCP/IP PAL on CI).
ghost
locked as resolved and limited conversation to collaborators
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
This sets the
CLOEXECflag on socket file descriptors created in the diagnostic server. It attempts to set it directly in the call tosocketif supported, otherwise it usesfcntl.