os: on unix Process.Kill() can kill the wrong process #13987

ibukanov · 2016-01-17T20:57:29Z

This is somewhat similar to the issue #9382. On Unix Process.Kill() just calls Process.Signal(Kill). As https://golang.org/src/os/exec_unix.go#L39 indicates, the Signal function invokes syscall.Kill(p.Pid) outside any lock after checking if the process still runs. Thus at the point when the signal is called the process could finish and Process.Wait() from another thread return. Thus OS is free to reuse the pid for another unrelated process. If this happens, Process.Kill() kills that process.

Due to this race it is impossible to write a correct platform-independent code in Go that kills the process if it does not terminate after a pause. I.e. the code fragment from #9382 is not correct on Unix:

func spawnAndKill(exePath string, counter int) error {
    cmd := exec.Command(exePath, fmt.Sprintf("%d", counter))
    err := cmd.Start()
    if err != nil {
        return err
    }
    go func() {
        time.Sleep(1000 * time.Millisecond)
        cmd.Process.Kill()
    }()
    cmd.Wait()
    return nil
}

The text was updated successfully, but these errors were encountered:

minux · 2016-01-17T22:58:08Z

The spwanAndKill func is racy. The cmd.Process.Kill() races with cmd.Wait. I think this is working as intended, there is no way for Go to prevent this race (say, we make Kill check if the waitpid has returned, but that is still racy, as waitpid could finish anytime after the check and before the kill syscall). It's the programmer's responsbility to eliminate the race. If you want to kill the process after a timeout, just wait for the timeout, kill it unconditionally, and then Wait.

ibukanov · 2016-01-18T07:34:59Z

@minux On Windows the above code is not racy and works correctly due to fixes that went into #9382. On Linux I can also implement the same semantics correctly via waiting for SIGCHLD and using non-blocking wait4 syscall. That allows to synchronize with kill attempts. However, this is non-portable.

What I would like to see is an option to write a portable Go code implementing the kill-if-not-finished-after-timeout pattern. If this requires new API, so be it, but at least document loudly that calling Process.Kill and Process.Wait from different threads at the same time could race and should be avoided in portable code.

minux · 2016-01-18T22:51:05Z

There is a portable way to implement the functionality. Just wait for the timeout, kill the process unconditionally and then wait. The suggested workaround for unix is too complicated (not to mention it needs one goroutine to pull with non- blocking wait4) when there is easy high-level workaround.

ianlancetaylor · 2016-01-19T00:03:30Z

I think we can do it without a race, and without an extra goroutine, if we add something to the runtime package to count SIGCHLD signals, and provide a way for a goroutine to retrieve the current SIGCHLD count and a way to sleep until the SIGCHLD counter is greater than N.

ibukanov · 2016-01-19T06:49:50Z

@minux The point is precisely not to wait to timeout as typically the process finishes much faster. In my case the timeout is really for runaway cases when something bad happens or the system is too busy. I also cannot just start another go routine that does kill/wait as I need to get the process return status to decide if everything is OK.

@ianlancetaylor Note I do not suggest to change the current implementation. Rather I suggest to document loudly that one should avoid Wait()/Kill() race in portable code. Then add some new API that allows to implement the desired functionality. For example, a version of exec.Cmd.Wait that takes a channel and waits until the child finishes or there is input on the channel is enough to implement all my use cases.

ianlancetaylor · 2016-01-19T18:04:34Z

If we can change the existing API so that it avoids a race, I think that is clearly better than introducing a new API.

minux · 2016-01-20T03:02:17Z

I still think the user code should be able to avoid this race
as much as possible.

For example, something like this should be pretty safe
without any complications in runtime and it also have
the benefit that as soon as the process exits, the timeout
goroutine will exit soon, rather than wait until the timer
expires.

func spawnAndKill(exePath string, counter int) error {
cmd := exec.Command(exePath, fmt.Sprintf("%d", counter))
err := cmd.Start()
if err != nil {
return err
}
ch := make(chan bool)
go func() {
select {
case <-time.After(1000 * time.Millisecond):
cmd.Process.Kill()
case <-ch:
return
}
}()
cmd.Wait()
close(ch)
return nil
}

