Skip to content

Commit

Permalink
sockerpair: avoid double close(), set FD_CLOEXEC
Browse files Browse the repository at this point in the history 
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 <[email protected]>
  • Loading branch information
@champtar
champtar committed Jan 30, 2024
1 parent 6f5a4d2 commit c4da6cf
Show file tree
Hide file tree
Showing 3 changed files with 102 additions and 131 deletions.
76 changes: 76 additions & 0 deletions pkg/net/socketpair.go
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,76 @@
/*
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 (
"fmt"
"net"
"os"
)

// SocketPair contains the os.Files of a connected pair of sockets.
type SocketPair struct {
local, peer *os.File
}

// LocalFile returns the socketpair fd for local usage 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 (sp SocketPair) PeerFile() *os.File {
return sp.peer
}

// LocalConn returns a net.Conn for the local end of the socketpair.
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: %w", file.Name(), err)
}
return conn, nil
}

// PeerConn returns a net.Conn for the peer end of the socketpair.
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: %w", file.Name(), err)
}
return conn, nil
}

// Close closes both ends of the socketpair.
func (sp SocketPair) Close() {
sp.LocalClose()
sp.PeerClose()
}

// LocalClose closes the local end of the socketpair.
func (sp SocketPair) LocalClose() {
sp.local.Close()
}

// PeerClose closes the peer end of the socketpair.
func (sp SocketPair) PeerClose() {
sp.peer.Close()
}
77 changes: 13 additions & 64 deletions pkg/net/socketpair_unix.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,78 +20,27 @@ package net

import (
"fmt"
"net"
"os"
"syscall"

syscall "golang.org/x/sys/unix"
"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

// 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 fds, 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]")
}

// 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]")
}

// LocalConn returns a net.Conn for the local end of the socketpair.
func (fds SocketPair) LocalConn() (net.Conn, error) {
file := fds.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 conn, nil
}

// PeerConn returns a net.Conn for the peer end of the socketpair.
func (fds SocketPair) PeerConn() (net.Conn, error) {
file := fds.PeerFile()
defer file.Close()
conn, err := net.FileConn(file)
syscall.ForkLock.RLock()
defer syscall.ForkLock.RUnlock()
fds, err := unix.Socketpair(unix.AF_UNIX, unix.SOCK_STREAM, 0)
if err != nil {
return nil, fmt.Errorf("failed to create net.Conn for %s[1]: %w", fds.fileName(), err)
return SocketPair{nil, nil}, fmt.Errorf("failed to create socketpair: %w", err)
}
return conn, nil
}
unix.CloseOnExec(fds[0])
unix.CloseOnExec(fds[1])

// Close closes both ends of the socketpair.
func (fds SocketPair) Close() {
fds.LocalClose()
fds.PeerClose()
}

// LocalClose closes the local end of the socketpair.
func (fds SocketPair) LocalClose() {
syscall.Close(fds[local])
}

// PeerClose closes the peer end of the socketpair.
func (fds SocketPair) PeerClose() {
syscall.Close(fds[peer])
}
filename := fmt.Sprintf("socketpair-#%d:%d", fds[0], fds[1])

func (fds SocketPair) fileName() string {
return fmt.Sprintf("socketpair-#%d:%d[0]", fds[local], fds[peer])
return SocketPair{
os.NewFile(uintptr(fds[0]), filename+"[0]"),
os.NewFile(uintptr(fds[1]), filename+"[1]"),
}, nil
}
80 changes: 13 additions & 67 deletions pkg/net/socketpair_windows.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,21 +20,13 @@ package net

import (
"fmt"
"net"
"os"
"syscall"
"unsafe"

sys "golang.org/x/sys/windows"
)

// SocketPair contains a connected pair of sockets.
type SocketPair [2]sys.Handle

const (
local = 0
peer = 1
)

// NewSocketPair returns a connected pair of sockets.
func NewSocketPair() (SocketPair, error) {
/* return [2]sys.Handle{sys.InvalidHandle, sys.InvalidHandle},
Expand All @@ -46,7 +38,7 @@ func NewSocketPair() (SocketPair, error) {

func emulateWithPreConnect() (SocketPair, error) {
var (
invalid = SocketPair{sys.InvalidHandle, sys.InvalidHandle}
invalid = SocketPair{nil, nil}
sa sys.SockaddrInet4
//sn sys.Sockaddr
l sys.Handle
Expand All @@ -55,15 +47,14 @@ func emulateWithPreConnect() (SocketPair, error) {
err error
)

syscall.ForkLock.RLock()
defer syscall.ForkLock.RUnlock()

l, err = socket(sys.AF_INET, sys.SOCK_STREAM, 0)
if err != nil {
return invalid, fmt.Errorf("failed to emulate socketpair (local Socket()): %w", err)
}
defer func() {
if err != nil {
sys.CloseHandle(l)
}
}()
defer sys.CloseHandle(l)

sa.Addr[0] = 127
sa.Addr[3] = 1
Expand Down Expand Up @@ -109,60 +100,15 @@ func emulateWithPreConnect() (SocketPair, error) {
}
}()

sys.CloseHandle(l)
return SocketPair{a, p}, nil
}

// Close closes both ends of the socketpair.
func (sp SocketPair) Close() {
sp.LocalClose()
sp.PeerClose()
}

// LocalFile returns the socketpair fd for local usage as an *os.File.
func (sp SocketPair) LocalFile() *os.File {
return os.NewFile(uintptr(sp[local]), sp.fileName()+"[0]")
}

// PeerFile returns the socketpair fd for peer usage as an *os.File.
func (sp SocketPair) PeerFile() *os.File {
return os.NewFile(uintptr(sp[peer]), sp.fileName()+"[1]")
}
sys.CloseOnExec(a)
sys.CloseOnExec(p)

// LocalConn returns a net.Conn for the local end of the socketpair.
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", sp.fileName(), err)
}
return conn, nil
}

// PeerConn returns a net.Conn for the peer end of the socketpair.
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", sp.fileName(), err)
}
return conn, nil
}

// LocalClose closes the local end of the socketpair.
func (sp SocketPair) LocalClose() {
sys.CloseHandle(sp[local])
}

// PeerClose closes the peer end of the socketpair.
func (sp SocketPair) PeerClose() {
sys.CloseHandle(sp[peer])
}
filename := fmt.Sprintf("socketpair-#%d:%d", a, p)

func (sp SocketPair) fileName() string {
return fmt.Sprintf("socketpair-#%d:%d[0]", sp[local], sp[peer])
return SocketPair{
os.NewFile(uintptr(a), filename+"[0]"),
os.NewFile(uintptr(p), filename+"[1]"),
}, nil
}

func socket(domain, typ, proto int) (sys.Handle, error) {
Expand Down

0 comments on commit c4da6cf

Please sign in to comment.