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: expose a config that allows LookPath and Command to take in relative paths #53962

Closed
Mark-Jung opened this issue Jul 20, 2022 · 9 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@Mark-Jung
Copy link

With Go 1.19, os/exec no longer accepts relative paths as part of PATH. Reading through https://github.com/golang/go/issues/43724, the suggested workaround is to export the paths as absolute paths.

However, In some build environments (i.e. Bazel), paths to executables that are compiled during build time are passed around as relative paths for hermetic builds.

Would it be possible for Go runtime to expose some type of configuration (maybe via environment variable?) that allows relative path lookups in os/exec?

@gopherbot gopherbot added this to the Proposal milestone Jul 20, 2022
@rsc
Copy link
Contributor

rsc commented Jul 20, 2022

The package docs say:


Code that insists on including results from relative path entries can instead override the error using an errors.Is check:

path, err := exec.LookPath("prog")
if errors.Is(err, exec.ErrDot) {
	err = nil
}
if err != nil {
	log.Fatal(err)
}
use(path)

and

cmd := exec.Command("prog")
if errors.Is(cmd.Err, exec.ErrDot) {
	cmd.Err = nil
}
if err := cmd.Run(); err != nil {
	log.Fatal(err)
}

Before adding such overrides, make sure you understand the security implications of doing so. See https://go.dev/blog/path-security for more information.


Those build environments should be able to add those three lines (the if statement) at the appropriate call sites to produce the "config" that is desired in just the call sites where it is appropriate.

If that's not an option, or if some other override is needed, can you say more about why that's not enough?

@rsc
Copy link
Contributor

rsc commented Jul 20, 2022

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

@sywhang
Copy link
Contributor

sywhang commented Jul 20, 2022

We are currently facing a similar issue at Uber - the main issue is that sometimes the code that does these relative path lookups are not necessarily owned/maintained by us.

For example, there are code generators that expect some plugins that are essentially artifacts produced during build to be part of the PATH. Currently these are relative paths because Bazel Files only expose relative paths from the sandbox.

@rsc
Copy link
Contributor

rsc commented Jul 27, 2022

@sywhang thanks for the information. What does the PATH look like when running under Bazel Files? Is it all locations relative to the current directory? Does that mean you can't chdir anywhere else without breaking your PATH?

A global override is probably a mistake, because you'd want to limit the effect to specific parts of a program.

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/419794 mentions this issue: os/exec: add GODEBUG setting to opt out of ErrDot changes

@sywhang
Copy link
Contributor

sywhang commented Jul 27, 2022

@rsc our build infrastructure compiles some code generators which then gets used to compile other parts of the monorepo. These code generators are sometimes invoking other code generator "plugin"s which gets outputted to some arbitrary location in the Bazel sandbox. These locations reported by Bazel rules are relative to the Bazel sandbox. These tools get invoked via Bazel rule build step so we don't chdir.

FWIW, we unblocked ourselves with a hack to test out Go 1.19 - we injected bash scripts before running any of these code generators to find the absolute paths to these plugins and put them on PATH, but it would be still nice if there were workarounds for these controlled sandboxed build environments that can be assumed "safe".

We also discussed potentially asking Bazel to report absolute paths to files generated, but that would go against the whole philosophy of having hermetic builds in Bazel.

Agreed that global override is not necessary here -- we want to be able to disable it just for these specific code generators whose effect/lifetime is well-known in advance.

@linzhp
Copy link
Contributor

linzhp commented Jul 27, 2022

What does the PATH look like when running under Bazel Files? Is it all locations relative to the current directory?

To give an example, this is a command Bazel calls during a build

(cd /home/user/.cache/bazel/_bazel_zplin/b97476d719d716accead0f2d5b93104f/sandbox/processwrapper-sandbox/243/execroot/__main__ && \
  exec env - \
    CGO_ENABLED=1 \
    GOARCH=amd64 \
    GOOS=linux \
    GOPATH='' \
    GOROOT=external/go_sdk \
    GOROOT_FINAL=GOROOT \
    PATH=/usr/bin:/bin \
  bazel-out/k8-opt-exec-2B5CBBC6/bin/external/go_sdk/builder compilepkg -sdk external/go_sdk -installsuffix linux_amd64 -src external/io_opentelemetry_go_proto_otlp/collector/trace/v1/trace_config.pb.go -src ...)

You can see that Bazel first cd into the execroot, which is a sandbox location. Everything passed to the executable are relative paths in the sandbox, translated from Bazel labels that are passed from target definitions. If a code generator wants to call gofmt at the end of code generation, then we need to add gofmt to the PATH in Bazel, and the gofmt binary path managed by Bazel is a relative path.

This design is intentional so actions don't accidentally read or execute any files outside the sandbox. The absolute path of the sandbox changes from machine to machine, even from build to build on the same machine. By not assuming any absolute path, Bazel can schedule this action to a remote build machine and get back the build result. By managing gofmt with Bazel instead of assuming each machine have gofmt installed, we can guaratee that the code generated by any machine is exactly the same.

One solution is so change all programs that Bazel calls to allow passing a path to any other programs that they need to run. For example, all code generators that want to call gofmt will have to accept an additional argument, so they can run the given relative path to gofmt directly instead of assuming it PATH. This applies to all plugins and binaries that any code generator calls. Some of those code generators are out of our control.

The other is like @sywhang said, we can hack all Bazel rules and append $PWD to all codegen plugins.

@ianlancetaylor
Copy link
Contributor

The change in 1.19 only affects the os/exec package when there are relative paths in the PATH environment variable. So one way to make your code work should be to find the code that sets PATH, and change it to store absolute paths rather than relative paths.

Note that your example of a command line only has absolute paths on PATH. So there is something else that is adding the relative paths.

It's not obvious to me why adding $PWD to plugins matters one way or another. But I suppose it does if those plugins are using os.Args[0] to add an entry to PATH.

@rsc
Copy link
Contributor

rsc commented Jul 28, 2022

Upon further reflection, I think it is appropriate to provide a GODEBUG opt-out for this change. We do the same in the crypto/* packages when we retire insecure algorithms and protocol versions, so that people affected by the improved security have a way to opt out without modifying their code at all. This change feels a bit like those. Since the Go 1.19 release is imminent (next week), I've gone ahead and added the GODEBUG opt-out in CL 419794.

Clearly there must be a little more to your example. Since GOROOT is a relative path, I wonder if something is adding $GOROOT/bin to $PATH. Or perhaps the value of the -sdk flag (the same relative path) is being added. That would cause a look for 'go' or 'gofmt' to fail:

% cat lookpath.go
package main

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

func main() {
	fmt.Println(exec.LookPath(os.Args[1]))
}
% go1.18 build -o look18 lookpath.go
% go build -o look19 lookpath.go
% ls -l go/bin/gofmt
-rwxr-xr-x  1 rsc  primarygroup  3350672 Jul 25 16:20 go/bin/gofmt
% PATH=go/bin:$PATH ./look18 gofmt
go/bin/gofmt <nil>
% PATH=go/bin:$PATH ./look19 gofmt
go/bin/gofmt exec: "gofmt": cannot run executable found relative to current directory
% 

The new GODEBUG restores the old behavior:

% GODEBUG=execerrdot=0 PATH=go/bin:$PATH ./look19 gofmt
go/bin/gofmt <nil>
% 

It would also suffice to invoke the binary directly by using filepath.Join(GOROOT, "bin/go"):

% ./look18 go/bin/gofmt
go/bin/gofmt <nil>
% ./look19 go/bin/gofmt
go/bin/gofmt <nil>
% 

@dmitshur dmitshur modified the milestones: Proposal, Go1.19 Jul 28, 2022
@dmitshur dmitshur changed the title proposal: os/exec: Expose a config that allows LookPath and Command to take in relative paths os/exec: expose a config that allows LookPath and Command to take in relative paths Jul 31, 2022
@dmitshur dmitshur added NeedsFix The path to resolution is known, but the work has not been done. and removed Proposal labels Jul 31, 2022
jproberts pushed a commit to jproberts/go that referenced this issue Aug 10, 2022
The changes are likely to break users, and we need
to make it easy to unbreak without code changes.

For golang#43724.
Fixes golang#53962.

Change-Id: I105c5d6c801d354467e0cefd268189c18846858e
Reviewed-on: https://go-review.googlesource.com/c/go/+/419794
Reviewed-by: Bryan Mills <bcmills@google.com>
Reviewed-by: Ian Lance Taylor <iant@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Russ Cox <rsc@golang.org>
problame added a commit to zrepl/zrepl that referenced this issue Feb 26, 2023
…Go 1.19

See the comment in the script.

refs golang/go#53962

 used by make test-platform breaks the test on Go 1.19
@golang golang locked and limited conversation to collaborators Jul 31, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

7 participants