sockerpair_unix: avoid double close(), set FD_CLOEXEC

champtar · champtar · commit 5d0b52bc45c9 · 2024-01-31T13:13:23.000+02:00
We were calling the close() syscall multiple time with the same fd number, leading to random issues like closing containerd stream port. In newLaunchedPlugin() we have: sockets, _ := net.NewSocketPair() defer sockets.Close() conn, _ := sockets.LocalConn() peerFile := sockets.PeerFile() defer func() { peerFile.Close() if retErr != nil { conn.Close() } }() cmd.Start() so we were doing: close(local) (in LocalConn()) cmd.Start() close(peer) (peerFile.Close()) close(local) (sockets.Close()) close(peer) (sockets.Close()) If the NRI plugin that we launch with cmd.Start() is not cached or the system is a bit busy, cmd.Start() gives a large enough window for another goroutine to open a file that will get the same fd number as local was, thus being closed by accident. Fix the situation by storing os.Files instead of ints, so that closing multiple times just returns an error (that we ignore). Also set FD_CLOEXEC on the sockets so we don't leak them. Fixes 1da2cdf Signed-off-by: Etienne Champetier <e.champetier@ateme.com>
diff --git a/pkg/net/socketpair_cloexec_linux.go b/pkg/net/socketpair_cloexec_linux.go
@@ -0,0 +1,27 @@
+//go:build linux
+
+/*
+   Copyright The containerd Authors.
+
+   Licensed under the Apache License, Version 2.0 (the "License");
+   you may not use this file except in compliance with the License.
+   You may obtain a copy of the License at
+
+       http://www.apache.org/licenses/LICENSE-2.0
+
+   Unless required by applicable law or agreed to in writing, software
+   distributed under the License is distributed on an "AS IS" BASIS,
+   WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+   See the License for the specific language governing permissions and
+   limitations under the License.
+*/
+
+package net
+
+import (
+	"golang.org/x/sys/unix"
+)
+
+func newSocketPairCLOEXEC() ([2]int, error) {
+	return unix.Socketpair(unix.AF_UNIX, unix.SOCK_STREAM|unix.SOCK_CLOEXEC, 0)
+}
diff --git a/pkg/net/socketpair_cloexec_unix.go b/pkg/net/socketpair_cloexec_unix.go
@@ -0,0 +1,38 @@
+//go:build !linux && !windows
+
+/*
+   Copyright The containerd Authors.
+
+   Licensed under the Apache License, Version 2.0 (the "License");
+   you may not use this file except in compliance with the License.
+   You may obtain a copy of the License at
+
+       http://www.apache.org/licenses/LICENSE-2.0
+
+   Unless required by applicable law or agreed to in writing, software
+   distributed under the License is distributed on an "AS IS" BASIS,
+   WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+   See the License for the specific language governing permissions and
+   limitations under the License.
+*/
+
+package net
+
+import (
+	"syscall"
+
+	"golang.org/x/sys/unix"
+)
+
+func newSocketPairCLOEXEC() ([2]int, error) {
+	syscall.ForkLock.RLock()
+	defer syscall.ForkLock.RUnlock()
+	fds, err := unix.Socketpair(unix.AF_UNIX, unix.SOCK_STREAM, 0)
+	if err != nil {
+		return fds, err
+	}
+	unix.CloseOnExec(fds[0])
+	unix.CloseOnExec(fds[1])
+
+	return fds, err
+}
diff --git a/pkg/net/socketpair_unix.go b/pkg/net/socketpair_unix.go
@@ -22,76 +22,74 @@ import (
 	"fmt"
 	"net"
 	"os"
-
-	syscall "golang.org/x/sys/unix"
-)
-
-const (
-	local = 0
-	peer  = 1
 )
 
-// SocketPair contains the file descriptors of a connected pair of sockets.
-type SocketPair [2]int
+// SocketPair contains the os.File of a connected pair of sockets.
+type SocketPair struct {
+	local, peer *os.File
+}
 
 // NewSocketPair returns a connected pair of sockets.
 func NewSocketPair() (SocketPair, error) {
-	fds, err := syscall.Socketpair(syscall.AF_UNIX, syscall.SOCK_STREAM, 0)
+	fds, err := newSocketPairCLOEXEC()
 	if err != nil {
-		return [2]int{-1, -1}, fmt.Errorf("failed to create socketpair: %w", err)
+		return SocketPair{nil, nil}, fmt.Errorf("failed to create socketpair: %w", err)
 	}
 
-	return fds, nil
+	filename := fmt.Sprintf("socketpair-#%d:%d", fds[0], fds[1])
+
+	return SocketPair{
+		os.NewFile(uintptr(fds[0]), filename+"[0]"),
+		os.NewFile(uintptr(fds[1]), filename+"[1]"),
+	}, nil
 }
 
-// LocalFile returns the socketpair fd for local usage as an *os.File.
-func (fds SocketPair) LocalFile() *os.File {
-	return os.NewFile(uintptr(fds[local]), fds.fileName()+"[0]")
+// LocalFile returns the local end of the socketpair as an *os.File.
+func (sp SocketPair) LocalFile() *os.File {
+	return sp.local
 }
 
-// PeerFile returns the socketpair fd for peer usage as an *os.File.
-func (fds SocketPair) PeerFile() *os.File {
-	return os.NewFile(uintptr(fds[peer]), fds.fileName()+"[1]")
+// PeerFile returns the peer end of the socketpair as an *os.File.
+func (sp SocketPair) PeerFile() *os.File {
+	return sp.peer
 }
 
 // LocalConn returns a net.Conn for the local end of the socketpair.
-func (fds SocketPair) LocalConn() (net.Conn, error) {
-	file := fds.LocalFile()
+// This closes LocalFile().
+func (sp SocketPair) LocalConn() (net.Conn, error) {
+	file := sp.LocalFile()
 	defer file.Close()
 	conn, err := net.FileConn(file)
 	if err != nil {
-		return nil, fmt.Errorf("failed to create net.Conn for %s[0]: %w", fds.fileName(), err)
+		return nil, fmt.Errorf("failed to create net.Conn for %s: %w", file.Name(), err)
 	}
 	return conn, nil
 }
 
 // PeerConn returns a net.Conn for the peer end of the socketpair.
-func (fds SocketPair) PeerConn() (net.Conn, error) {
-	file := fds.PeerFile()
+// This closes PeerFile().
+func (sp SocketPair) PeerConn() (net.Conn, error) {
+	file := sp.PeerFile()
 	defer file.Close()
 	conn, err := net.FileConn(file)
 	if err != nil {
-		return nil, fmt.Errorf("failed to create net.Conn for %s[1]: %w", fds.fileName(), err)
+		return nil, fmt.Errorf("failed to create net.Conn for %s: %w", file.Name(), err)
 	}
 	return conn, nil
 }
 
 // Close closes both ends of the socketpair.
-func (fds SocketPair) Close() {
-	fds.LocalClose()
-	fds.PeerClose()
+func (sp SocketPair) Close() {
+	sp.LocalClose()
+	sp.PeerClose()
 }
 
 // LocalClose closes the local end of the socketpair.
-func (fds SocketPair) LocalClose() {
-	syscall.Close(fds[local])
+func (sp SocketPair) LocalClose() {
+	sp.local.Close()
 }
 
 // PeerClose closes the peer end of the socketpair.
-func (fds SocketPair) PeerClose() {
-	syscall.Close(fds[peer])
-}
-
-func (fds SocketPair) fileName() string {
-	return fmt.Sprintf("socketpair-#%d:%d[0]", fds[local], fds[peer])
+func (sp SocketPair) PeerClose() {
+	sp.peer.Close()
 }