ibukanov · 2016-01-20T08:01:44Z

The above code just minimizes the chances of race. It is still there. As it is impossible to make its probability arbitrary small, one should avoid it, say, in production code.

gopherbot · 2016-06-10T06:00:20Z

CL https://golang.org/cl/23967 mentions this issue.

On systems that support the POSIX.1-2008 waitid function, we can use it to block until a wait will succeed. This avoids a possible race condition: if a program calls p.Kill/p.Signal and p.Wait from two different goroutines, then it is possible for the wait to complete just before the signal is sent. In that case, it is possible that the system will start a new process using the same PID between the wait and the signal, causing the signal to be sent to the wrong process. The Process.isdone field attempts to avoid that race, but there is a small gap of time between when wait returns and isdone is set when the race can occur. This CL avoids that race by using waitid to wait until the process has exited without actually collecting the PID. Then it sets isdone, then waits for any active signals to complete, and only then collects the PID. No test because any plausible test would require starting enough processes to recycle all the process IDs. Update #13987. Update #16028. Change-Id: Id2939431991d3b355dfb22f08793585fc0568ce8 Reviewed-on: https://go-review.googlesource.com/23967 Run-TryBot: Ian Lance Taylor <iant@golang.org> Reviewed-by: Austin Clements <austin@google.com> TryBot-Result: Gobot Gobot <gobot@golang.org>

ianlancetaylor · 2016-06-10T18:12:00Z

I believe this race is now fixed on GNU/Linux.

Leaving the issue open as the fix will have to be implemented on other Unix systems. Systems that provide waitid (which should be most of them, since it is in POSIX.1-2008) need to implement that; it's slightly painful since it's not in the syscall package.

gopherbot · 2016-06-11T11:00:20Z

CL https://golang.org/cl/24021 mentions this issue.

gopherbot · 2016-06-11T11:00:22Z

CL https://golang.org/cl/24020 mentions this issue.

This change is a followup to https://go-review.googlesource.com/23967 for Darwin. Updates #13987. Updates #16028. Change-Id: Ib1fb9f957fafd0f91da6fceea56620e29ad82b00 Reviewed-on: https://go-review.googlesource.com/24020 Reviewed-by: Ian Lance Taylor <iant@golang.org>

This change is a followup to https://go-review.googlesource.com/23967 for FreeBSD. Updates #13987. Updates #16028. Change-Id: I0f0737372fce6df89d090fe9847305749b79eb4c Reviewed-on: https://go-review.googlesource.com/24021 Reviewed-by: Ian Lance Taylor <iant@golang.org>

Follow CL 23967 and CL 24021 which did the same on linux and freebsd, respectively. Updates golang#13987 Updates golang#16028 Change-Id: Ia30ef8b5cffd8f9eb75c29ee5fe350dac2be6d44

gopherbot · 2021-05-01T18:13:33Z

Change https://golang.org/cl/315279 mentions this issue: os, syscall: use wait6 to avoid wait/kill race on dragonfly

gopherbot · 2021-05-02T11:25:28Z

Change https://golang.org/cl/315281 mentions this issue: os, syscall: use wait6 to avoid wait/kill race on netbsd

Follow CL 23967 and CL 24021 which did the same on linux and freebsd, respectively. Updates #13987 Updates #16028 Change-Id: Ia30ef8b5cffd8f9eb75c29ee5fe350dac2be6d44 Reviewed-on: https://go-review.googlesource.com/c/go/+/315279 Trust: Tobias Klauser <tobias.klauser@gmail.com> Run-TryBot: Tobias Klauser <tobias.klauser@gmail.com> TryBot-Result: Go Bot <gobot@golang.org> Reviewed-by: Ian Lance Taylor <iant@golang.org>

