os: report more precise error from StartProcess #3649
Labels
Comments
If we remove the current check, we need to do something else to address the problem it is fixing. Maybe it would work to send back more than just the one number from the child process. If we sent back both what was going on and the error, that might be enough. The problem is that the child sends back <error-from-chdir> and then the parent makes an error that says exec program: <error-from-chdir> which inevitably ends up saying something confusing like exec program: path does not exist which is bogus. The current code checks the directory early so that it can return an error like chdir dir: path does not exist instead. Status changed to Accepted. |
I understand that exec() errors can be confounding. Since there is a pipe in startProcess() for communication, the child process could simply send a complete error string down the pipe, maybe prefixed with an errno number? If that is not possible within the constraints (no malloc, no locks, etc.) in the child, we could send the syscall number instead, so the error would be "SYS_CHDIR failed: ENOENT" (I'd be happy to try to send a patch, it's overdue for me to get my hands dirty with the go source code.) |
|
This issue was closed by revision d36c095. Status changed to Fixed. |
rsc
pushed a commit
that referenced
this issue
May 11, 2015
««« backport 2aaa88600d48 os: make POSIX StartProcess work with chroot again. Skip directory check in startProcess in the presence of SysProcAttr. Fixes #3649. R=golang-dev, rsc CC=golang-dev https://golang.org/cl/6297083 »»»
This issue was closed.
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
What steps will reproduce the problem? 1. Use os/exec with syscall.SysProcAttr{Chroot: ... } for cmd.SysProcAttr. 2. Try to run; this will fail, since src/pkg/os/exec_posix.go does func StartProcess(name string, argv []string, attr *ProcAttr) (p *Process, err error) { + // Double-check existence of the directory we want + // to chdir into. We can make the error clearer this way. + if attr != nil && attr.Dir != "" { + if _, err := Stat(attr.Dir); err != nil { + pe := err.(*PathError) + pe.Op = "chdir" + return nil, pe + } + } + Of course, this should rather stat attr.Sys.Chroot + "/" + attr.Dir. Working around this problem is tricky, since newProcess(pid, h) (used in startProcess) is not available outside os package. I'm not sure if the check is a good idea at all, given that setgid/setuid etc. could also cause the stat to fail spuriously too. Which compiler are you using (5g, 6g, 8g, gccgo)? 6g Which operating system are you using? linux. Which version are you using? (run 'go version') hanwen@oats:~/vc/go$ hg sum parent: 12994:2ccfd4b451d3 release go1.0.1 go1.0.1 branch: release-branch.go1 commit: 4 unknown (clean) update: 2 new changesets (update) Please provide any additional information below.The text was updated successfully, but these errors were encountered: