Repository: opencontainers/runc
Pull request #4704 (2025-04-01 15:43:48)

Override HOME if its set to the empty string
I'm testing runc 1.3 in docker CI and I found these issues.

I've also added some e2e tests and verified that runc 1.2 behaves the same way.

Below the commit msg of the fix, but see each commit msg for more details

---

Before commit 06f1e0765 ("libct: speedup process.Env handling") we were
overriding HOME if it was set to "" too[1]. But now we only override it
if it wasn't set at all.

This patch restores the old behavior of overriding it if it was set to
an empty value.
    
Docker relies on this behaviour since ages[2].
    
[1]: https://github.com/opencontainers/runc/blob/1c508045727231e7342e258ab30add1478c1f981/libcontainer/init_linux.go#L544-L549
[2]: https://github.com/moby/moby/blob/843e51459f14ebc964d349eba1013dc8a3e9d52e/integration-cli/docker_cli_run_test.go#L822-L843


rata: @kolyshkin I thought about adding tests there. The unit tests are nice, but in a big refactor they might not exist anymore. In fact, `prepareEnv()` didn't exist before the last refactor, it might not exist after the next one.

I really think we need e2e tests for this. I want us to be able to refactor and change heavily the way we set env vars, and have tests that catch any buggy behavior. This is what the e2e test I added try to do.
rata: This indeed fixes docker CI and it's ready for review now :)
kolyshkin: > @kolyshkin I thought about adding tests there. The unit tests are nice, but in a big refactor they might not exist anymore. In fact, `prepareEnv()` didn't exist before the last refactor, it might not exist after the next one.

In that case, the tests should migrate to a different place.

But if you modify a function which already have a unit test, it's best to amend the unit test as well.

Feel free to add bats tests, too; I understand their benefit. Just don't abandon the unit test.
rata: @kolyshkin Fixed all the comments and added unit tests. PTAL. I left unresolved the only comment that I think the discussion is interesting and you might want to see in more detail.

Patch:
diff --git a/libcontainer/env.go b/libcontainer/env.go
index fe1abd50..71b2f69d 100644
--- a/libcontainer/env.go
+++ b/libcontainer/env.go
@@ -17,13 +17,16 @@ import (
 //     contains no \0 (nil) bytes;
 //   - removes any duplicates (keeping only the last value for each key)
 //   - sets PATH for the current process, if found in the list;
-//   - adds HOME to returned environment, if not found in the list.
+//   - adds HOME to returned environment, if not found in the list,
+//     or the value is empty.
 //
 // Returns the prepared environment.
 func prepareEnv(env []string, uid int) ([]string, error) {
 	if env == nil {
 		return nil, nil
 	}
+	var homeIsSet bool
+
 	// Deduplication code based on dedupEnv from Go 1.22 os/exec.
 
 	// Construct the output in reverse order, to preserve the
@@ -40,6 +43,7 @@ func prepareEnv(env []string, uid int) ([]string, error) {
 			return nil, errors.New("invalid environment variable: name cannot be empty")
 		}
 		key := kv[:i]
+		val := kv[i+1:]
 		if saw[key] { // Duplicate.
 			continue
 		}
@@ -49,17 +53,25 @@ func prepareEnv(env []string, uid int) ([]string, error) {
 		}
 		if key == "PATH" {
 			// Needs to be set as it is used for binary lookup.
-			if err := os.Setenv("PATH", kv[i+1:]); err != nil {
+			if err := os.Setenv("PATH", val); err != nil {
 				return nil, err
 			}
 		}
+		if key == "HOME" {
+			if val != "" {
+				homeIsSet = true
+			} else {
+				// Don't add empty HOME to the environment, we will override it later.
+				continue
+			}
+		}
 		out = append(out, kv)
 	}
 	// Restore the original order.
 	slices.Reverse(out)
 
 	// If HOME is not found in env, get it from container's /etc/passwd and add.
-	if !saw["HOME"] {
+	if !homeIsSet {
 		home, err := getUserHome(uid)
 		if err != nil {
 			// For backward compatibility, don't return an error, but merely log it.


Test patch:
diff --git a/libcontainer/env_test.go b/libcontainer/env_test.go
index c9174716..18dc52e8 100644
--- a/libcontainer/env_test.go
+++ b/libcontainer/env_test.go
@@ -37,6 +37,18 @@ func TestPrepareEnv(t *testing.T) {
 			env:     []string{"TERM=vt100", "HOME=/home/one", "HOME=/home/two", "TERM=xterm", "HOME=/home/three", "FOO=bar"},
 			wantEnv: []string{"TERM=xterm", "HOME=/home/three", "FOO=bar"},
 		},
+		{
+			env:     []string{"HOME=", "HOME=/foo"},
+			wantEnv: []string{"HOME=/foo"},
+		},
+		{
+			env:     []string{"HOME="},
+			wantEnv: []string{home},
+		},
+		{
+			env:     []string{"HOME=/foo", "HOME="},
+			wantEnv: []string{home},
+		},
 	}
 
 	for _, tc := range tests {
diff --git a/tests/integration/env.bats b/tests/integration/env.bats
new file mode 100644
index 00000000..cb984942
--- /dev/null
+++ b/tests/integration/env.bats
@@ -0,0 +1,97 @@
+#!/usr/bin/env bats
+# shellcheck disable=SC2016
+# This disables the check for shell variables inside single quotes
+# We do that all the time in this file, as we are testing env vars.
+
+load helpers
+
+function setup() {
+	setup_busybox
+}
+
+function teardown() {
+	teardown_bundle
+}
+
+# Several of these tests are inspired on regressions caught by Docker, besides other tests that
+# check the behavior we already had in runc:
+# https://github.com/moby/moby/blob/843e51459f14ebc964d349eba1013dc8a3e9d52e/integration-cli/docker_cli_links_test.go#L197-L204
+# https://github.com/moby/moby/blob/843e51459f14ebc964d349eba1013dc8a3e9d52e/integration-cli/docker_cli_run_test.go#L822-L843
+#
+
+@test "non-empty HOME env is used" {
+	update_config ' .process.env += ["HOME=/override"]'
+	update_config ' .process.args += ["-c", "echo $HOME"]'
+
+	runc run test_busybox
+	[ "$status" -eq 0 ]
+	[[ "${lines[0]}" == '/override' ]]
+}
+
+@test "empty HOME env var is overridden" {
+	update_config ' .process.env += ["HOME="]'
+	update_config ' .process.args += ["-c", "echo $HOME"]'
+
+	runc run test_busybox
+	[ "$status" -eq 0 ]
+	[[ "${lines[0]}" == '/root' ]]
+}
+
+@test "empty HOME env var is overridden with multiple overrides" {
+	update_config ' .process.env += ["HOME=/override", "HOME="]'
+	update_config ' .process.args += ["-c", "echo $HOME"]'
+
+	runc run test_busybox
+	[ "$status" -eq 0 ]
+	[[ "${lines[0]}" == '/root' ]]
+}
+
+@test "env var HOME is set only once" {
+	# env will show if an env var is set multiple times.
+	update_config ' .process.args = ["env"]'
+	update_config ' .process.env = ["HOME=", "PATH=/usr/bin:/bin"]'
+
+	runc run test_busybox
+	[ "$status" -eq 0 ]
+
+	# There should be 2 words/env-vars: HOME and PATH.
+	[ "$(wc -w <<<"$output")" -eq 2 ]
+}
+
+@test "env var override is set only once" {
+	# env will show if an env var is set multiple times.
+	update_config ' .process.args = ["env"]'
+	update_config ' .process.env = ["ONE=two", "ONE=", "PATH=/usr/bin:/bin"]'
+
+	runc run test_busybox
+	[ "$status" -eq 0 ]
+
+	# There should be 3 words/env-vars: ONE, PATH and HOME.
+	[ "$(wc -w <<<"$output")" -eq 3 ]
+}
+
+@test "env var override" {
+	update_config ' .process.env += ["ONE=two", "ONE=three"]'
+	update_config ' .process.args += ["-c", "echo ONE=\"$ONE\""]'
+
+	runc run test_busybox
+	[ "$status" -eq 0 ]
+	[[ "${lines[0]}" == "ONE=three" ]]
+}
+
+@test "env var with new-line is honored" {
+	update_config ' .process.env = ["NEW_LINE_ENV=\n", "PATH=/usr/bin:/bin"]'
+	update_config ' .process.args = ["env"]'
+
+	runc run test_busybox
+	[ "$status" -eq 0 ]
+
+	# There should be 4 lines
+	# NEW_LINE is a \n and when printed, it takes another line:
+	# 1. HOME=...
+	# 2. PATH=...
+	# 3. NEW_LINE_ENV=
+	# 4.
+	#
+	[ "$(wc -l <<<"$output")" -eq 4 ]
+}