OCI spec may contain duplicate environment variables

Closes #5226

PiperOrigin-RevId: 351259576

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)