From 7e462a1c7f56b9b8439ad1ac92906bd8dd376ab7 Mon Sep 17 00:00:00 2001 From: Fabricio Voznika Date: Mon, 11 Jan 2021 16:23:44 -0800 Subject: [PATCH] OCI spec may contain duplicate environment variables Closes #5226 PiperOrigin-RevId: 351259576 --- runsc/boot/loader.go | 11 +++- runsc/cmd/exec.go | 29 +------- runsc/container/multi_container_test.go | 88 +++++++++++++++++++++++++ runsc/specutils/specutils.go | 25 +++++++ 4 files changed, 125 insertions(+), 28 deletions(-) diff --git a/runsc/boot/loader.go b/runsc/boot/loader.go index f41d6c665..d7afd3dc1 100644 --- a/runsc/boot/loader.go +++ b/runsc/boot/loader.go @@ -440,6 +440,10 @@ func createProcessArgs(id string, spec *specs.Spec, creds *auth.Credentials, k * if err != nil { return kernel.CreateProcessArgs{}, fmt.Errorf("creating limits: %v", err) } + env, err := specutils.ResolveEnvs(spec.Process.Env) + if err != nil { + return kernel.CreateProcessArgs{}, fmt.Errorf("resolving env: %w", err) + } wd := spec.Process.Cwd if wd == "" { @@ -449,7 +453,7 @@ func createProcessArgs(id string, spec *specs.Spec, creds *auth.Credentials, k * // Create the process arguments. procArgs := kernel.CreateProcessArgs{ Argv: spec.Process.Args, - Envv: spec.Process.Env, + Envv: env, WorkingDirectory: wd, Credentials: creds, Umask: 0022, @@ -933,6 +937,11 @@ func (l *Loader) executeAsync(args *control.ExecArgs) (kernel.ThreadID, error) { } } + args.Envv, err = specutils.ResolveEnvs(args.Envv) + if err != nil { + return 0, fmt.Errorf("resolving env: %w", err) + } + // Add the HOME environment variable if it is not already set. if kernel.VFS2Enabled { root := args.MountNamespaceVFS2.Root() diff --git a/runsc/cmd/exec.go b/runsc/cmd/exec.go index 8558d34ae..e9726401a 100644 --- a/runsc/cmd/exec.go +++ b/runsc/cmd/exec.go @@ -118,14 +118,14 @@ func (ex *Exec) Execute(_ context.Context, f *flag.FlagSet, args ...interface{}) } log.Debugf("Exec arguments: %+v", e) - log.Debugf("Exec capablities: %+v", e.Capabilities) + log.Debugf("Exec capabilities: %+v", e.Capabilities) // Replace empty settings with defaults from container. if e.WorkingDirectory == "" { e.WorkingDirectory = c.Spec.Process.Cwd } if e.Envv == nil { - e.Envv, err = resolveEnvs(c.Spec.Process.Env, ex.env) + e.Envv, err = specutils.ResolveEnvs(c.Spec.Process.Env, ex.env) if err != nil { Fatalf("getting environment variables: %v", err) } @@ -382,31 +382,6 @@ func argsFromProcess(p *specs.Process, enableRaw bool) (*control.ExecArgs, error }, nil } -// resolveEnvs transforms lists of environment variables into a single list of -// environment variables. If a variable is defined multiple times, the last -// value is used. -func resolveEnvs(envs ...[]string) ([]string, error) { - // First create a map of variable names to values. This removes any - // duplicates. - envMap := make(map[string]string) - for _, env := range envs { - for _, str := range env { - parts := strings.SplitN(str, "=", 2) - if len(parts) != 2 { - return nil, fmt.Errorf("invalid variable: %s", str) - } - envMap[parts[0]] = parts[1] - } - } - // Reassemble envMap into a list of environment variables of the form - // NAME=VALUE. - env := make([]string, 0, len(envMap)) - for k, v := range envMap { - env = append(env, fmt.Sprintf("%s=%s", k, v)) - } - return env, nil -} - // capabilities takes a list of capabilities as strings and returns an // auth.TaskCapabilities struct with those capabilities in every capability set. // This mimics runc's behavior. diff --git a/runsc/container/multi_container_test.go b/runsc/container/multi_container_test.go index 29db1b7e8..044eec6fe 100644 --- a/runsc/container/multi_container_test.go +++ b/runsc/container/multi_container_test.go @@ -1803,3 +1803,91 @@ func TestMultiContainerEvent(t *testing.T) { } } } + +// Tests that duplicate variables in the spec are merged into a single one. +func TestDuplicateEnvVariable(t *testing.T) { + conf := testutil.TestConfig(t) + + rootDir, cleanup, err := testutil.SetupRootDir() + if err != nil { + t.Fatalf("error creating root dir: %v", err) + } + defer cleanup() + conf.RootDir = rootDir + + // Create files to dump `env` output. + files := [3]*os.File{} + for i := 0; i < len(files); i++ { + var err error + files[i], err = ioutil.TempFile(testutil.TmpDir(), "env-var-test") + if err != nil { + t.Fatalf("creating temp file: %v", err) + } + defer files[i].Close() + defer os.Remove(files[i].Name()) + } + + // Setup the containers. Use root container to test exec too. + cmd1 := fmt.Sprintf("env > %q; sleep 1000", files[0].Name()) + cmd2 := fmt.Sprintf("env > %q", files[1].Name()) + cmdExec := fmt.Sprintf("env > %q", files[2].Name()) + testSpecs, ids := createSpecs([]string{"/bin/bash", "-c", cmd1}, []string{"/bin/bash", "-c", cmd2}) + testSpecs[0].Process.Env = append(testSpecs[0].Process.Env, "VAR=foo", "VAR=bar") + testSpecs[1].Process.Env = append(testSpecs[1].Process.Env, "VAR=foo", "VAR=bar") + + containers, cleanup, err := startContainers(conf, testSpecs, ids) + if err != nil { + t.Fatalf("error starting containers: %v", err) + } + defer cleanup() + + // Wait for the `env` from the root container to finish. + expectedPL := []*control.Process{ + newProcessBuilder().Cmd("bash").Process(), + newProcessBuilder().Cmd("sleep").Process(), + } + if err := waitForProcessList(containers[0], expectedPL); err != nil { + t.Errorf("failed to wait for sleep to start: %v", err) + } + if ws, err := containers[1].Wait(); err != nil { + t.Errorf("failed to wait container 1: %v", err) + } else if es := ws.ExitStatus(); es != 0 { + t.Errorf("container %s exited with non-zero status: %v", containers[1].ID, es) + } + + execArgs := &control.ExecArgs{ + Filename: "/bin/bash", + Argv: []string{"/bin/bash", "-c", cmdExec}, + Envv: []string{"VAR=foo", "VAR=bar"}, + } + if ws, err := containers[0].executeSync(execArgs); err != nil || ws.ExitStatus() != 0 { + t.Fatalf("exec failed, ws: %v, err: %v", ws, err) + } + + // Now read and check that none of the env has repeated values. + for _, file := range files { + out, err := ioutil.ReadAll(file) + if err != nil { + t.Fatal(err) + } + t.Logf("Checking env %q:\n%s", file.Name(), out) + envs := make(map[string]string) + for _, line := range strings.Split(string(out), "\n") { + if len(line) == 0 { + continue + } + envVar := strings.SplitN(line, "=", 2) + if len(envVar) != 2 { + t.Fatalf("invalid env variable: %s", line) + } + key := envVar[0] + if val, ok := envs[key]; ok { + t.Errorf("env variable %q is duplicated: %q and %q", key, val, envVar[1]) + } + envs[key] = envVar[1] + } + if _, ok := envs["VAR"]; !ok { + t.Errorf("variable VAR missing: %v", envs) + } + } +} diff --git a/runsc/specutils/specutils.go b/runsc/specutils/specutils.go index fdbba1832..ea55bbc7d 100644 --- a/runsc/specutils/specutils.go +++ b/runsc/specutils/specutils.go @@ -493,6 +493,31 @@ func EnvVar(env []string, name string) (string, bool) { return "", false } +// ResolveEnvs transforms lists of environment variables into a single list of +// environment variables. If a variable is defined multiple times, the last +// value is used. +func ResolveEnvs(envs ...[]string) ([]string, error) { + // First create a map of variable names to values. This removes any + // duplicates. + envMap := make(map[string]string) + for _, env := range envs { + for _, str := range env { + parts := strings.SplitN(str, "=", 2) + if len(parts) != 2 { + return nil, fmt.Errorf("invalid variable: %s", str) + } + envMap[parts[0]] = parts[1] + } + } + // Reassemble envMap into a list of environment variables of the form + // NAME=VALUE. + env := make([]string, 0, len(envMap)) + for k, v := range envMap { + env = append(env, fmt.Sprintf("%s=%s", k, v)) + } + return env, nil +} + // FaqErrorMsg returns an error message pointing to the FAQ. func FaqErrorMsg(anchor, msg string) string { return fmt.Sprintf("%s; see https://gvisor.dev/faq#%s for more details", msg, anchor)