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: use SOCK_CLOEXEC with FD_CLOEXEC fallback #14672

Merged
merged 6 commits into from
Jun 14, 2024

Conversation

ysbaddaden
Copy link
Contributor

@ysbaddaden ysbaddaden commented Jun 6, 2024

Harmonizes the implementations between Darwin and other POSIX platforms for the "close on exec" behavior.

When SOCK_CLOEXEC is available, we always use it in the socket, socketpair and accept4 syscalls. When SOCK_CLOEXEC isn't available, we don't delay to Socket#initialize_handle anymore to set FD_CLOEXEC for Darwin only, but immediately call fcntl to set it after the above syscalls.

The accept4 syscall is non-standard but widely available: Linux, all supported BSD, except for Darwin (obviously).

The patch also fixes an issue where TCP and UNIX client sockets didn't have FD_CLOEXEC on POSIX platforms... except for Darwin.

closes #14650

Harmonizes the implementations between Darwin and other POSIX platforms.
Now we try to use `SOCK_CLOEXEC` in the `socket`, `socketpair` and
`accept4` syscalls.

The `accept4` syscall is non-standard but widely available: Linux, all
supported BSD, except for Darwin (obviously).

When `SOCK_CLOEXEC` isn't available, we don't delay to
`Socket#initialize_handle` anymore to set `FD_CLOEXEC` for Darwin only,
but immediately call `fcntl` to set it after the above syscalls.

The patch also fixes an issue where TCP and UNIX client sockets didn't
have `FD_CLOEXEC` on all POSIX platforms... except for Darwin.
@ysbaddaden
Copy link
Contributor Author

Follow up: we'll want to prevent fork when SOCK_CLOEXEC isn't available (i.e. Darwin only) with a readers-writer-lock, so we don't hit a race where thread 1 creates a socket while thread 2 forks then execs, which would leak the file descriptor to the executed process (oops).

Prior art: Go is doing that.

@ysbaddaden ysbaddaden marked this pull request as ready for review June 6, 2024 17:34
@ysbaddaden
Copy link
Contributor Author

ysbaddaden commented Jun 7, 2024

suggestion: add/rework specs.

@ysbaddaden ysbaddaden self-assigned this Jun 7, 2024
No need to check for SOCK_CLOEXEC since there is no point in
implementing the accept4 syscall without this feature.
@straight-shoota straight-shoota added this to the 1.13.0 milestone Jun 13, 2024
spec/std/socket/unix_socket_spec.cr Outdated Show resolved Hide resolved
@ysbaddaden ysbaddaden merged commit a269524 into crystal-lang:master Jun 14, 2024
61 checks passed
@ysbaddaden ysbaddaden deleted the fix/sock-cloexec branch June 14, 2024 10:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants