os: crashes due to new error from FindProcess (CL 542699)

[stgraber]

Go version

go version devel go1.23-5d4e8f5 Thu Feb 22 01:57:32 2024 +0000 linux/amd64

Output of go env in your module/workspace:

GO111MODULE=''
GOARCH='amd64'
GOBIN=''
GOCACHE='/home/stgraber/.cache/go-build'
GOENV='/home/stgraber/.config/go/env'
GOEXE=''
GOEXPERIMENT=''
GOFLAGS=''
GOHOSTARCH='amd64'
GOHOSTOS='linux'
GOINSECURE=''
GOMODCACHE='/home/stgraber/data/code/go/pkg/mod'
GONOPROXY=''
GONOSUMDB=''
GOOS='linux'
GOPATH='/home/stgraber/data/code/go'
GOPRIVATE=''
GOPROXY='https://proxy.golang.org,direct'
GOROOT='/home/stgraber/sdk/gotip'
GOSUMDB='sum.golang.org'
GOTMPDIR=''
GOTOOLCHAIN='auto'
GOTOOLDIR='/home/stgraber/sdk/gotip/pkg/tool/linux_amd64'
GOVCS=''
GOVERSION='devel go1.23-5d4e8f5 Thu Feb 22 01:57:32 2024 +0000'
GCCGO='gccgo'
GOAMD64='v1'
AR='ar'
CC='gcc'
CXX='g++'
CGO_ENABLED='1'
GOMOD='/dev/null'
GOWORK=''
CGO_CFLAGS='-I/home/stgraber/data/code/go/deps/raft/include/ -I/home/stgraber/data/code/go/deps/cowsql/include/'
CGO_CPPFLAGS=''
CGO_CXXFLAGS='-O2 -g'
CGO_FFLAGS='-O2 -g'
CGO_LDFLAGS='-L/home/stgraber/data/code/go/deps/raft/.libs -L/home/stgraber/data/code/go/deps/cowsql/.libs/'
PKG_CONFIG='pkg-config'
GOGCCFLAGS='-fPIC -m64 -pthread -Wl,--no-gc-sections -fmessage-length=0 -ffile-prefix-map=/tmp/go-build3052341145=/tmp/go-build -gno-record-gcc-switches'

What did you do?

Following https://pkg.go.dev/os#FindProcess, I have logic which assumes that on Linux systems, os.FindProcess will always succeed and that the resulting record can then be used with Signal to validate its actual existence.

However it appears the recent switch to using PIDFD within Go may have regressed this?
@kolyshkin

Example code:

package main

import (
	"fmt"
	"os"
	"syscall"
)

func main() {
	// Go doc says this will always work on Linux.
	pr, err := os.FindProcess(12345678)
	if err != nil {
		fmt.Printf("Supposedly unreachable code reached\n")
		os.Exit(1)
	}

	// Check if process exist by sending signal 0.
	err = pr.Signal(syscall.Signal(0))
	if err != nil {
		fmt.Printf("Process doesn't exist\n")
		os.Exit(0)
	}

	fmt.Printf("Process exists\n")
}

What did you see happen?

stgraber@castiana:~$ go version
go version go1.21.5 linux/amd64
stgraber@castiana:~$ go run ./main.go 
Process doesn't exist
stgraber@castiana:~$ 

What did you expect to see?

stgraber@castiana:~$ gotip version
go version devel go1.23-5d4e8f5 Thu Feb 22 01:57:32 2024 +0000 linux/amd64
stgraber@castiana:~$ gotip run ./main.go 
Supposedly unreachable code reached
exit status 1
stgraber@castiana:~$ 

[prattmic]

This behavior was intentionally changed in https://go.dev/cl/542699, merged just yesterday, under the belief that almost no code would depend on the old behavior. GODEBUG=osfinderr=0 was added to switch to the old behavior if a tiny number of programs do depend on the old behavior.

That said, receiving a report of breakage mere hours after merging the CL (thank you for proactively testing tip!) makes me suspect that dependence is more widespread than expected and that we should reconsider changing this behavior.

Could you share more about your original code that broke because of this change? Based on the issue title, I'm guessing the code did not check the error from FindProcess at all (since the docs said it would always succeed on Unix), and thus got a nil dereference when calling Process.Signal?

cc @ianlancetaylor @kolyshkin

[prattmic]

If we do need to change behavior, I see a few options:

1. Along with an error, also return a dummy "error" os.Process, which makes Process.Signal, etc unconditionally return an error.
2. Same as (1), but do not return an error, only the dummy "error" os.Process.

(1) would catch users that did not do error checking on FindProcess, but still allow early return for those that do. However, it could still break users if their code behaves differently on early error. (e.g., there is some extra cleanup that they do when Process.Signal fails that they don't do (but should) when FindProcess fails). (2) would fix those cases as well.

Neither case will work if users are legitimately depending on the racy behavior. e.g., they call FindProcess(1234) at startup and then occasionally poll Process.Signal to determine when process 1234 exists. I can't think of legitimate uses for this, but if they exist then I think the only option would be to document FindProcess as explicitly racy and introduce a new non-racy API.

@stgraber would any/all of these alternatives work for your application?

[stgraber]

Could you share more about your original code that broke because of this change? Based on the issue title, I'm guessing the code did not check the error from FindProcess at all (since the docs said it would always succeed on Unix), and thus got a nil dereference when calling Process.Signal?

That's correct, our code was calling FindProcess and then unconditionally call .Signal on the returned value. As this can now be nil, we ended up with a panic...

In our case I've simply sent a PR which now properly handles the err value of FindProcess so we'll be fine moving forward.

This unfortunately will not help anyone building any of our current stable releases with Go 1.23 as anyone doing that will be getting confusing failures.
(This isn't terribly likely in our case as we don't support our releases long enough for that to be an issue, but we may not be the only ones who followed that docstring and just never bothered even checking err for code that can only ever run on unix/linux).

[stgraber]

For us, 1) would work, so long as the error we get from Signal for that dummy error process ends up being os.ErrProcessDone.

I think that'd be the best option as anyone doing proper error checking on FindProcess (as we do now) will get the early result and save themselves the call to Signal. Anyone else who took the old docstring to the letter will still get the old behavior when calling Signal and crucially, no panics :)

What I'd really like to avoid here is other folks with code similar to ours out there, where the user decides to rebuild the application with the latest Go release. They'd get no compile time error at all and would have a resulting binary that appears to work fine until something triggers the code path that hits FindProcess+Signal and panics the whole thing.
That would be rather tricky for folks to debug and depending on the codebase, could be exploited by a remote unprivileged user as a way to DoS the system.

[bcmills]

I don't see a proposal associated with https://go.dev/cl/542699, and it seems like that sort of incompatible change really ought to have one. I suggest that we revert it and send the change through the proposal process to work out the compatibility implications in more detail.

[prattmic]

#62654 was originally a proposal, but it was taken out of the proposal process because it wasn't considered "a significant functionality change". Perhaps we should have put it back when we decided it was big enough to add a GODEBUG option.

[kolyshkin]

If we do need to change behavior, I see a few options:

1. Along with an error, also return a dummy "error" os.Process, which makes Process.Signal, etc unconditionally return an error.
2. Same as (1), but do not return an error, only the dummy "error" os.Process.

(1) would catch users that did not do error checking on FindProcess, but still allow early return for those that do. However, it could still break users if their code behaves differently on early error. (e.g., there is some extra cleanup that they do when Process.Signal fails that they don't do (but should) when FindProcess fails). (2) would fix those cases as well.

I think that (1) is the way to go; I'll work on that.

[kolyshkin]

I think that (1) is the way to go; I'll work on that.

https://go-review.googlesource.com/c/go/+/566179

[gopherbot]

Change https://go.dev/cl/566179 mentions this issue: os: FindProcess must not return nil

[kolyshkin]

@stgraber thanks for reporting that! I guess the scenario is quite common and it totally slipped my mind.

[kolyshkin]

@prattmic here is one more alternative to 1. and 2. proposed above. Best expressed as a diff:

 func findProcess(pid int) (p *Process, err error) {
        h, err := pidfdFind(pid)
        if err == ErrProcessDone {
-               return nil, err
+               return newProcess(pid, unsetHandle), err
        }
        // Ignore all other errors from pidfdFind,

This way, for users who do not check for errors from os.FindProcess will see the exactly same behavior as before, and those who do may see ErrProcessDone error.

OTOH this is worse than your proposal 1 (implemented in https://go.dev/cl/566179) because it won't hint the users to start checking for errors.

[prattmic]

Given that the best path forward for this issue isn't obvious and we have test failures as well (#65857), I think we should revert these CLs until we decide the correct approach (and fix the tests). I will send reverts.