Repository: cilium/ebpf
Pull request #1043 (2023-05-25 09:28:47)

sys: prevent clobbering existing entries in fd tracing registry
newFD() didn't check whether the stack it tried to insert for the given fd would overwrite an existing entry.

Use LoadOrStore to check if a stack is already present, and panic if there is.


Patch:
diff --git a/internal/sys/fd.go b/internal/sys/fd.go
index f5ae7b0..941a56f 100644
--- a/internal/sys/fd.go
+++ b/internal/sys/fd.go
@@ -18,7 +18,13 @@ type FD struct {
 
 func newFD(value int) *FD {
 	if onLeakFD != nil {
-		fds.Store(value, callersFrames())
+		// Attempt to store the caller's stack for the given fd value.
+		// Panic if fds contains an existing stack for the fd.
+		old, exist := fds.LoadOrStore(value, callersFrames())
+		if exist {
+			f := old.(*runtime.Frames)
+			panic(fmt.Sprintf("found existing stack for fd %d:\n%s", value, FormatFrames(f)))
+		}
 	}
 
 	fd := &FD{value}
@@ -86,17 +92,16 @@ func (fd *FD) Close() error {
 		return nil
 	}
 
+	return unix.Close(fd.disown())
+}
+
+func (fd *FD) disown() int {
 	value := int(fd.raw)
+	fds.Delete(int(value))
 	fd.raw = -1
 
-	fds.Delete(value)
-
-	fd.Forget()
-	return unix.Close(value)
-}
-
-func (fd *FD) Forget() {
 	runtime.SetFinalizer(fd, nil)
+	return value
 }
 
 func (fd *FD) Dup() (*FD, error) {
@@ -114,7 +119,15 @@ func (fd *FD) Dup() (*FD, error) {
 	return newFD(dup), nil
 }
 
+// File takes ownership of FD and turns it into an [*os.File].
+//
+// You must not use the FD after the call returns.
+//
+// Returns nil if the FD is not valid.
 func (fd *FD) File(name string) *os.File {
-	fd.Forget()
-	return os.NewFile(uintptr(fd.raw), name)
+	if fd.raw < 0 {
+		return nil
+	}
+
+	return os.NewFile(uintptr(fd.disown()), name)
 }
diff --git a/internal/sys/fd_trace.go b/internal/sys/fd_trace.go
index ed2f32c..cd50dd1 100644
--- a/internal/sys/fd_trace.go
+++ b/internal/sys/fd_trace.go
@@ -1,6 +1,8 @@
 package sys
 
 import (
+	"bytes"
+	"fmt"
 	"runtime"
 	"sync"
 )
@@ -76,3 +78,16 @@ func callersFrames() *runtime.Frames {
 
 	return runtime.CallersFrames(c)
 }
+
+// FormatFrames formats a runtime.Frames as a human-readable string.
+func FormatFrames(fs *runtime.Frames) string {
+	var b bytes.Buffer
+	for {
+		f, more := fs.Next()
+		b.WriteString(fmt.Sprintf("\t%s+%#x\n\t\t%s:%d\n", f.Function, f.PC-f.Entry, f.File, f.Line))
+		if !more {
+			break
+		}
+	}
+	return b.String()
+}


Test patch:
diff --git a/internal/sys/fd_test.go b/internal/sys/fd_test.go
index 46d6681..b84121c 100644
--- a/internal/sys/fd_test.go
+++ b/internal/sys/fd_test.go
@@ -47,3 +47,20 @@ func TestFD(t *testing.T) {
 
 	reserveFdZero()
 }
+
+func TestFDFile(t *testing.T) {
+	fd := newFD(openFd(t))
+	file := fd.File("test")
+	qt.Assert(t, file, qt.IsNotNil)
+	qt.Assert(t, file.Close(), qt.IsNil)
+	qt.Assert(t, fd.File("closed"), qt.IsNil)
+
+	_, err := fd.Dup()
+	qt.Assert(t, err, qt.ErrorIs, ErrClosedFd)
+}
+
+func openFd(tb testing.TB) int {
+	fd, err := unix.Open(os.DevNull, syscall.O_RDONLY, 0)
+	qt.Assert(tb, err, qt.IsNil)
+	return fd
+}
diff --git a/internal/testutils/fdtrace/fd.go b/internal/testutils/fdtrace/fd.go
index 612d32d..d7d9534 100644
--- a/internal/testutils/fdtrace/fd.go
+++ b/internal/testutils/fdtrace/fd.go
@@ -1,7 +1,6 @@
 package fdtrace
 
 import (
-	"bytes"
 	"fmt"
 	"os"
 	"runtime"
@@ -19,7 +18,7 @@ func TestMain(m *testing.M) {
 	var leak bool
 	sys.OnLeakFD(func(fs *runtime.Frames) {
 		fmt.Fprintln(os.Stderr, "leaked fd created at:")
-		fmt.Fprintln(os.Stderr, formatFrames(fs))
+		fmt.Fprintln(os.Stderr, sys.FormatFrames(fs))
 		leak = true
 	})
 
@@ -33,15 +32,3 @@ func TestMain(m *testing.M) {
 
 	os.Exit(ret)
 }
-
-func formatFrames(fs *runtime.Frames) string {
-	var b bytes.Buffer
-	for {
-		f, more := fs.Next()
-		b.WriteString(fmt.Sprintf("\t%s+%#x\n\t\t%s:%d\n", f.Function, f.PC-f.Entry, f.File, f.Line))
-		if !more {
-			break
-		}
-	}
-	return b.String()
-}
diff --git a/map_test.go b/map_test.go
index d283aaf..0a3359a 100644
--- a/map_test.go
+++ b/map_test.go
@@ -908,7 +908,7 @@ func TestNewMapInMapFromFD(t *testing.T) {
 	defer nested.Close()
 
 	// Do not copy this, use Clone instead.
-	another, err := NewMapFromFD(nested.FD())
+	another, err := NewMapFromFD(dupFD(t, nested.FD()))
 	if err != nil {
 		t.Fatal("Can't create a new nested map from an FD")
 	}
@@ -1539,18 +1539,12 @@ func TestMapFromFD(t *testing.T) {
 
 	// If you're thinking about copying this, don't. Use
 	// Clone() instead.
-	m2, err := NewMapFromFD(m.FD())
+	m2, err := NewMapFromFD(dupFD(t, m.FD()))
 	testutils.SkipIfNotSupported(t, err)
 	if err != nil {
 		t.Fatal(err)
 	}
-
-	// Both m and m2 refer to the same fd now. Closing either of them will
-	// release the fd to the OS, which then might re-use that fd for another
-	// test. Once we close the second map we might close the re-used fd
-	// inadvertently, leading to spurious test failures.
-	// To avoid this we have to "leak" one of the maps.
-	m2.fd.Forget()
+	defer m2.Close()
 
 	var val uint32
 	if err := m2.Lookup(uint32(0), &val); err != nil {
diff --git a/prog_test.go b/prog_test.go
index bfa915d..d4da093 100644
--- a/prog_test.go
+++ b/prog_test.go
@@ -622,11 +622,12 @@ func TestProgramFromFD(t *testing.T) {
 
 	// If you're thinking about copying this, don't. Use
 	// Clone() instead.
-	prog2, err := NewProgramFromFD(prog.FD())
+	prog2, err := NewProgramFromFD(dupFD(t, prog.FD()))
 	testutils.SkipIfNotSupported(t, err)
 	if err != nil {
 		t.Fatal(err)
 	}
+	defer prog2.Close()
 
 	// Name and type are supposed to be copied from program info.
 	if haveObjName() == nil && prog2.name != "test" {
@@ -636,13 +637,6 @@ func TestProgramFromFD(t *testing.T) {
 	if prog2.typ != SocketFilter {
 		t.Errorf("Expected program to have type SocketFilter, got '%s'", prog2.typ)
 	}
-
-	// Both programs refer to the same fd now. Closing either of them will
-	// release the fd to the OS, which then might re-use that fd for another
-	// test. Once we close the second map we might close the re-used fd
-	// inadvertently, leading to spurious test failures.
-	// To avoid this we have to "leak" one of the programs.
-	prog2.fd.Forget()
 }
 
 func TestHaveProgTestRun(t *testing.T) {
@@ -1103,3 +1097,14 @@ func ExampleProgramSpec_Tag() {
 		fmt.Println("The programs are identical, tag is", tag)
 	}
 }
+
+func dupFD(tb testing.TB, fd int) int {
+	tb.Helper()
+
+	dup, err := unix.FcntlInt(uintptr(fd), unix.F_DUPFD_CLOEXEC, 1)
+	if err != nil {
+		tb.Fatal("Can't dup fd:", err)
+	}
+
+	return dup
+}