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/exec: Cmd produces misleading error when using Setpgid #20285

Open
aronatkins opened this issue May 8, 2017 · 6 comments

Comments

Projects
None yet
4 participants
@aronatkins
Copy link

commented May 8, 2017

What version of Go are you using (go version)?

$ go version
go version go1.8.1 darwin/amd64

What operating system and processor architecture are you using (go env)?

$ go env
GOARCH="amd64"
GOBIN=""
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOOS="darwin"
GOPATH="/Users/aron/go"
GORACE=""
GOROOT="/usr/local/go"
GOTOOLDIR="/usr/local/go/pkg/tool/darwin_amd64"
GCCGO="gccgo"
CC="clang"
GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/ts/s940qvdj5vj1czr9qh07fvtw0000gn/T/go-build901749947=/tmp/go-build -gno-record-gcc-switches -fno-common"
CXX="clang++"
CGO_ENABLED="1"
PKG_CONFIG="pkg-config"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"

What did you do?

package main

import (
	"fmt"
	"os/exec"
	"syscall"
)

func main() {
	var cmd *exec.Cmd
	var err error

	cmd = exec.Command("/bin/ls")
	cmd.Dir = "/this/does/not/exist"
	cmd.SysProcAttr = &syscall.SysProcAttr{
		Setpgid: true,
	}
	err = cmd.Run()
	fmt.Printf("error was: %v\n", err)

	cmd = exec.Command("/bin/ls")
	cmd.Dir = "/this/does/not/exist"
	err = cmd.Run()
	fmt.Printf("error was: %v\n", err)
}

What did you expect to see?

$ go run baddir.go
error was: chdir /this/does/not/exist: no such file or directory
error was: chdir /this/does/not/exist: no such file or directory

What did you see instead?

$ go run baddir.go
error was: fork/exec /bin/ls: no such file or directory
error was: chdir /this/does/not/exist: no such file or directory

Note that the error string makes the reader believe that "/bin/ls" is the thing that may not exist. Compare when not using Setpgid; the working directory is properly reported as not existing.

@aronatkins

This comment has been minimized.

Copy link
Author

commented May 8, 2017

This issue is also present on a linux/amd64 system (still using Go 1.8.1).

@bradfitz bradfitz changed the title exec.Cmd produces misleading error when using Setpgid os/exec: Cmd produces misleading error when using Setpgid May 8, 2017

@bradfitz bradfitz added this to the Go1.9Maybe milestone May 8, 2017

@aronatkins

This comment has been minimized.

Copy link
Author

commented May 8, 2017

Workaround: Test that the configured working directory exists before attempting to exec a process.

@aronatkins

This comment has been minimized.

Copy link
Author

commented May 8, 2017

Looks like the presence of a SysProcAttr is enough to avoid this pre-check (which produces the nice error message):

go/src/os/exec_posix.go

Lines 22 to 31 in 88672de

// If there is no SysProcAttr (ie. no Chroot or changed
// UID/GID), double-check existence of the directory we want
// to chdir into. We can make the error clearer this way.
if attr != nil && attr.Sys == nil && attr.Dir != "" {
if _, err := Stat(attr.Dir); err != nil {
pe := err.(*PathError)
pe.Op = "chdir"
return nil, pe
}
}

@bradfitz bradfitz modified the milestones: Go1.9Maybe, Go1.10 Jul 20, 2017

@odeke-em

This comment has been minimized.

Copy link
Member

commented Dec 6, 2017

Hello @aronatkins, sorry that we hadn't looked at this.

Am I right to allude that you'd suggest that we Stat for presence of the directory regardless of
if we have the SysProcAttr or not?
If so, that code was updated by @hanwen's CL https://golang.org/cl/6297083 to solve issue #3649.
Interestingly I think @rsc postulated in #3649 (comment), such a problem as you've reported.

I'll invite @hanwen and @rsc to chime in, and perhaps we might punt this bug to Go1.11. If that happens apologies for that, we'll most definitely work on it during Go1.11.

@aronatkins

This comment has been minimized.

Copy link
Author

commented Dec 6, 2017

@odeke-em Thanks for the update. I'm OK with shifting this out of Go 1.10.

From reading #3649, it appears that there are certain SysProcAttr configurations where we want to test the Dir directly, certain cases where that directory might need to be considered with the chroot, and certain cases where the error needs to come from deeper in the call stack. I don't have enough context to see the change that addresses all these cases.

@rsc's comment #3649 (comment) is exactly what this issue is tracking.

@odeke-em odeke-em modified the milestones: Go1.10, Go1.11 Dec 17, 2017

@odeke-em

This comment has been minimized.

Copy link
Member

commented Dec 17, 2017

Cool, milestone set to Go1.11, please feel free to revert if there is something that we can do for Go1.10.

@gopherbot gopherbot modified the milestones: Go1.11, Unplanned May 23, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.