Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

os: report more precise error from StartProcess #3649

Closed
hanwen opened this issue May 21, 2012 · 4 comments
Closed

os: report more precise error from StartProcess #3649

hanwen opened this issue May 21, 2012 · 4 comments

Comments

@hanwen
Copy link
Contributor

@hanwen hanwen commented May 21, 2012

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.
@rsc

This comment has been minimized.

Copy link
Contributor

@rsc rsc commented May 22, 2012

Comment 1:

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.

@hanwen

This comment has been minimized.

Copy link
Contributor Author

@hanwen hanwen commented May 22, 2012

Comment 2:

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.)
@hanwen

This comment has been minimized.

Copy link
Contributor Author

@hanwen hanwen commented Jun 13, 2012

Comment 3:

as discussed privately with Russ, signaling the error from syscall to os is not an
option, since it would require changing the API of syscall package. Instead, we can skip
the check if SysProcAttr != nil.
@rsc

This comment has been minimized.

Copy link
Contributor

@rsc rsc commented Jun 24, 2012

Comment 4:

This issue was closed by revision d36c095.

Status changed to Fixed.

@hanwen hanwen added fixed labels Jun 24, 2012
rsc added 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

»»»
@golang golang locked and limited conversation to collaborators Jun 24, 2016
This issue was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants
You can’t perform that action at this time.