os/exec: look in current directory for executable on all platforms?

[alexbrainman]

This test

# cat main_test.go
package main_test

import (
        "io/ioutil"
        "os"
        "os/exec"
        "path/filepath"
        "testing"
)

func testALEX(t *testing.T, exe string) {
        tmp, err := ioutil.TempDir("", "TestALEX")
        if err != nil {
                t.Fatal(err)
        }
        defer os.RemoveAll(tmp)

        newdir := filepath.Join(tmp, "d")
        err = os.Mkdir(newdir, 0777)
        if err != nil {
                t.Fatal(err)
        }
        err = os.Symlink("/bin/echo", filepath.Join(newdir, "c"))
        if err != nil {
                t.Fatal(err)
        }

        cmd := exec.Command(exe, "-n", "foo")
        cmd.Dir = newdir
        out, err := cmd.CombinedOutput()
        if err != nil {
                t.Error(err)
                return
        }
        want := "foo"
        if got := string(out); got != want {
                t.Errorf("exec: want %q, got %q", want, got)
        }
}

func TestALEX(t *testing.T) {
        testALEX(t, "./c")
        testALEX(t, "c")
}
#

should succeed, but fails:

# PATH=.:$PATH go test -v
=== RUN TestALEX
--- FAIL: TestALEX (0.00 seconds)
        main_test.go:32: exec: "c": executable file not found in $PATH
FAIL
exit status 1
FAIL    t       0.003s
# hg -R $GOROOT id
17d15bc23bce+ tip
#

I am not fussed, if we decide either way. But I would like to know how to handle this
scenario, because that is what windows does (windows always look in the . before
searching the PATH). This is just another side of issue #7377.

Alex

[bradfitz]

Comment 1:

Having the current directory in $PATH is a security problem. I see no desire to mimic
that behavior in Go for portability reasons. Doing it because it's the default on
Windows makes enough sense, but I'd rather not do that elsewhere.
I vote WorkingAsIntended. We could even add a test to ensure that's always the case, if
you're feeling like writing code for this bug.

[ianlancetaylor]

Comment 2:

I agree with Brad.

Status changed to WorkingAsIntended.

[alexbrainman]

Comment 3:

But I will implement this behaviour on windows. Agreed? I will try and write all
appropriate tests and document this platform difference.
Alex

[alexbrainman]

Comment 4:

Now thinking about this again. If you mark this as WorkingAsIntended because you "see no
desire to mimic" "the current directory in $PATH is a security problem", then here is a
simple variation on my original test where you do "mimic"  "the current directory in
$PATH is a security problem".
package main_test
import (
        "io/ioutil"
        "os"
        "os/exec"
        "os"
        "os/exec"
        "path/filepath"
        "testing"
)
func testALEX(t *testing.T, exe string) {
        tmp, err := ioutil.TempDir("", "TestALEX")
        if err != nil {
                t.Fatal(err)
        }
        defer os.RemoveAll(tmp)
        err = os.Symlink("/bin/echo", filepath.Join(tmp, "c"))
        if err != nil {
                t.Fatal(err)
        }
        wd, err := os.Getwd()
        if err != nil {
                t.Fatal(err)
        }
        err = os.Chdir(tmp)
        if err != nil {
                t.Fatal(err)
        }
        defer os.Chdir(wd)
        cmd := exec.Command(exe, "-n", "foo")
        out, err := cmd.CombinedOutput()
        if err != nil {
                t.Error(err)
                return
        }
        want := "foo"
        if got := string(out); got != want {
                t.Errorf("exec: want %q, got %q", want, got)
        }
}
func TestALEX(t *testing.T) {
        testALEX(t, "./c")
        testALEX(t, "c")
}
The test PASSes now:
# PATH=.:$PATH go test -v
=== RUN TestALEX
--- PASS: TestALEX (0.00 seconds)
PASS
ok      t       0.004s
I think we have a problem here. I think our lib is inconsistent in how it behaves with
and without Command.Dir parameter. I think we should make this consistent or document
why they are different regardless of what we will decide about "the current directory in
$PATH is a security problem".
Alex

[ianlancetaylor]

Comment 5:

Your most recent program doesn't pass on GNU/Linux:
--- FAIL: TestALEX (0.00 seconds)
    foo_test.go:36: exec: "c": executable file not found in $PATH
FAIL
FAIL    command-line-arguments  0.004s
We may well have a problem on Windows.  If Windows always implicitly puts . on PATH, I'm
OK with doing whatever that implies in the os/exec package.

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

