Fix recvfrom goroutine leak

[nekohasekai]

unix.Recvfrom does not return after fd close.

Fixes #792

[nekohasekai]

About why old TestIfSocketCloses passed:

SetReceiveTimeout make Receive() always return unix.EAGAIN

[thaJeztah]

Looks like this one may have introduced a regression; see moby/moby#46982 (comment)

EINTR on netlink sockets is a new one. I suspect it has more to do with the netlink dependency bump you pulled in when rebasing than on the Go toolchain bump. I think the bug is here:

netlink/nl/nl_linux.go

Lines 821 to 824 in 9264582

	err = rawConn.Read(func(fd uintptr) (done bool) {
	nr, from, innerErr = unix.Recvfrom(int(fd), rb[:], 0)
	return innerErr != unix.EWOULDBLOCK
	})

It retries on EWOULDBLOCK (a.k.a. EAGAIN) but neglects to retry on EINTR.

[database64128]

@thaJeztah IO calls on non-blocking sockets will never return -EINTR. The problem here is that Moby calls SetSocketTimeout, which sets SO_SNDTIMEO and SO_RCVTIMEO. These socket options are only useful for sockets in blocking mode. Setting these probably places the socket back into blocking mode.

https://github.com/moby/moby/blob/92a05cf4148adc26054a5b52b191e0b4d13112cd/libnetwork/ns/init_linux.go#L25-L40

netlink/nl/nl_linux.go

Lines 848 to 860 in 9264582

	// SetSendTimeout allows to set a send timeout on the socket
	func (s *NetlinkSocket) SetSendTimeout(timeout *unix.Timeval) error {
	// Set a send timeout of SOCKET_SEND_TIMEOUT, this will allow the Send to periodically unblock and avoid that a routine
	// remains stuck on a send on a closed fd
	return unix.SetsockoptTimeval(int(s.fd), unix.SOL_SOCKET, unix.SO_SNDTIMEO, timeout)
	}
	// SetReceiveTimeout allows to set a receive timeout on the socket
	func (s *NetlinkSocket) SetReceiveTimeout(timeout *unix.Timeval) error {
	// Set a read timeout of SOCKET_READ_TIMEOUT, this will allow the Read to periodically unblock and avoid that a routine
	// remains stuck on a recvmsg on a closed fd
	return unix.SetsockoptTimeval(int(s.fd), unix.SOL_SOCKET, unix.SO_RCVTIMEO, timeout)
	}

I'm not very familiar with this project, but it seems to me that before this PR, only blocking IO is used. This PR adds calls to set the socket as non-blocking, but still allows setting the timeout socket options.

[thaJeztah]

Thanks; let me check with the networking folks about those parts.

[corhere]

Tracing through the kernel sources, I can confidently claim that the timeout sockopts have no effect when the O_NONBLOCK fcntl is enabled. That's a red herring.

• O_NONBLOCK on the socket forces the MSG_DONTWAIT flag to be set on all recv[from] syscalls
• recv on a netlink socket is implemented in terms of skb_recv_datagram
• The implementation of skb_recv_datagram only blocks if the timeout is non-zero

The timeout for a non-blocking socket would be zero due to the MSG_DONTWAIT flag being forced on; therefore setting the timeout sockopts must have no effect on the behavior of the recv syscall.

Need further proof? The timeouts on new sockets are initialized to LONG_MAX. And there is no magic hiding in the setsockopt syscall.

System calls that "shouldn't" return -EINTR have been known to return -EINTR in practice when a signal is delivered to the task while in a syscall. See e.g. https://go.dev/issue/38033. Maybe it's a kernel bug which results in a recv on a non-blocking socket returning with -EINTR upon receiving a signal despite the SA_RESTART sigaction when a blocking recv would have been implicitly restarted?

[database64128]

This all still feels very strange to me.

I checked tokio's implementation of TcpStream and followed the references all the way down to the libc calls. There's no handling of -EINTR (std::io::ErrorKind::Interrupted) at all. The netlink-sys crate's tokio socket implementation does not handle -EINTR either.

I actually rolled my own rtnetlink Go package a while ago, and it never had to deal with -EINTR.

[corhere]

Unlike Go programs, Rust programs don't (inherently) have a runtime which sends SIGURG to the process baked in.

[database64128]

Unlike Go programs, Rust programs don't (inherently) have a runtime which sends SIGURG to the process baked in.

Tokio does allow you to register for receiving signals. It's actually quite similar to os/signal in terms of usage. I imagine there are programs that use Tokio for network IO and receiving signals at the same time.

[robmry]

This change breaks the timeouts, I can raise a PR to fix that.

But, the EINTR we've been seeing in moby CI is unrelated - it's due to #925, exposing an NLM_F_DUMP_INTR (reported in the netlink response) as an EINTR ...

netlink/nl/nl_linux.go

Lines 582 to 584 in 9264582

	if m.Header.Flags&unix.NLM_F_DUMP_INTR != 0 {
	return syscall.Errno(unix.EINTR)
	}