Follow CL 23967 and CL 24021 which did the same on linux and freebsd, respectively. Updates #13987 Updates #16028 Change-Id: I95b13d8ddde4cea1ef4fb7d655f1ad1a219d13aa Reviewed-on: https://go-review.googlesource.com/c/go/+/315281 Trust: Tobias Klauser <tobias.klauser@gmail.com> Run-TryBot: Tobias Klauser <tobias.klauser@gmail.com> Reviewed-by: Ian Lance Taylor <iant@golang.org>

gopherbot · 2021-05-29T00:30:55Z

Change https://golang.org/cl/323489 mentions this issue: doc/go1.17: mention SYS_WAIT6/WEXITED on NetBSD

For #13987 For #16028 For #44513 Change-Id: I7a73446fcc80a01fa6de24eec1e5b993e543be37 Reviewed-on: https://go-review.googlesource.com/c/go/+/323489 Trust: Ian Lance Taylor <iant@golang.org> Reviewed-by: Emmanuel Odeke <emmanuel@orijtech.com>

gopherbot · 2021-10-06T09:40:42Z

Change https://golang.org/cl/354249 mentions this issue: os: don't use wait6 on netbsd

CL 315281 changed the os package use wait6 on netbsd. This seems to be causing frequent test failures as reported in #48789. Revert that change using wait6 on netbsd for now. Updates #13987 Updates #16028 For #48789 Change-Id: Ieddffc65611c7f449971eaa8ed6f4299a5f742c2 Reviewed-on: https://go-review.googlesource.com/c/go/+/354249 Trust: Tobias Klauser <tobias.klauser@gmail.com> Trust: Bryan C. Mills <bcmills@google.com> Trust: Benny Siegert <bsiegert@gmail.com> Run-TryBot: Tobias Klauser <tobias.klauser@gmail.com> TryBot-Result: Go Bot <gobot@golang.org> Reviewed-by: Bryan C. Mills <bcmills@google.com> Reviewed-by: Benny Siegert <bsiegert@gmail.com> Reviewed-by: Ian Lance Taylor <iant@golang.org>

gopherbot · 2022-09-19T15:27:40Z

Change https://go.dev/cl/431855 mentions this issue: os: use wait6 to avoid wait/kill race on netbsd

Resend of CL 315281 which was partially reverted by CL 354249 after the original CL was suspected to cause test failures as reported in #48789. It seems that both wait4 and wait6 lead to that particular deadlock, so let's use wait6. That way we at least don't hit #13987 on netbsd. Updates #13987 For #48789 For #50138 Change-Id: Iadc4a771217b7e9e821502e89afa07036e0dcb6f Reviewed-on: https://go-review.googlesource.com/c/go/+/431855 Reviewed-by: Benny Siegert <bsiegert@gmail.com> Auto-Submit: Tobias Klauser <tobias.klauser@gmail.com> Run-TryBot: Tobias Klauser <tobias.klauser@gmail.com> Reviewed-by: Bryan Mills <bcmills@google.com> TryBot-Result: Gopher Robot <gobot@golang.org>

gopherbot · 2022-10-12T17:41:21Z

Change https://go.dev/cl/442478 mentions this issue: os: use the correct constant for P_PID on NetBSD

gopherbot · 2022-10-12T19:31:25Z

Change https://go.dev/cl/442575 mentions this issue: os: split wait6 syscall wrapper into per-platform files