Status changed to Accepted.

[alexbrainman]

Comment 6:

Ian,
re: most recent test failing on your GNU/Linux:
Did you change your PATH during the test, like so:
PATH=.:$PATH go test -v
?
Alex

[ianlancetaylor]

Comment 7:

Sorry, I missed that bit.  Yes, the test passes if . is on PATH.
I guess I don't see why that is a problem.  It seems to me that that is correct
behaviour: if . is on PATH, and the job is executed in a directory that contains a file
with the name of the executable, then I would expect it to run the file in that
directory.  And that seems to be what happens.
In your first example, the job is executed in a different directory, but I would not
expect that directory to be used to find the executable.  And that is also what happens.
So, yes, the behaviour differs, but it still seems right to me.

[alexbrainman]

Comment 8:

My confusion here is about interpreting the ".". It seems to me, it means different
things in different context.
cmd := Command("./prog")
cmd.Dir = "/tmp/d"
"." in ./prog means /tmp/d
while "." in the PATH means "my current directory".
I can live with that. But our current code
http://tip.golang.org/src/pkg/os/exec/exec.go#L115 is not helping with my confusion.
Perhaps it needs to be documented better too.
I will see if I can implement all that on windows.
Thank you for your input.
Alex

[alexbrainman]

Comment 9:

I am struggling with another issue I discovered here. There is a small difference in the
way exec.Command and cmd.Cmd{Path: ...} behave. This test will demonstrate:
diff --git a/src/pkg/os/exec/exec_test.go b/src/pkg/os/exec/exec_test.go
--- a/src/pkg/os/exec/exec_test.go
+++ b/src/pkg/os/exec/exec_test.go
@@ -687,3 +687,51 @@
        os.Exit(2)
    }
 }
+
+func runCommand(t *testing.T, name string) string {
+   cmd := exec.Command(name)
+   out, err := cmd.CombinedOutput()
+   if err != nil {
+       t.Fatal(err)
+   }
+   return string(out)
+}
+
+func runNewCmd(t *testing.T, name string) string {
+   cmd := exec.Cmd{Path: name}
+   out, err := cmd.CombinedOutput()
+   if err != nil {
+       t.Fatal(err)
+   }
+   return string(out)
+}
+
+func TestALEX(t *testing.T) {
+   tmp, err := ioutil.TempDir("", "TestALEX")
+   if err != nil {
+       t.Fatal(err)
+   }
+   defer os.RemoveAll(tmp)
+
+   err = os.Symlink("/bin/echo", filepath.Join(tmp, "date"))
+   if err != nil {
+       t.Fatal(err)
+   }
+
+   wd, err := os.Getwd()
+   if err != nil {
+       t.Fatal(err)
+   }
+
+   err = os.Chdir(tmp)
+   if err != nil {
+       t.Fatal(err)
+   }
+   defer os.Chdir(wd)
+
+   outCommand := runCommand(t, "date")
+   outNewCmd := runNewCmd(t, "date")
+   if outCommand != outNewCmd {
+       t.Fatalf("outCommand(%q) and outNewCmd(%q) are different", outCommand, outNewCmd)
+   }
+}
The test outputs:
# go test -run=ALEX
--- FAIL: TestALEX (0.00 seconds)
        exec_test.go:735: outCommand("Fri Mar 28 12:09:19 EST 2014\n") and outNewCmd("\n") are different
FAIL
exit status 1
FAIL    os/exec 0.005s
#
Is that something you consider broken? If so, how should it be fixed? Thank you.
Alex

[ianlancetaylor]

Comment 10:

The exec.Command function is documented as looking up a name in the PATH.  The cmd.Path
field is documented as evaluating the name relative to the current directory.  So both
appear to be working as documented.
You seem to be uncomfortable with the current documented behaviour.  Do you want to
propose a different behaviour?

[alexbrainman]

Comment 11:

> ... You seem to be uncomfortable with the current documented behaviour.  Do you want
to propose a different behaviour?
I am not good at expressing myself, so I will defer to your judgement re documentation.
I'm rearranging os/exec to fix https://golang.org/issue/7377, and
I'm worried about breaking things. So I value your input here. Thank you.
I also asked that question because in
https://golang.org/issue/7570?c=1 Brad said "Having the current
directory in $PATH is a security problem. I see no desire to mimic that behavior in Go
...", but exec.Cmd{Path: "date"} actually looks inside your current directory (PATH or
not PATH). So, I though, perhaps there is some contradiction here. But I will take it as
is.
Alex

