From 09501d96d2d72709223b50458ed569cca93247d8 Mon Sep 17 00:00:00 2001 From: Rodrigo Campos Date: Tue, 1 Apr 2025 17:13:08 +0200 Subject: [PATCH 1/2] libct: Override HOME if its set to the empty string 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 Signed-off-by: Rodrigo Campos --- libcontainer/env.go | 18 +++++++++++++++--- libcontainer/env_test.go | 12 ++++++++++++ 2 files changed, 27 insertions(+), 3 deletions(-) diff --git a/libcontainer/env.go b/libcontainer/env.go index fe1abd500..71b2f69d0 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. diff --git a/libcontainer/env_test.go b/libcontainer/env_test.go index c91747167..18dc52e88 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 { From 19c65154712e46f8ce154b7972fe522fa3b9cc35 Mon Sep 17 00:00:00 2001 From: Rodrigo Campos Date: Wed, 2 Apr 2025 14:01:48 +0200 Subject: [PATCH 2/2] tests: Add env var tests This adds some e2e tests for environment variables set in the config.json. These were based on tests that failed on docker CI[1][2] after the refactor on 06f1e0765 ("libct: speedup process.Env handling") and some bugs that I had along the way trying to fix it. These tests pass with runc 1.2 too. [1]: https://github.com/moby/moby/blob/843e51459f14ebc964d349eba1013dc8a3e9d52e/integration-cli/docker_cli_run_test.go#L822-L843 [2]: https://github.com/moby/moby/blob/843e51459f14ebc964d349eba1013dc8a3e9d52e/integration-cli/docker_cli_links_test.go#L197-L204 Signed-off-by: Rodrigo Campos --- tests/integration/env.bats | 97 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 97 insertions(+) create mode 100644 tests/integration/env.bats diff --git a/tests/integration/env.bats b/tests/integration/env.bats new file mode 100644 index 000000000..cb984942e --- /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 ] +}