Dragonfly and FreeBSD both used numerical values for these constants chosen to be the same as on Solaris. For some reason, NetBSD did not, and happens to interpret value 0 as P_ALL instead of P_PID (see https://github.com/NetBSD/src/blob/3323ceb7822f98b3d2693aa26fd55c4ded6d8ba4/sys/sys/idtype.h#L43-L44). Using the correct value for P_PID should cause wait6 to wait for the correct process, which may help to avoid the deadlocks reported in For #50138. Updates #13987. Change-Id: I0eacd1faee4a430d431fe48f9ccf837f49c42f39 Reviewed-on: https://go-review.googlesource.com/c/go/+/442478 Auto-Submit: Bryan Mills <bcmills@google.com> Reviewed-by: Benny Siegert <bsiegert@gmail.com> Run-TryBot: Bryan Mills <bcmills@google.com>

There are getting to be enough special cases in this wrapper that the increase in clarity from having a single file is starting to be outweighed by the complexity from chained conditionals. Updates #50138. Updates #13987. Change-Id: If4f1be19c0344e249aa6092507c28363ca6c8438 Reviewed-on: https://go-review.googlesource.com/c/go/+/442575 Run-TryBot: Bryan Mills <bcmills@google.com> TryBot-Result: Gopher Robot <gobot@golang.org> Auto-Submit: Bryan Mills <bcmills@google.com> Reviewed-by: Ian Lance Taylor <iant@google.com>

Dragonfly and FreeBSD both used numerical values for these constants chosen to be the same as on Solaris. For some reason, NetBSD did not, and happens to interpret value 0 as P_ALL instead of P_PID (see https://github.com/NetBSD/src/blob/3323ceb7822f98b3d2693aa26fd55c4ded6d8ba4/sys/sys/idtype.h#L43-L44). Using the correct value for P_PID should cause wait6 to wait for the correct process, which may help to avoid the deadlocks reported in For golang#50138. Updates golang#13987. Change-Id: I0eacd1faee4a430d431fe48f9ccf837f49c42f39 Reviewed-on: https://go-review.googlesource.com/c/go/+/442478 Auto-Submit: Bryan Mills <bcmills@google.com> Reviewed-by: Benny Siegert <bsiegert@gmail.com> Run-TryBot: Bryan Mills <bcmills@google.com>

There are getting to be enough special cases in this wrapper that the increase in clarity from having a single file is starting to be outweighed by the complexity from chained conditionals. Updates golang#50138. Updates golang#13987. Change-Id: If4f1be19c0344e249aa6092507c28363ca6c8438 Reviewed-on: https://go-review.googlesource.com/c/go/+/442575 Run-TryBot: Bryan Mills <bcmills@google.com> TryBot-Result: Gopher Robot <gobot@golang.org> Auto-Submit: Bryan Mills <bcmills@google.com> Reviewed-by: Ian Lance Taylor <iant@google.com>

gopherbot · 2023-09-14T21:58:13Z

Change https://go.dev/cl/528439 mentions this issue: os: use PidfdSendSignal on linux if possible

gopherbot · 2023-09-16T02:57:32Z

Change https://go.dev/cl/528798 mentions this issue: os: make use of pidfd on linux

gopherbot · 2023-11-16T09:48:25Z

Change https://go.dev/cl/528438 mentions this issue: os: make use of pidfd on linux

Use Process.handle field to store pidfd, and make use of it. Only use pidfd functionality if all the needed syscalls are available. 1. StartProcess: obtain the pidfd from the kernel, if available, using the functionality added by CL 520266. Note we could not modify syscall.StartProcess to return pidfd directly because it is a public API and its callers do not expect it, so we have to use ensurePidfd and getPidfd. 2. (*Process).Kill: use pidfdSendSignal, if the syscall is available and pidfd is known. This is slightly more complicated than it should be, since the syscall can be blocked by e.g. seccomp security policy, therefore the need for a function to check if it's actually working, and a soft fallback to kill. Perhaps this precaution is not really needed. 3. (*Process).Wait: use pidfdWait, if available, otherwise fall back to using waitid/wait4. This is also more complicated than expected due to struct siginfo_t idiosyncrasy. NOTE pidfdSendSignal and pidfdWait are used without a race workaround (blockUntilWaitable and sigMu, added by CL 23967) because with pidfd, PID recycle issue doesn't exist (IOW, pidfd, unlike PID, is guaranteed to refer to one particular process) and thus the race doesn't exist either. For #62654. Updates #13987. Change-Id: I22ebcc7142b16a3a94c422d2f32504d1a80e8a8f Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com> Reviewed-on: https://go-review.googlesource.com/c/go/+/528438 TryBot-Result: Gopher Robot <gobot@golang.org> Reviewed-by: Michael Pratt <mpratt@google.com> Reviewed-by: Michael Knyszek <mknyszek@google.com> Reviewed-by: Ian Lance Taylor <iant@golang.org> LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>

gopherbot · 2024-03-08T18:19:47Z

Change https://go.dev/cl/570036 mentions this issue: os: make use of pidfd on linux

ianlancetaylor added this to the Unplanned milestone Jan 18, 2016

ianlancetaylor mentioned this issue Jun 10, 2016

os/exec: race between Wait and ctx.Done #16028

Closed

ianlancetaylor added OS-Dragonfly OS-FreeBSD OS-Darwin OS-NetBSD OS-OpenBSD OS-Solaris labels Jun 10, 2016

ianlancetaylor removed OS-Darwin OS-FreeBSD labels Jun 13, 2016

tklauser removed the OS-Dragonfly label May 2, 2021

tklauser removed the OS-NetBSD label May 4, 2021

bcmills mentioned this issue Oct 5, 2021

os: on NetBSD, *Process.Wait sometimes deadlocks after cmd.Process.Signal returns "process already finished" #48789

Closed

bcmills mentioned this issue Jan 4, 2022

os/exec: add fields for managing termination signals and pipes #50436

Closed

tklauser mentioned this issue Sep 19, 2022

os: (*Process).Wait sometimes hangs on netbsd #50138

Closed

An00bRektn mentioned this issue Jan 10, 2023

Known Issue: SIGSEGV when using os.Process.Kill() on Linux An00bRektn/gopher47#1

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

os: on unix Process.Kill() can kill the wrong process #13987

os: on unix Process.Kill() can kill the wrong process #13987

ibukanov commented Jan 17, 2016

minux commented Jan 17, 2016 via email

ibukanov commented Jan 18, 2016

minux commented Jan 18, 2016 via email

ianlancetaylor commented Jan 19, 2016

ibukanov commented Jan 19, 2016

ianlancetaylor commented Jan 19, 2016

minux commented Jan 20, 2016

ibukanov commented Jan 20, 2016

gopherbot commented Jun 10, 2016

ianlancetaylor commented Jun 10, 2016

gopherbot commented Jun 11, 2016

gopherbot commented Jun 11, 2016

gopherbot commented May 1, 2021

gopherbot commented May 2, 2021

gopherbot commented May 29, 2021

gopherbot commented Oct 6, 2021

gopherbot commented Sep 19, 2022

gopherbot commented Oct 12, 2022

gopherbot commented Oct 12, 2022

gopherbot commented Sep 14, 2023

gopherbot commented Sep 16, 2023

gopherbot commented Nov 16, 2023

gopherbot commented Mar 8, 2024

os: on unix Process.Kill() can kill the wrong process #13987

os: on unix Process.Kill() can kill the wrong process #13987

Comments

ibukanov commented Jan 17, 2016

minux commented Jan 17, 2016 via email

ibukanov commented Jan 18, 2016

minux commented Jan 18, 2016 via email

ianlancetaylor commented Jan 19, 2016

ibukanov commented Jan 19, 2016

ianlancetaylor commented Jan 19, 2016

minux commented Jan 20, 2016

ibukanov commented Jan 20, 2016

gopherbot commented Jun 10, 2016

ianlancetaylor commented Jun 10, 2016

gopherbot commented Jun 11, 2016

gopherbot commented Jun 11, 2016

gopherbot commented May 1, 2021

gopherbot commented May 2, 2021

gopherbot commented May 29, 2021

gopherbot commented Oct 6, 2021

gopherbot commented Sep 19, 2022

gopherbot commented Oct 12, 2022

gopherbot commented Oct 12, 2022

gopherbot commented Sep 14, 2023

gopherbot commented Sep 16, 2023

gopherbot commented Nov 16, 2023

gopherbot commented Mar 8, 2024