From 5d0b52bc45c998c5fcb1a60a7289f86d9c92f836 Mon Sep 17 00:00:00 2001 From: Etienne Champetier Date: Mon, 29 Jan 2024 23:57:27 +0200 Subject: [PATCH] sockerpair_unix: avoid double close(), set FD_CLOEXEC 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 1da2cdfebfcecfb6cb0c93fc20fa3822d918f00b Signed-off-by: Etienne Champetier --- pkg/net/socketpair_cloexec_linux.go | 27 ++++++++++++ pkg/net/socketpair_cloexec_unix.go | 38 ++++++++++++++++ pkg/net/socketpair_unix.go | 68 ++++++++++++++--------------- 3 files changed, 98 insertions(+), 35 deletions(-) create mode 100644 pkg/net/socketpair_cloexec_linux.go create mode 100644 pkg/net/socketpair_cloexec_unix.go diff --git a/pkg/net/socketpair_cloexec_linux.go b/pkg/net/socketpair_cloexec_linux.go new file mode 100644 index 00000000..0ca83f68 --- /dev/null +++ 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 new file mode 100644 index 00000000..ed3c2f99 --- /dev/null +++ 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 index f0b5cdb0..ee304964 100644 --- 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() }