os: always return syscall.ECHILD from Wait for done process

For processes that don't exist at lookup time, CL 570036 and CL 588675
make Wait return unconditionally return ErrProcessDone when using pidfd,
rather than attempting to make a wait system call.

This is consistent with Signal/Kill, but inconsistent with the previous
behavior of Wait, which would pass through the kernel error,
syscall.ECHILD.

Switch the ErrProcessDone case to return syscall.ECHILD instead for
consistency with previous behavior.

That said, I do think a future release should consider changing ECHILD
to ErrProcessDone in all cases (including when making an actual wait
system call) for better consistency with Signal/Kill/FindProcess.

Fixes #67926.

Cq-Include-Trybots: luci.golang.try:gotip-darwin-amd64_14,gotip-solaris-amd64,gotip-openbsd-amd64
Change-Id: I1f688a5751d0f3aecea99c3a5b35c7894cfc2beb
Reviewed-on: https://go-review.googlesource.com/c/go/+/591816
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Kirill Kolyshkin <kolyshkin@gmail.com>
Reviewed-by: Cherry Mui <cherryyz@google.com>

diff --git a/src/os/exec_test.go b/src/os/exec_test.go
index b49dd0dd91470d777605be8a78cc4d4f6ed90013..07a06ec0ff3b2cf3988fdbc282069bbbef472809 100644 (file)
--- a/src/os/exec_test.go
+++ b/src/os/exec_test.go
@@ -6,7 +6,6 @@ package os_test
 
 import (
        "internal/testenv"
-       "math"
        "os"
        "os/signal"
        "runtime"
@@ -76,28 +75,3 @@ func TestProcessReleaseTwice(t *testing.T) {
                t.Fatalf("second Release: got err %v, want %v", err, want)
        }
 }
-
-// Lookup of a process that does not exist at time of lookup.
-func TestProcessAlreadyDone(t *testing.T) {
-       if runtime.GOOS == "windows" {
-               t.Skip("Windows does not support lookup of non-existant process")
-       }
-       if runtime.GOARCH == "wasm" {
-               t.Skip("Wait not supported om wasm port")
-       }
-
-       // Theoretically MaxInt32 is a valid PID, but the chance of it actually
-       // being used is extremely unlikely.
-       p, err := os.FindProcess(math.MaxInt32)
-       if err != nil {
-               t.Fatalf("FindProcess(math.MaxInt32) got err %v, want nil", err)
-       }
-
-       if ps, err := p.Wait(); err != os.ErrProcessDone {
-               t.Errorf("Wait() got err %v (ps %+v), want ErrProcessDone", err, ps)
-       }
-
-       if err := p.Release(); err != nil {
-               t.Errorf("Release() got err %v, want nil", err)
-       }
-}

diff --git a/src/os/exec_unix_test.go b/src/os/exec_unix_test.go
index 69bcdbdad1825e2f66a57a18b1d188bd6e24b72b..81d8e1cfee6ab4b0f22840cd35ae883fbb49288d 100644 (file)
--- a/src/os/exec_unix_test.go
+++ b/src/os/exec_unix_test.go
@@ -7,8 +7,11 @@
 package os_test
 
 import (
+       "errors"
        "internal/testenv"
+       "math"
        . "os"
+       "runtime"
        "syscall"
        "testing"
 )
@@ -27,6 +30,33 @@ func TestErrProcessDone(t *testing.T) {
        }
 }
 
+// Lookup of a process that does not exist at time of lookup.
+func TestProcessAlreadyDone(t *testing.T) {
+       // Theoretically MaxInt32 is a valid PID, but the chance of it actually
+       // being used is extremely unlikely.
+       pid := math.MaxInt32
+       if runtime.GOOS == "solaris" || runtime.GOOS == "illumos" {
+               // Solaris/Illumos have a lower limit, above which wait returns
+               // EINVAL (see waitid in usr/src/uts/common/os/exit.c in
+               // illumos). This is configurable via sysconf(_SC_MAXPID), but
+               // we'll just take the default.
+               pid = 30000-1
+       }
+
+       p, err := FindProcess(pid)
+       if err != nil {
+               t.Fatalf("FindProcess(math.MaxInt32) got err %v, want nil", err)
+       }
+
+       if ps, err := p.Wait(); !errors.Is(err, syscall.ECHILD) {
+               t.Errorf("Wait() got err %v (ps %+v), want %v", err, ps, syscall.ECHILD)
+       }
+
+       if err := p.Release(); err != nil {
+               t.Errorf("Release() got err %v, want nil", err)
+       }
+}
+
 func TestUNIXProcessAlive(t *testing.T) {
        testenv.MustHaveGoBuild(t)
        t.Parallel()

diff --git a/src/os/pidfd_linux.go b/src/os/pidfd_linux.go
index c71c366de63b8893c9605aebb5ebc8625426e717..0404c4ff64b72e5f2fa951b28f5843f619ecf644 100644 (file)
--- a/src/os/pidfd_linux.go
+++ b/src/os/pidfd_linux.go
@@ -74,7 +74,10 @@ func (p *Process) pidfdWait() (*ProcessState, error) {
        handle, status := p.handleTransientAcquire()
        switch status {
        case statusDone:
-               return nil, ErrProcessDone
+               // Process already completed Wait, or was not found by
+               // pidfdFind. Return ECHILD for consistency with what the wait
+               // syscall would return.
+               return nil, NewSyscallError("wait", syscall.ECHILD)
        case statusReleased:
                return nil, syscall.EINVAL
        }

diff --git a/src/os/pidfd_linux_test.go b/src/os/pidfd_linux_test.go
index 2f567eed409ee88d4dac151069ffcc5f982882c1..837593706bae8e404a2c5352bdb807cf6b001c19 100644 (file)
--- a/src/os/pidfd_linux_test.go
+++ b/src/os/pidfd_linux_test.go
@@ -5,8 +5,10 @@
 package os_test
 
 import (
+       "errors"
        "internal/testenv"
        "os"
+       "syscall"
        "testing"
 )
 
@@ -47,7 +49,7 @@ func TestFindProcessViaPidfd(t *testing.T) {
        if err := proc.Signal(os.Kill); err != os.ErrProcessDone {
                t.Errorf("Signal: got %v, want %v", err, os.ErrProcessDone)
        }
-       if _, err := proc.Wait(); err != os.ErrProcessDone {
+       if _, err := proc.Wait(); !errors.Is(err, syscall.ECHILD) {
                t.Errorf("Wait: got %v, want %v", err, os.ErrProcessDone)
        }
        // Release never returns errors on Unix.