Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

wrong usage of os.FindProcess #1709

Open
1 of 5 tasks
NitroCao opened this issue Sep 12, 2024 · 0 comments · May be fixed by #1716
Open
1 of 5 tasks

wrong usage of os.FindProcess #1709

NitroCao opened this issue Sep 12, 2024 · 0 comments · May be fixed by #1716

Comments

@NitroCao
Copy link

NitroCao commented Sep 12, 2024

Describe the bug
When using gopsutil to traverse all processes on Linux systems which support pidfd_open(2), the process will hold too many file descriptors with pidfd type.
The bug is caused by the wrong usage of os.FindProcess() in PidExistsWithContext, we need call proc.Release() immediately after os.FindProcess() returns:

func PidExistsWithContext(ctx context.Context, pid int32) (bool, error) {
if pid <= 0 {
return false, fmt.Errorf("invalid pid %v", pid)
}
proc, err := os.FindProcess(int(pid))
if err != nil {
return false, err
}
if isMount(common.HostProcWithContext(ctx)) { // if /<HOST_PROC>/proc exists and is mounted, check if /<HOST_PROC>/proc/<PID> folder exists

The correct code would be:

func PidExistsWithContext(ctx context.Context, pid int32) (bool, error) {
	if pid <= 0 {
		return false, fmt.Errorf("invalid pid %v", pid)
	}
	proc, err := os.FindProcess(int(pid))
	if err != nil {
		return false, err
	}
	defer proc.Release()

	if isMount(common.HostProcWithContext(ctx)) { // if /<HOST_PROC>/proc exists and is mounted, check if /<HOST_PROC>/proc/<PID> folder exists

The doc in os library:

Release releases any resources associated with the Process p, rendering it unusable in the future. Release only needs to be called if Process.Wait is not.

The reason is for kernels which support pidfd_open(2), os.FindProcess() will call pidfd_open(2) and save the fd into Process.handle, and then close it when (*Process).Release() called.
https://github.com/golang/go/blob/e9a500f47dadcd73c970649a1072d28997617610/src/os/exec_unix.go#L156-L170

func findProcess(pid int) (p *Process, err error) {
	h, err := pidfdFind(pid)
	if err == ErrProcessDone {
		// We can't return an error here since users are not expecting
		// it. Instead, return a process with a "done" state already
		// and let a subsequent Signal or Wait call catch that.
		return newDoneProcess(pid), nil
	} else if err != nil {
		// Ignore other errors from pidfdFind, as the callers
		// do not expect them. Fall back to using the PID.
		return newPIDProcess(pid), nil
	}
	// Use the handle.
	return newHandleProcess(pid, h), nil
}

https://github.com/golang/go/blob/e9a500f47dadcd73c970649a1072d28997617610/src/os/exec.go#L219-L221

func (p *Process) handlePersistentRelease(reason processStatus) processStatus {
	if p.mode != modeHandle {
		panic("handlePersistentRelease called in invalid mode")
	}

	for {
		refs := p.state.Load()
		status := processStatus(refs & processStatusMask)
		if status != statusOK {
			// Both Release and successful Wait will drop the
			// Process' persistent reference on the handle. We
			// can't allow concurrent calls to drop the reference
			// twice, so we use the status as a guard to ensure the
			// reference is dropped exactly once.
			return status
		}
		if refs == 0 {
			// This should never happen because dropping the
			// persistent reference always sets a status.
			panic("release of handle with refcount 0")
		}
		new := (refs - 1) | uint64(reason)
		if !p.state.CompareAndSwap(refs, new) {
			continue
		}
		if new&^processStatusMask == 0 {
			p.closeHandle()
		}
		return status
	}
}

Besides SendSignalWithContext is also affected.

To Reproduce

package main

import (
	"log"
	"os"
	"time"

	"github.com/shirou/gopsutil/v4/process"
)

func main() {
	log.Printf("my pid: %d", os.Getpid())
	if err != nil {
		log.Fatal(err)
	}
	pids, err := process.Pids()
	for _, pid := range pids {
		p, err := process.NewProcess(pid)
		if err != nil {
			continue
		}
		log.Println(p.Pid)
	}

	log.Printf("the number of running procs: %d", len(pids))
	time.Sleep(30 * time.Second)
}

compile it by executing go build -o demo main.go. After executing the demo code, use ls -l /proc/$(pgrep demo)/fd | grep pidfd | wc -l command to count the number of pidfds.
Expected behavior

Environment (please complete the following information):

  • Windows: [paste the result of ver]
  • Linux: 5.10.134-008.2.ali5000.al8.x86_64
  • Mac OS: [paste the result of sw_vers and uname -a
  • FreeBSD: [paste the result of freebsd-version -k -r -u and uname -a]
  • OpenBSD: [paste the result of uname -a]
NAME="Alibaba Cloud Linux"
VERSION="3 (Soaring Falcon)"
ID="alinux"
ID_LIKE="rhel fedora centos anolis"
VERSION_ID="3"
UPDATE_ID="9"
PLATFORM_ID="platform:al8"
PRETTY_NAME="Alibaba Cloud Linux 3 (Soaring Falcon)"
ANSI_COLOR="0;31"
HOME_URL="https://www.aliyun.com/"

Additional context

@NitroCao NitroCao linked a pull request Sep 27, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant