fix: add CLOEXEC flag to socket created directly by smira · Pull Request #550 · insomniacslk/dhcp

smira · 2024-10-01T12:29:03Z

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.

Natolumin

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

Natolumin · 2024-10-01T13:21:42Z

internal/xsocket/xsocket.go

+		return fd, nil
+	}
+
+	if err == unix.EINVAL || err == unix.EPROTONOSUPPORT {


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

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

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

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

@smira I'm running into this not working on darwin whilst working on siderolabs/talos#10537. FD_CLOEXEC / O_NOINHERIT should be available on nearly all systems so you could refactor this to use them or add CLOEXEC directly

Since this is closely based on the version from mdlayher/socket, and we already import that library in this one, can we simply use that socket.Socket implementation in here instead of copying it?

While I'm not an active maintainer, I don't think anyone here wants to have to maintain cross-platform socket-API level code for platforms that most of the maintainers don't use and can't test on, so if we can use that upstream lib implementation this would be much easier to merge imo

I tried that, but the problem is that mdlayher's socket is a wrapper around raw file descriptors, so this will be a cascading change all over the codebase. probably a change for good, but I don't know if it will be accepted or not, as it certainly might break in subtle ways

Makes sense, thanks for the explanation!
I'm interested in seeing how invasive that change would be but probably would be better handled separately in a later MR if it's such a widespread change (ie no need to do it now or send a PR for it but if you did experiment with it before and have even a draft branch or incomplete commit laying around somewhere i'd love to look at it)

I haven't traced the full path, but e.g. if I change makeRawSocket:

dhcp/dhcpv4/client4/client.go

Lines 78 to 84 in f80a195

// makeRawSocket creates a socket that can be passed to unix.Sendto.

func makeRawSocket(ifname string) (int, error) {

fd, err := unix.Socket(unix.AF_INET, unix.SOCK_RAW, unix.IPPROTO_RAW)

if err != nil {

return fd, err

}

err = unix.SetsockoptInt(fd, unix.SOL_SOCKET, unix.SO_REUSEADDR, 1)

It cascades into Exchange

dhcp/dhcpv4/client4/client.go

Lines 198 to 203 in f80a195

if raddr.IP.Equal(net.IPv4bcast) {

sfd, err = MakeBroadcastSocket(ifname)

} else {

sfd, err = makeRawSocket(ifname)

}

if err != nil {

which is basically the core of dhcp4 client.

It's changing every instance of int fd into socket.Socket, which I think is a change for good, but massive refactoring.

It looks like we never leak the int fd objects outside of NewIPv[46]UDPConn, where we put it into an net.UDPConn, so that doesn't have too much impact on outside APIs
There's also the dhcpv4/client4 stuff you pointed above, but nobody should be using that anyway, so there shouldn't be too much effort put into keeping that compatible (people can use nclient4 instead)

If you have the time and will to implement those changes to remove the internal/xsocket implementation in favor of the upstream one and send a PR, it would be appreciated

This is the first step in splitting up the darwin qemu provider feature into smaller, easier to review bits. Make the minimal changes required to have the qemu provider build on darwin. The following changes will come in the "first crash" order. Currently the next step that needs fixing is the preflight checks. After that come the various networking bits. * use a newer commit of the dhcpd fork that has the darwin compatible logic (see insomniacslk/dhcp#550) * make sure that the (broken) darwin qemu logic only runs when `DARWIN_QEMU` env var is set * avoid using the linux specific cni.ns package (this can be changed later, just has to be done for now so the thing compiles) * use a cross-platform fallocate package Signed-off-by: Orzelius <33936483+Orzelius@users.noreply.github.com>

Make the minimal changes required to have the qemu provider build on darwin. This is the first step in splitting up the darwin qemu provider feature into smaller, easier to review bits. The following changes will come in the "first crash" order. Currently the next step that needs fixing is the preflight checks. After that come the various networking bits. * use a newer commit of the dhcpd fork that has the darwin compatible logic (see insomniacslk/dhcp#550) * make sure that the (currently broken) darwin qemu logic only runs when `DARWIN_QEMU` env var is set * avoid using the linux specific cni.ns package (this can be changed back later, just has to be done for now so the thing compiles) * use a cross-platform fallocate package https://github.com/detailyang/go-fallocate Signed-off-by: Orzelius <33936483+Orzelius@users.noreply.github.com>

Make the minimal changes required to have the qemu provider build on darwin. This is the first step in splitting up the darwin qemu provider feature into smaller, easier to review bits. The following changes will come in the "first crash" order. Currently the next step that needs fixing is the preflight checks. After that come the various networking bits. * use a newer commit of the dhcpd fork that has the darwin compatible logic (see insomniacslk/dhcp#550) * make sure that the (currently broken) darwin qemu logic only runs when `DARWIN_QEMU` env var is set * avoid using the linux specific cni.ns package (this can be changed back later, just has to be done for now so the thing compiles) * use a cross-platform fallocate package https://github.com/detailyang/go-fallocate Signed-off-by: Orzelius <33936483+Orzelius@users.noreply.github.com> Signed-off-by: Andrey Smirnov <andrey.smirnov@siderolabs.com>

smira · 2025-07-31T15:17:12Z

hey, anything I can do to get this PR merged/reviewed?

insomniacslk · 2025-08-07T12:32:05Z

@smira I'm open to this change, I just didn't have enough time yet to go through the latest commit yet (personal life in between). I should have some time next week, and hopefully @Natolumin can chime in as well

Natolumin

Code lgtm, but I haven't tested on any platform without cloexec, only linux so far

internal/xsocket/xsocket.go

Natolumin · 2025-10-07T10:30:43Z

internal/xsocket/xsocket.go

+		return fd, nil
+	}
+
+	if err == unix.EINVAL || err == unix.EPROTONOSUPPORT {


Makes sense, thanks for the explanation!
I'm interested in seeing how invasive that change would be but probably would be better handled separately in a later MR if it's such a widespread change (ie no need to do it now or send a PR for it but if you did experiment with it before and have even a draft branch or incomplete commit laying around somewhere i'd love to look at it)

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 <andrey.smirnov@siderolabs.com>

smira · 2025-10-07T13:38:57Z

Code lgtm, but I haven't tested on any platform without cloexec, only linux so far

we have it tested on Mac OS X - a small DHCP server works without issues, and Linux (DHCP client)

smira mentioned this pull request Oct 1, 2024

Talos leaks file descriptors to child processes siderolabs/talos#9412

Closed

smira changed the title ~~fix: add CLOEXEC flag to socket creates directly~~ fix: add CLOEXEC flag to socket created directly Oct 1, 2024

Natolumin reviewed Oct 1, 2024

View reviewed changes

smira force-pushed the fix/cloexec branch from 2675097 to 99942ba Compare April 7, 2025 15:30

Orzelius mentioned this pull request Apr 30, 2025

chore: make qemu provider build on darwin siderolabs/talos#10820

Merged

Natolumin previously approved these changes Oct 7, 2025

View reviewed changes

fix: add CLOEXEC flag to socket creates directly

b942328

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 <andrey.smirnov@siderolabs.com>

smira dismissed Natolumin’s stale review via b942328 October 7, 2025 13:38

smira force-pushed the fix/cloexec branch from 99942ba to b942328 Compare October 7, 2025 13:38

Natolumin approved these changes Oct 7, 2025

View reviewed changes

insomniacslk merged commit da879a2 into insomniacslk:master Oct 7, 2025
8 checks passed

	// makeRawSocket creates a socket that can be passed to unix.Sendto.
	func makeRawSocket(ifname string) (int, error) {
	fd, err := unix.Socket(unix.AF_INET, unix.SOCK_RAW, unix.IPPROTO_RAW)
	if err != nil {
	return fd, err
	}
	err = unix.SetsockoptInt(fd, unix.SOL_SOCKET, unix.SO_REUSEADDR, 1)

	if raddr.IP.Equal(net.IPv4bcast) {
	sfd, err = MakeBroadcastSocket(ifname)
	} else {
	sfd, err = makeRawSocket(ifname)
	}
	if err != nil {

Conversation

smira commented Oct 1, 2024

Uh oh!

Natolumin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Natolumin Sep 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

smira commented Jul 31, 2025

Uh oh!

insomniacslk commented Aug 7, 2025

Uh oh!

Natolumin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

smira commented Oct 7, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Natolumin Sep 27, 2025 •

edited

Loading