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: add CLOEXEC flag to socket created directly #550

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

smira
Copy link
Contributor

@smira smira commented Oct 1, 2024

Go runtime adds CLOEXEC flags automatically, but when using raw syscalls, we have to do it manually.

Without this flag, file descriptors leak to the child processes.

Go runtime adds `CLOEXEC` flags automatically, but when using raw
syscalls, we have to do it manually.

Without this flag, file descriptors leak to the child processes.

Signed-off-by: Andrey Smirnov <[email protected]>
@smira smira changed the title fix: add CLOEXEC flag to socket creates directly fix: add CLOEXEC flag to socket created directly Oct 1, 2024
Copy link
Collaborator

@Natolumin Natolumin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch! An inline note about the workaround where I would rather avoid messing with fork as much as possible

return fd, nil
}

if err == unix.EINVAL || err == unix.EPROTONOSUPPORT {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

EINVAL seems awfully wide an error to catch for this I think, especially with having to take the forklock in the fallback case.
Can this be further narrowed per-platform with build tags? Afaict linux and all the go-supported bsds support SOCK_CLOEXEC, so the only actually relevant platform would be darwin, non-illumos solaris (before 11.4 according to net source ), maybe AIX?
It looks like GOOS=darwin doesn't even define unix.SOCK_CLOEXEC so this workaround won't even compile there (would it be necessary? I'm not sure)

I'd be inclined to not support it at all, I don't think any of these platforms are tested at all anyway

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Darwin should support CLOEXEC I believe starting with some version, but I haven't tested it.

There's a similar workaround in Go runtime, and this code is loosely based on https://github.com/mdlayher/socket/blob/9c51a391be6c6bc5b7ce52f328497931c5e97733/conn.go#L254-L303

But I'm happy to drop it and simply do | unix.O_CLOEXEC

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, SOCK_CLOEXEC is missing on Darwin, but other constants are also missing, so not sure how much relevant that is:

# github.com/insomniacslk/dhcp/dhcpv4/nclient4
dhcpv4/nclient4/conn_unix.go:42:59: undefined: unix.ETH_P_IP

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Natolumin do you want me to simplify and just add CLOEXEC directly?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants