os: double close pidfd if caller uses pidfd updated by os.StartProcess

[fuweid]

Go version

go1.23.0

Output of go env in your module/workspace:

GO111MODULE=''
GOARCH='amd64'
GOBIN=''
GOCACHE='/home/fuweid/.cache/go-build'
GOENV='/home/fuweid/.config/go/env'
GOEXE=''
GOEXPERIMENT=''
GOFLAGS=''
GOHOSTARCH='amd64'
GOHOSTOS='linux'
GOINSECURE=''
GOMODCACHE='/home/fuweid/go/pkg/mod'
GONOPROXY=''
GONOSUMDB=''
GOOS='linux'
GOPATH='/home/fuweid/go'
GOPRIVATE=''
GOPROXY='https://goproxy.cn,direct'
GOROOT='/usr/local/go'
GOSUMDB='sum.golang.org'
GOTMPDIR=''
GOTOOLCHAIN='auto'
GOTOOLDIR='/usr/local/go/pkg/tool/linux_amd64'
GOVCS=''
GOVERSION='go1.23.0'
GODEBUG=''
GOTELEMETRY='local'
GOTELEMETRYDIR='/home/fuweid/.config/go/telemetry'
GCCGO='gccgo'
GOAMD64='v1'
AR='ar'
CC='gcc'
CXX='g++'
CGO_ENABLED='1'
GOMOD='/home/fuweid/workspace/demo/go.mod'
GOWORK=''
CGO_CFLAGS='-O2 -g'
CGO_CPPFLAGS=''
CGO_CXXFLAGS='-O2 -g'
CGO_FFLAGS='-O2 -g'
CGO_LDFLAGS='-O2 -g'
PKG_CONFIG='pkg-config'
GOGCCFLAGS='-fPIC -m64 -pthread -Wl,--no-gc-sections -fmessage-length=0 -ffile-prefix-map=/tmp/go-build655447324=/tmp/go-build -gno-record-gcc-switches

What did you do?

NOTE: It requires kernel to support pidfd.

I try to use os.StartProcess with ptrace to remove all the usage of linkname in containerd project containerd/containerd#10611.

For the os.StartProcess option. I set PidFD, use os.NewFile for it and close it.

go/src/syscall/exec_linux.go

Lines 108 to 111 in 6885bad

	// PidFD, if not nil, is used to store the pidfd of a child, if the
	// functionality is supported by the kernel, or -1. Note *PidFD is
	// changed only if the process starts successfully.
	PidFD *int

But I run into accept4: bad file descriptor issue randomly, because the Process has finalizer to close the pidfd during GC.

go/src/os/exec_posix.go

Lines 64 to 72 in 6885bad

	if runtime.GOOS != "windows" {
	var ok bool
	h, ok = getPidfd(sysattr.Sys)
	if !ok {
	return newPIDProcess(pid), nil
	}
	}
	return newHandleProcess(pid, h), nil

go/src/os/exec.go

Lines 114 to 122 in 6885bad

	func newHandleProcess(pid int, handle uintptr) *Process {
	p := &Process{
	Pid: pid,
	mode: modeHandle,
	handle: handle,
	}
	p.state.Store(1) // 1 persistent reference
	runtime.SetFinalizer(p, (*Process).Release)
	return p

There is POC code for reproducing it.

package main

import (
        "fmt"
        "os"
        "runtime"
        "syscall"
        "time"

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

func main() {
        firstFd := createNoopFd()
        defer firstFd.Close()

        var pidfd int
        proc, err := os.StartProcess("/proc/self/exe", []string{"echo"}, &os.ProcAttr{
                Sys: &syscall.SysProcAttr{
                        Ptrace:    true,
                        PidFD:     &pidfd,
                        Pdeathsig: syscall.SIGKILL,
                },
        })
        if err != nil {
                panic(err)
        }

        if pidfd <= 0 {
                proc.Kill()
                proc.Wait()
                panic("empty pidfd")
        }

        fmt.Println("pidfd = ", pidfd)
        pidFD := os.NewFile(uintptr(pidfd), "pidfd")
        unix.PidfdSendSignal(int(pidFD.Fd()), unix.SIGKILL, nil, 0)
        unix.Waitid(unix.P_PIDFD, int(pidFD.Fd()), nil, unix.WEXITED, nil)
        pidFD.Close()
        fmt.Println("Closed pidfd")

        secondFd := createNoopFd()
        defer secondFd.Close()

        thirdFd := createNoopFd()
        defer thirdFd.Close()

        failedFd := createNoopFd()
        defer failedFd.Close()

        if fd := failedFd.Fd(); fd != uintptr(pidfd) {
                panic("please align fd manually")
        }
        fmt.Println("target fd ", failedFd.Fd())

        for {
                _, err := failedFd.Write([]byte("hello"))
                if err != nil {
                        panic(err)
                }
                time.Sleep(1 * time.Second)
                runtime.GC() // target finalizer for `proc`
        }
}

func createNoopFd() *os.File {
        f, err := os.CreateTemp("/tmp", "PIDFD")
        if err != nil {
                panic(err)
        }
        return f
}

I think the document should mention that the PidFD should not be used or the GO runtime should dup it before using it.

What did you see happen?

When you run the poc code by go1.23.0, you will see the error like.

➜  demo go version
go version go1.23.0 linux/amd64
➜  demo go run main.go
pidfd =  8
Closed pidfd
target fd  8
panic: write /tmp/PIDFD3716412711: bad file descriptor

goroutine 1 [running]:
main.main()
        /home/fuweid/workspace/demo/main.go:59 +0x50b
exit status 2

What did you expect to see?

Should not use user's pidfd value directly.

[fuweid]

cc @kolyshkin

[gabyhelp]

Related Issues and Documentation

• os/exec: Can't use os.Command (with ExtraFiles) that uses /proc/self/fd/<exe fd> as the executable #66654
• os: crashes due to new error from FindProcess (CL 542699) #65866 (closed)
• runtime: golang Automatically Closed fd 1 and fd 2 #58424 (closed)
• affected/package: os/exec, context not work. #53700 (closed)
• os.RemoveAll: openFdAt function without O_CLOEXEC and cause fd escape to child process #33405 (closed)
• os: return value of Fd can be closed unexpectedly by finalizer #9046 (closed)
• getpid returns fault pid everytime in some code #30858 (closed)
• syscall: nextfd handling for attr.Files shuffle will clobber files #61751
• os/exec: #42803 (closed)
• runtime: accidental stack growth in runtime.sigfillset after forking #43066 (closed)

_{(Emoji vote if this was helpful or unhelpful; more detailed feedback welcome in this discussion.)}

[prattmic]

I’d say this is a bug. If you manually set PidFD in SysProcAttr, then os.Process shouldn’t close it. That should be your responsibility.

[prattmic]

cc @golang/runtime

[gopherbot]

Change https://go.dev/cl/607695 mentions this issue: os: dup pidfd if caller sets PidFD manually

[prattmic]

@gopherbot Please backport to 1.23. This is a regression in os.StartProcess for programs already using pidfds.

[gopherbot]

Backport issue(s) opened: #69119 (for 1.23).

Remember to create the cherry-pick CL(s) as soon as the patch is submitted to master, according to https://go.dev/wiki/MinorReleases.

[gopherbot]

Change https://go.dev/cl/611415 mentions this issue: [release-branch.go1.23] os: dup pidfd if caller sets PidFD manually