[go] os/exec: second call to Cmd.Start is always an error
Alan Donovan (Gerrit)
Alan Donovan has uploaded the change for review![]( "Open in Gerrit")
Commit message
os/exec: second call to Cmd.Start is always an error
Previously it would return an error only if the first call
resulted in process creation, contra the intent of the
comment at exec.Cmd:
// A Cmd cannot be reused after calling its [Cmd.Start], [Cmd.Run],
// [Cmd.Output], or [Cmd.CombinedOutput] methods.
Fixes #76746
Change diff
diff --git a/src/os/exec/exec.go b/src/os/exec/exec.go
index e84ebfc..06c1839 100644
--- a/src/os/exec/exec.go
+++ b/src/os/exec/exec.go
@@ -102,6 +102,7 @@
"runtime"
"strconv"
"strings"
+ "sync/atomic"
"syscall"
"time"
)
@@ -354,6 +355,9 @@
// the work of resolving the extension, so Start doesn't need to do it again.
// This is only used on Windows.
cachedLookExtensions struct{ in, out string }
+
+ // startCalled records that Start was attempted, regardless of outcome.
+ startCalled atomic.Bool
}
// A ctxResult reports the result of watching the Context associated with a
@@ -635,7 +639,8 @@
func (c *Cmd) Start() error {
// Check for doubled Start calls before we defer failure cleanup. If the prior
// call to Start succeeded, we don't want to spuriously close its pipes.
- if c.Process != nil {
+ // It is an error to call Start twice even if the first call did not create a process.
+ if c.startCalled.Swap(true) {
return errors.New("exec: already started")
}
diff --git a/src/os/exec/exec_test.go b/src/os/exec/exec_test.go
index 1decebd..3c71ab0 100644
--- a/src/os/exec/exec_test.go
+++ b/src/os/exec/exec_test.go
@@ -1839,3 +1839,19 @@
}
})
}
+
+// Calling Start twice is an error, regardless of outcome.
+func TestStart_twice(t *testing.T) {
+ testenv.MustHaveExec(t)
+
+ cmd := exec.Command("/bin/nonesuch")
+ for i, want := range []string{
+ "fork/exec /bin/nonesuch: no such file or directory",
+ "exec: already started",
+ } {
+ err := cmd.Start()
+ if got := fmt.Sprint(err); got != want {
+ t.Errorf("Start call #%d return err %q, want %q", i+1, got, want)
+ }
+ }
+}
Change information
- M src/os/exec/exec.go
- M src/os/exec/exec_test.go
Related details
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alan Donovan (Gerrit)
Alan Donovan added 1 comment![]( "Open in Gerrit")
startCalled atomic.BoolHmm, I'm not sure this need to be atomic. There shouldn't be races on calls to Start et al.
Related details
- Damien Neil
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alan Donovan (Gerrit)
Alan Donovan added 1 comment![]( "Open in Gerrit")
"fork/exec /bin/nonesuch: no such file or directory",Windows spells this `exec: "/bin/nonesuch": executable file not found in %PATH%`
Related details
- Damien Neil
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ефим Верзаков (Gerrit)
Ефим Верзаков added 1 comment![]( "Open in Gerrit")
Can we add runc mantainer Kirill Kolyshkin as a Reviewer? The initial "bug" situation was found by testing runc. Also i suspect this can influence on some Kubernetes clusters based on Ubuntu 20.04 with runc 1.4 https://github.com/opencontainers/runc/issues/5060 because the setnsProcess is used to exec command into the container.
Related details
- Alan Donovan
- Damien Neil
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ефим Верзаков (Gerrit)
Ефим Верзаков added 1 comment![]( "Open in Gerrit")
}Can you add c.goroutine = nil here? I think it is not good to keep inappropriate fields in Cmd struct (funcs' pipes are closed in this defer). In successful scenario they are also deleted (line 768).
Ефим Верзаков (Gerrit)
Ефим Верзаков added 1 comment![]( "Open in Gerrit")
In Issue you have said that the user should recreate the New Cmd object. However, in runc Cmd object is created early https://github.com/opencontainers/runc/blob/v1.4.0/libcontainer/container_linux.go#L616 before it is started https://github.com/opencontainers/runc/blob/v1.4.0/libcontainer/process_linux.go#L370 . So, it will be more hard to recreate then copy. As for copy mechanism i have several notes. If it is just var cmd2 Cmd = *cmd1 then childIOFiles and parentIOPipes will be copied with its elements. And if Stdin/StdoutPipe is called before copy, then their Files will be closed twice in defer function. Also, by default Cancel funtion will try to kill the Process from the first cmd. If the first Cmd Starts returns an error then it will be nil pointer dereference. Link to playground https://play.golang.com/p/lJ6ivb8vhW_f . Also, i do not understand how to refresh ctx. I think it is need to implement copy mechanism or to document about hidden problems.
Alan Donovan (Gerrit)
Alan Donovan added 1 comment![]( "Open in Gerrit")
In Issue you have said that the user should recreate the New Cmd object. However, in runc Cmd object is created early https://github.com/opencontainers/runc/blob/v1.4.0/libcontainer/container_linux.go#L616 before it is started https://github.com/opencontainers/runc/blob/v1.4.0/libcontainer/process_linux.go#L370 . So, it will be more hard to recreate then copy. As for copy mechanism i have several notes. If it is just var cmd2 Cmd = *cmd1 then childIOFiles and parentIOPipes will be copied with its elements. And if Stdin/StdoutPipe is called before copy, then their Files will be closed twice in defer function. Also, by default Cancel funtion will try to kill the Process from the first cmd. If the first Cmd Starts returns an error then it will be nil pointer dereference. Link to playground https://play.golang.com/p/lJ6ivb8vhW_f . Also, i do not understand how to refresh ctx. I think it is need to implement copy mechanism or to document about hidden problems.
I think that runc is not using Cmd in a manner consistent with its documentation, and should be changed. I agree that copying the Cmd is not a solution, because it will copy the hidden fields too (and in general this is not a safe operation: consider a struct that contains a pointer to itself). But copying only the public fields seems like a safe solution. Alternatively, use some other structure to hold all the information needed to populate a Cmd, and create the Cmd just before you call Start.
Related details
- Damien Neil
- Ефим Верзаков
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Damien Neil (Gerrit)
Damien Neil added 2 comments![]( "Open in Gerrit")
Windows failures seem real?
startCalled atomic.BoolHmm, I'm not sure this need to be atomic. There shouldn't be races on calls to Start et al.
Seems harmless for it to be atomic, and this will ensure that misuse is detected.
Related details
- Alan Donovan
- Ефим Верзаков
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alan Donovan (Gerrit)
Alan Donovan uploaded new patchset![]( "Open in Gerrit")
- TryBots-Pass: LUCI-TryBot-Result-1 by Go LUCI
Related details
- Alan Donovan
- Ефим Верзаков
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alan Donovan (Gerrit)
Alan Donovan added 4 comments![]( "Open in Gerrit")
Alan DonovanWindows failures seem real?
Oh, no doubt, I was just taking my sweet time. Fixed.
Alan DonovanCan we add runc mantainer Kirill Kolyshkin as a Reviewer? The initial "bug" situation was found by testing runc. Also i suspect this can influence on some Kubernetes clusters based on Ubuntu 20.04 with runc 1.4 https://github.com/opencontainers/runc/issues/5060 because the setnsProcess is used to exec command into the container.
By all means we can add Kirill as a reviewer; done.
startCalled atomic.BoolDamien NeilHmm, I'm not sure this need to be atomic. There shouldn't be races on calls to Start et al.
Seems harmless for it to be atomic, and this will ensure that misuse is detected.
OK.
Windows spells this `exec: "/bin/nonesuch": executable file not found in %PATH%`
- Damien Neil
- Kirill Kolyshkin
- Ефим Верзаков
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alan Donovan (Gerrit)
Alan Donovan uploaded new patchset![]( "Open in Gerrit")
- Alan Donovan
- Damien Neil
- Kirill Kolyshkin
- Ефим Верзаков
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alan Donovan (Gerrit)
Alan Donovan voted and added 1 comment![]( "Open in Gerrit")
Votes added by Alan Donovan
| Commit-Queue | +1 |
1 comment
}Alan DonovanCan you add c.goroutine = nil here? I think it is not good to keep inappropriate fields in Cmd struct (funcs' pipes are closed in this defer). In successful scenario they are also deleted (line 768).
Done.
Damien, is fixing this additional minor lifetime/finalization issue within scope for a freeze CL?
Related details
- Alan Donovan
- Damien Neil
- Kirill Kolyshkin
- Ефим Верзаков
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Damien Neil (Gerrit)
Damien Neil added 2 comments![]( "Open in Gerrit")
Alan DonovanCan you add c.goroutine = nil here? I think it is not good to keep inappropriate fields in Cmd struct (funcs' pipes are closed in this defer). In successful scenario they are also deleted (line 768).
Done.
Damien, is fixing this additional minor lifetime/finalization issue within scope for a freeze CL?
- Alan Donovan
- Kirill Kolyshkin
- Ефим Верзаков
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alan Donovan (Gerrit)
Alan Donovan uploaded new patchset![]( "Open in Gerrit")
- TryBots-Pass: LUCI-TryBot-Result-1 by Go LUCI
Related details
- Alan Donovan
- Kirill Kolyshkin
- Ефим Верзаков
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alan Donovan (Gerrit)
Alan Donovan added 3 comments![]( "Open in Gerrit")
In Issue you have said that the user should recreate the New Cmd object. However, in runc Cmd object is created early https://github.com/opencontainers/runc/blob/v1.4.0/libcontainer/container_linux.go#L616 before it is started https://github.com/opencontainers/runc/blob/v1.4.0/libcontainer/process_linux.go#L370 . So, it will be more hard to recreate then copy. As for copy mechanism i have several notes. If it is just var cmd2 Cmd = *cmd1 then childIOFiles and parentIOPipes will be copied with its elements. And if Stdin/StdoutPipe is called before copy, then their Files will be closed twice in defer function. Also, by default Cancel funtion will try to kill the Process from the first cmd. If the first Cmd Starts returns an error then it will be nil pointer dereference. Link to playground https://play.golang.com/p/lJ6ivb8vhW_f . Also, i do not understand how to refresh ctx. I think it is need to implement copy mechanism or to document about hidden problems.
I think that runc is not using Cmd in a manner consistent with its documentation, and should be changed. I agree that copying the Cmd is not a solution, because it will copy the hidden fields too (and in general this is not a safe operation: consider a struct that contains a pointer to itself). But copying only the public fields seems like a safe solution. Alternatively, use some other structure to hold all the information needed to populate a Cmd, and create the Cmd just before you call Start.
Acknowledged
Can we add runc mantainer Kirill Kolyshkin as a Reviewer? The initial "bug" situation was found by testing runc. Also i suspect this can influence on some Kubernetes clusters based on Ubuntu 20.04 with runc 1.4 https://github.com/opencontainers/runc/issues/5060 because the setnsProcess is used to exec command into the container.
By all means we can add Kirill as a reviewer; done.
- Damien Neil
- Kirill Kolyshkin
- Ефим Верзаков
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |