Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
26 changes: 20 additions & 6 deletions internal/jobcontainers/jobcontainer.go
Original file line number Diff line number Diff line change
Expand Up @@ -174,14 +174,22 @@ func (c *JobContainer) CreateProcess(ctx context.Context, config interface{}) (_
return nil, errors.New("console emulation not supported for job containers")
}

absPath, commandLine, err := getApplicationName(conf.CommandLine, c.sandboxMount, os.Getenv("PATH"))
// Replace any occurences of the sandbox mount point env variable in the commandline.
// %CONTAINER_SANDBOX_MOUNTPOINT%\mybinary.exe -> C:\C\123456789\mybinary.exe
commandLine := c.replaceWithMountPoint(conf.CommandLine)

workDir := c.sandboxMount
if conf.WorkingDirectory != "" {
workDir = c.replaceWithMountPoint(conf.WorkingDirectory)
}

// Reassign commandline here in case it needed to be quoted. For example if "foo bar baz" was supplied, and
// "foo bar.exe" exists, then return: "\"foo bar\" baz"
absPath, commandLine, err := getApplicationName(commandLine, workDir, os.Getenv("PATH"))
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It's hard to follow what is happening with the commandLine here. Why is assigned above and then re-assigned here? Are they expected to be difference values?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

So the first thing we do is replace any instances of the env variable if there are any in the commandline.

Then on line 184 we reassign commandLine as it may have changed depending on if it needed to be quoted or not.

For instance, if someone passed foo bar baz, and foo bar.exe exists and baz was simply an argument, we need to quote the commandline to "foo bar" baz

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I'll comment the heck out of this, and re-push. It is a bit hard to follow

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think it is outside the scope of this PR but the getApplicationName isn't clear as to what it does and that caused some confusion when reading. Comments help though it be better long term to refactor this a bit for clarity. Maybe its harder than it looks... just an observation

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It's a pretty close interpretation of the c++ logic for windows server containers, so this likely played a role in its complexity :P. I'll keep a note of this.

if err != nil {
return nil, errors.Wrapf(err, "failed to get application name from commandline %q", conf.CommandLine)
}

commandLine = strings.ReplaceAll(commandLine, "%"+sandboxMountPointEnvVar+"%", c.sandboxMount)
commandLine = strings.ReplaceAll(commandLine, "$env:"+sandboxMountPointEnvVar, c.sandboxMount)

var token windows.Token
if getUserTokenInheritAnnotation(c.spec.Annotations) {
token, err = openCurrentProcessToken()
Expand All @@ -205,12 +213,12 @@ func (c *JobContainer) CreateProcess(ctx context.Context, config interface{}) (_

cmd := &exec.Cmd{
Env: env,
Dir: c.sandboxMount,
Dir: workDir,
Path: absPath,
Args: splitArgs(commandLine),
SysProcAttr: &syscall.SysProcAttr{
// CREATE_BREAKAWAY_FROM_JOB to make sure that we're not inheriting the job object (and by extension its limits)
// from whatever process is running this code.
// from whatever process is going to launch the container.
CreationFlags: windows.CREATE_NEW_PROCESS_GROUP | windows.CREATE_BREAKAWAY_FROM_JOB,
Token: syscall.Token(token),
},
Expand Down Expand Up @@ -566,3 +574,9 @@ func systemProcessInformation() ([]*winapi.SYSTEM_PROCESS_INFORMATION, error) {

return procInfos, nil
}

// Takes a string and replaces any occurences of CONTAINER_SANDBOX_MOUNT_POINT with where the containers volume is mounted.
func (c *JobContainer) replaceWithMountPoint(str string) string {
str = strings.ReplaceAll(str, "%"+sandboxMountPointEnvVar+"%", c.sandboxMount)
return strings.ReplaceAll(str, "$env:"+sandboxMountPointEnvVar, c.sandboxMount)
}
5 changes: 5 additions & 0 deletions internal/jobcontainers/path.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package jobcontainers
import (
"fmt"
"os"
"path/filepath"
"strings"

"github.com/Microsoft/hcsshim/internal/winapi"
Expand Down Expand Up @@ -71,6 +72,10 @@ func getApplicationName(commandLine, workingDirectory, pathEnv string) (string,
searchPath string
result string
)

// Clean the path, to get rid of any . elements
commandLine = filepath.Clean(commandLine)

// First we get the system paths concatenated with semicolons (C:\windows;C:\windows\system32;C:\windows\system;)
// and use this as the basis for the directories to search for the application.
systemPaths, err := getSystemPaths()
Expand Down
33 changes: 31 additions & 2 deletions internal/jobcontainers/path_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,16 @@ package jobcontainers
import (
"os"
"os/exec"
"strings"
"testing"
)

func assertStr(t *testing.T, a string, b string) {
if !strings.EqualFold(a, b) {
t.Fatalf("expected %s, got %s", a, b)
}
}

func TestSearchPath(t *testing.T) {
// Testing that relative paths work.
_, err := searchPathForExe("windows\\system32\\ping", "C:\\")
Expand All @@ -17,30 +24,52 @@ func TestSearchPath(t *testing.T) {
if err != nil {
t.Fatal(err)
}

_, err = searchPathForExe("ping", "C:\\windows\\system32")
if err != nil {
t.Fatal(err)
}
}

func TestGetApplicationName(t *testing.T) {
expected := "C:\\WINDOWS\\system32\\ping.exe"

cwd, err := os.Getwd()
if err != nil {
t.Fatal(err)
}

_, _, err = getApplicationName("ping", cwd, os.Getenv("PATH"))
path, _, err := getApplicationName("ping", cwd, os.Getenv("PATH"))
if err != nil {
t.Fatal(err)
}
assertStr(t, expected, path)

path, _, err = getApplicationName("./ping", cwd, os.Getenv("PATH"))
if err != nil {
t.Fatal(err)
}
assertStr(t, expected, path)

path, _, err = getApplicationName(".\\ping", cwd, os.Getenv("PATH"))
if err != nil {
t.Fatal(err)
}
assertStr(t, expected, path)

// Test that we only find the first element of the commandline if the binary exists.
_, _, err = getApplicationName("ping test", cwd, os.Getenv("PATH"))
path, _, err = getApplicationName("ping test", cwd, os.Getenv("PATH"))
if err != nil {
t.Fatal(err)
}
assertStr(t, expected, path)

// Test quoted application name with an argument afterwards.
path, cmdLine, err := getApplicationName("\"ping\" 127.0.0.1", cwd, os.Getenv("PATH"))
if err != nil {
t.Fatal(err)
}
assertStr(t, expected, path)

args := splitArgs(cmdLine)
cmd := &exec.Cmd{
Expand Down