[alexbrainman]

Comment 12:

Sorry, but I need more help. This test
diff --git a/src/pkg/os/exec/exec_test.go b/src/pkg/os/exec/exec_test.go
--- a/src/pkg/os/exec/exec_test.go
+++ b/src/pkg/os/exec/exec_test.go
@@ -687,3 +687,41 @@
        os.Exit(2)
    }
 }
+
+func TestALEX(t *testing.T) {
+   tmp, err := ioutil.TempDir("", "TestALEX")
+   if err != nil {
+       t.Fatal(err)
+   }
+   defer os.RemoveAll(tmp)
+
+   err = os.Symlink("/bin/echo", filepath.Join(tmp, "ccc"))
+   if err != nil {
+       t.Fatal(err)
+   }
+
+   wd, err := os.Getwd()
+   if err != nil {
+       t.Fatal(err)
+   }
+
+   err = os.Chdir(tmp)
+   if err != nil {
+       t.Fatal(err)
+   }
+   defer os.Chdir(wd)
+
+   os.Setenv("PATH", os.Getenv("PATH")+":.")
+
+   cmd := exec.Command("ccc", "-n", "foo")
+   cmd.Dir = "/bin" // remove this line and test PASSes
+   out, err := cmd.CombinedOutput()
+   if err != nil {
+       t.Error(err)
+       return
+   }
+   want := "foo"
+   if got := string(out); got != want {
+       t.Errorf("exec: want %q, got %q", want, got)
+   }
+}
fails with
# go test -v -run=ALEX
=== RUN TestALEX
--- FAIL: TestALEX (0.00 seconds)
        exec_test.go:720: fork/exec ./ccc: no such file or directory
FAIL
exit status 1
FAIL    os/exec 0.004s
#
but succeeds, if I remove cmd.Dir = ... line. Why does it fail?
Thank you.
Alex

[ianlancetaylor]

Comment 13:

What is happening here is that exec.Command looks up "ccc" in PATH.  It finds it in the
directory ".", and it prepares to execute the file "./ccc".  When you set the Dir field,
the child process changes directory and then attempts to execute the file "./ccc". 
Since that file does not exist, the execution fails.
This is all still documented behaviour.  exec.Command says "If name contains no path
separators, Command uses LookPath to resolve the path to a complete name if possible." 
LookPath says "The result may be an absolute path or a path relative to the current
directory."  So exec.Command can in some cases, like this one, turn a name with no path
separators into a path relative to the current directory.  The documentation for the
Path field of Command says "If Path is relative, it is evaluated relative to Dir."  So
the relative path produced by exec.Command is evaluated relative to the Dir you set, and
fails.
The os/exec package is designed to make certain common operations convenient, and it
evidently does so at the cost of some API complexity.  By having exec.Command call
LookPath for you, we are walking into this lack of clarity.  But I don't see anything
clearly wrong here.  You are describing a complex scenario, but the package does appear
to be behaving as documented, and the package does provide ways to avoid the
complexities.
I understand that you are trying to figure out when the os/exec package should add a
.exe suffix.  At present LookPath adds .exe (and other suffixes) when looking for an
executable file.  Command is documented to call LookPath in certain cases and that
should not change.  As far as I can tell the os and syscall packages never add a .exe
suffix.
So I think the only question here is: what should Command do on Windows when given a
name that does contain path separators?  In that case, should it add .exe or not?  In Go
1.2 it did.  On tip it does not.  I think it would be reasonable to say that, on
Windows, when Command sees a name that does contain path separators, that it will try
adding .exe (and other suffixes), but will not make any other change to the name.

[alexbrainman]

Comment 14:

> ... I think it would be reasonable to say that, on Windows, when Command sees a name
that does contain path separators, that it will try adding .exe (and other suffixes),
but will not make any other change to the name.
I agree. The only problem is we need Cmd.Dir field to do the right thing here, and
Command does not have Cmd.Dir yet. I've implemented your suggested change, but in
Cmd.Start (Cmd.Dir should be set by then). Here is the change
https://golang.org/cl/83020043/.
Thank you for your help again.
Alex

[bradfitz]

Comment 15:

*** Submitted as https://code.google.com/p/go/source/detail?r=0bf0c11aea78 ***

Status changed to Fixed.