Skip to content

Set PATHEXT for job containers to handle binaries with no extension #1174

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.

Sign up for GitHub

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

Merged
dcantah merged 1 commit into from
Sep 24, 2021
Merged

Set PATHEXT for job containers to handle binaries with no extension #1174

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions internal/jobcontainers/jobcontainer.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -211,6 +211,16 @@ func (c *JobContainer) CreateProcess(ctx context.Context, config interface{}) (_
env = append(env, envMapToSlice(conf.Environment)...)
env = append(env, sandboxMountPointEnvVar+"="+c.sandboxMount)

// exec.Cmd internally does its own path resolution and as part of this checks some well known file extensions on the file given (e.g. if
// the user just provided /path/to/mybinary). CreateProcess is perfectly capable of launching an executable that doesn't have the .exe extension
// so this adds an empty string entry to the end of what extensions GO checks against so that a binary with no extension can be launched.
// The extensions are checked in order, so that if mybinary.exe and mybinary both existed in the same directory, mybinary.exe would be chosen.
// This is mostly to handle a common Kubernetes test image named agnhost that has the main entrypoint as a binary named agnhost with no extension.
// https://github.com/kubernetes/kubernetes/blob/d64e91878517b1208a0bce7e2b7944645ace8ede/test/images/agnhost/Dockerfile_windows
if err := os.Setenv("PATHEXT", ".COM;.EXE;.BAT;.CMD; "); err != nil {
return nil, errors.Wrap(err, "failed to set PATHEXT")
}

Comment on lines +220 to +223
Copy link
Copy Markdown

@katiewasnothere katiewasnothere Sep 24, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we add this only to the environment that's passed to the cmd instead of setting it on the running environment?

Copy link
Copy Markdown
Contributor Author

@dcantah dcantah Sep 24, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's needed to find the process to launch in the first place so nope. Basically exec.Cmd calls exec.LookPath as one of the steps before launching the process and this is what fails as it doesn't treat a file with an extension not in its default set of extensions (and no extension falls in this category) as valid.

Copy link
Copy Markdown
Contributor Author

@dcantah dcantah Sep 24, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So we're adding an empty string entry so it deems this as valid now.

cmd := &exec.Cmd{
Env: env,
Dir: workDir,
Expand Down