From 19bd8271cfd303463d10fe775af68d2eaad36e98 Mon Sep 17 00:00:00 2001
From: Etienne Champetier <e.champetier@ateme.com>
Date: Mon, 29 Jan 2024 23:57:27 +0200
Subject: [PATCH] net/sockerpair_unix: avoid closing unrelated fds

We were calling the close() syscall multiple time
with the same value, 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.
---
 pkg/net/socketpair_unix.go | 27 ++++++++++++++-------------
 1 file changed, 14 insertions(+), 13 deletions(-)

diff --git a/pkg/net/socketpair_unix.go b/pkg/net/socketpair_unix.go
index f0b5cdb0..e4115a0c 100644
--- a/pkg/net/socketpair_unix.go
+++ b/pkg/net/socketpair_unix.go
@@ -32,26 +32,31 @@ const (
 )
 
 // SocketPair contains the file descriptors of a connected pair of sockets.
-type SocketPair [2]int
+type SocketPair [2]*os.File
 
 // NewSocketPair returns a connected pair of sockets.
 func NewSocketPair() (SocketPair, error) {
 	fds, err := syscall.Socketpair(syscall.AF_UNIX, syscall.SOCK_STREAM, 0)
 	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[local], fds[peer])
+
+	return SocketPair{
+		os.NewFile(uintptr(fds[local]), filename+"[0]"),
+		os.NewFile(uintptr(fds[peer]), 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]")
+	return fds[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]")
+	return fds[peer]
 }
 
 // LocalConn returns a net.Conn for the local end of the socketpair.
@@ -60,7 +65,7 @@ func (fds SocketPair) LocalConn() (net.Conn, error) {
 	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
 }
@@ -71,7 +76,7 @@ func (fds SocketPair) PeerConn() (net.Conn, error) {
 	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
 }
@@ -84,14 +89,10 @@ func (fds SocketPair) Close() {
 
 // LocalClose closes the local end of the socketpair.
 func (fds SocketPair) LocalClose() {
-	syscall.Close(fds[local])
+	fds[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])
+	fds[peer].Close()
 }