os/exec: Command does a LookPath in the wrong place.

[gopherbot]

by david@scraperwiki.com:

What steps will reproduce the problem?
If possible, include a link to a program on play.golang.org.

http://play.golang.org/p/3MjLGe5gFd

If I am planning to set the .Dir property of a Cmd object before calling .Run(), it
probably will not work because exec.Command() curses the Cmd object.

If the filename of a binary passed to exec.Command() does not exist then the returned
Cmd object is cursed and will not .Run() even if Dir is assigned before .Run() is called.

In the example in the code, exec.Command("./ls") followed by .Dir =
"/bin" followed by .Run() does not work, even though /bin/ls exists. (first
.Run() invocation in the code).

Internally, what is happening is that exec.Command() is calling LookPath and setting the
.err property. However, if "./ls" exists, then the LookPath will succeed, and
the .Run() will work, and it will run "/bin/ls" instead of "./ls"
(second invocation in the code).

The upshot is that exec.Command() should not be doing the LookPath. The LookPath should
be done only when .Run() is called, because .Dir may change.


What is the expected output?

We should see:
<a listing of the /bin directory>
<date and time> error: <nil>
<a listing of the /bin directory>
<date and time> error: <nil>

(note that this is the same thing twice)


What do you see instead?

2014/01/28 17:02:57 error: exec: "./ls": stat ./ls: no such file or directory
<a listing of the /bin directory>
<date and time> error: <nil>

(note that this has an error message followed by one part of the expected output)



Which compiler are you using (5g, 6g, 8g, gccgo)?

WAT?

Which operating system are you using?

$ uname -a
Linux ash 3.8.0-32-generic #47~precise1-Ubuntu SMP Wed Oct 2 16:19:35 UTC 2013 x86_64
x86_64 x86_64 GNU/Linux


Which version are you using?  (run 'go version')

$ go version
go version go1.1.2 linux/amd64

Please provide any additional information below.

Note that the sandbox provided by the go playground doesn't have a "/bin"
directory, but we can still see evidence of the buggy behaviour, because we get two
different error messages.

[bradfitz]

Comment 1:

We're unlikely to change the behavior at this point, for compatibility reasons.  Also,
there's a work-around: don't use the exec.Command utility function.  Just build the Cmd
by hand if that's what you want.
If anything, this might be a documentation issue.  Documentation suggestions welcome.

Labels changed: added release-go1.3maybe, repo-main.

Status changed to WaitingForReply.

[gopherbot]

Comment 2 by peter.waller:

Hi Brad,
This bug lead to a nasty issue where we were convinced that something did work when it
actually didn't. Also, the error message given was very confusing since initially it
seemed like the stat system call was failing, except it was impossible to see why.
Surely this could be fixed by: if LookPath returns an error, check to see if the
cmd.Path is valid with respect to cmd.Dir?
Then you couldn't end up in the extremely confusing situation where the success of the
Command() call depends on the existence of a file which is not being executed.

[adg]

Comment 3:

I don't think Cmd.Dir should affect the behavior of the implicit LookPath. The LookPath
part is done by the current process, while the Dir is where the new process is being
executed.
Your proposed fix may change the behavior of existing programs, and I don't think it's
the right thing to do anyway.
If you know the directory you want to run in, use exec.Command(filepath.Join(dir, bin))
in the first place.
Agree that there might be some documentation concerns here.

[gopherbot]

Comment 4 by peter@scraperwiki.com:

Hm. I'm not sure documentation would have prevented the multiple hours it took for us to
understand why our program wasn't working, because the principle of least surprise is
violated. Here's the story:
We were in the unfortunate situation where something called "./foo" happened to be in
the working directory of the process and executable, by happen-stance. Then we did the
equivalent of Command(Dir: bar, Path: ./foo).Run(), and bar/foo (a completely different
program with the same name) was correctly executed, as expected. At some time much later
"./foo" was removed from the filesystem.
At this point the program stopped functioning with the error "error: exec: "./foo": stat
./foo: no such file or directory". It was very hard to comprehend why the behaviour of
the program had changed since the critical fact was that the behaviour of the program
depended on something which had never been executed by our program in the past.
The effect of fixing this that I would like to have is to remove that dependency,
because it was very hard to find. It would have been the case if there was documentation
in front of my face because I had *seen it work correctly* before and I had
exec.Command's source code in front of me. I was thinking that I was seeing a filesystem
problem.
I'm happy that I can use exec.Command(filepath.Join()) and that maybe LookPath shouldn't
look in cmd.Dir, but can this surprising behaviour really not be fixed? I understand and
appreciate the importance of not changing the behaviour of programs in the wild, but can
we demonstrate that it would be harmful to fix the issue as I've described it here?

[bradfitz]

Comment 5:

If you can think of a fix that doesn't change break any existing valid programs, perhaps.
But since we currently do LookPath in the current directory, we can't remove that.  So
then if we changed things, we'd be in a situation where we first LookPath in the current
directory and only if that fails, then LookPath in the cmd.Dir later at Run time. 
That's even more surprising: now there's two paths that are searched, and it depends on
the context (current dir) of the program where it was run which one is used.
So I really doubt we can change it.
Maybe we can improve the error messages, though.

[adg]

Comment 6:

So what's the proposed change? That we add a second LookPath from Cmd.Dir on Run, but
only if the first one failed? To do that we'd need to add more unexported state to the
Cmd struct to signify that the Command function was used to construct the Cmd and that
the LookPath failed.
I have three concerns with this change:
1. It makes the behavior of Command and Cmd's run methods do surprising things. I
certainly wouldn't expect it to try LookPath in two separate locations.
2. It will change the behavior of existing programs. Even if not technically backward
compatible, it adds complexity. 
3. I just don't think it's the right behavior. The directory in which you exec a process
has nothing to do with the location of the process binary. Also, you're specifying the
command path before setting the Dir field, so it's not clear that you *should* be
expecting it to look inside Dir for the binary. (You yourself said that if you hadn't
seen it apparently working before, you wouldn't have thought it would be the case.)
I think this is just one of those instances in debugging where your assumptions were
wrong and you went down a blind alley. Happens to the best of us.
This is all IMO, and my word is not necessarily final. I am happy to discuss it further.

[gopherbot]

Comment 7 by david@scraperwiki.com:

With respect to #3 "The directory in which you exec a process has nothing to do with the
location of the process binary": if you think that is an assertion about the current
behaviour then you are wrong. Look at the original example. The command is "./ls", but
the process binary that gets executed is "/bin/ls".

[adg]

Comment 8:

I would be incredibly surprised that LookPath("./ls") should return "/bin/ls" when run
outside of "/bin".
This program:
package main
import (
        "fmt"
        "os/exec"
)
func main() {
        fmt.Println(exec.LookPath("./ls"))
        fmt.Println(exec.LookPath("ls"))
        fmt.Println(exec.Command("./ls").Run())
        fmt.Println(exec.Command("ls").Run())
}
Returns this output on my system:
$ go run lookpath.go 
 exec: "./ls": stat ./ls: no such file or directory
/bin/ls <nil>
exec: "./ls": stat ./ls: no such file or directory
<nil>
which seems correct to me.
And if I "cd /bin" I see what I would expect also:
$ cd /bin
$ go run /Users/adg/test/lookpath.go 
./ls <nil>
/bin/ls <nil>
<nil>
<nil>

[adg]

Comment 9:

Sorry I was unclear in my statement. I meant that cmd.Dir has nothing to do with
cmd.Path.

[gopherbot]

Comment 10 by david@scraperwiki.com:

#8 I agree LookPath returns "./ls", but when .Dir is set before .Run() is called, then
the actual process binary that gets run can be in another location.
In the original example, why doesn't the executable script called "ls" in the current
directory get executed (it is empty, so should have the same behaviour as true)?

[adg]

Comment 11:

Ah, I see the problem now.
That is really subtle.
I had thought (wrongly) that forkexec was a singular syscall that takes a directory name
as well as the program path. That's not the case. forkAndExecInChild is a function in
the syscall package that forks, chdirs to the directory, and then execs the program.
Knowing that, it's obvious why running "./ls" with a Dir field set would run the "ls"
binary from the that directory, and never the one from the working directory.
Ugh.

[ianlancetaylor]

Comment 12:

On Unix systems, I suggest that if Command is called with a relative path (a path
containing a '/' that does not start with a '/') then it should not call LookPath.  That
is in fact the documented behaviour.  Calling LookPath in that case does not actually do
anything except check whether the named file exists, which will be done by the exec call
anyhow.
Unfortunately we can't literally not call LookPath as that will do the wrong thing on
Plan 9 and Windows in some cases.  So instead I suggest that if Command is called with a
relative path, and LookPath returns an exec.Error whose err value is an os.IsNotExist
error, Command should ignore the error.

Status changed to Accepted.

[gopherbot]

Comment 13 by david@scraperwiki.com:

a@ and i@ I Agree. Command should ignore errors from LookPath if there is a slash in the
path (possibly this could be done simply by not calling LookPath if there is a slash in
the path, since we "know" LookPath won't do anything useful in that case anyway).

[alexbrainman]

Comment 14:

Is this related to https://golang.org/issue/3622 ?
Alex

[ianlancetaylor]

Comment 15:

This is related to issue #3622, but it's not the same, as this is really about the use of
LookPath from Command.