diff --git a/CHANGELOG.md b/CHANGELOG.md index 3f42f11309e..92e1b23cfde 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### libcontainer API + * `configs.CommandHook` struct has changed, Command is now a pointer. + Also, `configs.NewCommandHook` now accepts a `*Command`. (#4325) + ## [1.2.0] - 2024-10-22 > できるときにできることをやるんだ。それが今だ。 diff --git a/libcontainer/configs/config.go b/libcontainer/configs/config.go index dbf34f120cc..0e0ba57b179 100644 --- a/libcontainer/configs/config.go +++ b/libcontainer/configs/config.go @@ -434,6 +434,16 @@ func (hooks Hooks) Run(name HookName, state *specs.State) error { return nil } +// SetDefaultEnv sets the environment for those CommandHook entries +// that do not have one set. +func (hooks HookList) SetDefaultEnv(env []string) { + for _, h := range hooks { + if ch, ok := h.(CommandHook); ok && len(ch.Env) == 0 { + ch.Env = env + } + } +} + type Hook interface { // Run executes the hook with the provided state. Run(*specs.State) error @@ -463,17 +473,17 @@ type Command struct { } // NewCommandHook will execute the provided command when the hook is run. -func NewCommandHook(cmd Command) CommandHook { +func NewCommandHook(cmd *Command) CommandHook { return CommandHook{ Command: cmd, } } type CommandHook struct { - Command + *Command } -func (c Command) Run(s *specs.State) error { +func (c *Command) Run(s *specs.State) error { b, err := json.Marshal(s) if err != nil { return err diff --git a/libcontainer/configs/config_test.go b/libcontainer/configs/config_test.go index ceabaca737f..644129941ce 100644 --- a/libcontainer/configs/config_test.go +++ b/libcontainer/configs/config_test.go @@ -15,7 +15,7 @@ import ( func TestUnmarshalHooks(t *testing.T) { timeout := time.Second - hookCmd := configs.NewCommandHook(configs.Command{ + hookCmd := configs.NewCommandHook(&configs.Command{ Path: "/var/vcap/hooks/hook", Args: []string{"--pid=123"}, Env: []string{"FOO=BAR"}, @@ -52,7 +52,7 @@ func TestUnmarshalHooksWithInvalidData(t *testing.T) { func TestMarshalHooks(t *testing.T) { timeout := time.Second - hookCmd := configs.NewCommandHook(configs.Command{ + hookCmd := configs.NewCommandHook(&configs.Command{ Path: "/var/vcap/hooks/hook", Args: []string{"--pid=123"}, Env: []string{"FOO=BAR"}, @@ -84,7 +84,7 @@ func TestMarshalHooks(t *testing.T) { func TestMarshalUnmarshalHooks(t *testing.T) { timeout := time.Second - hookCmd := configs.NewCommandHook(configs.Command{ + hookCmd := configs.NewCommandHook(&configs.Command{ Path: "/var/vcap/hooks/hook", Args: []string{"--pid=123"}, Env: []string{"FOO=BAR"}, @@ -194,7 +194,7 @@ exit 0 } defer os.Remove(filename) - cmdHook := configs.NewCommandHook(configs.Command{ + cmdHook := configs.NewCommandHook(&configs.Command{ Path: filename, Args: []string{filename, "testarg"}, Env: []string{"FOO=BAR"}, @@ -216,7 +216,7 @@ func TestCommandHookRunTimeout(t *testing.T) { } timeout := 100 * time.Millisecond - cmdHook := configs.NewCommandHook(configs.Command{ + cmdHook := configs.NewCommandHook(&configs.Command{ Path: "/bin/sleep", Args: []string{"/bin/sleep", "1"}, Timeout: &timeout, diff --git a/libcontainer/env.go b/libcontainer/env.go new file mode 100644 index 00000000000..d205cb65014 --- /dev/null +++ b/libcontainer/env.go @@ -0,0 +1,59 @@ +package libcontainer + +import ( + "errors" + "fmt" + "os" + "slices" + "strings" +) + +// prepareEnv processes a list of environment variables, preparing it +// for direct consumption by unix.Exec. In particular, it: +// - validates each variable is in the NAME=VALUE format and +// 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. +// +// It returns the deduplicated environment, a flag telling whether HOME +// is present in the input, and an error. +func prepareEnv(env []string) ([]string, bool, error) { + if env == nil { + return nil, false, nil + } + // Deduplication code based on dedupEnv from Go 1.22 os/exec. + + // Construct the output in reverse order, to preserve the + // last occurrence of each key. + out := make([]string, 0, len(env)) + saw := make(map[string]bool, len(env)) + for n := len(env); n > 0; n-- { + kv := env[n-1] + i := strings.IndexByte(kv, '=') + if i == -1 { + return nil, false, errors.New("invalid environment variable: missing '='") + } + if i == 0 { + return nil, false, errors.New("invalid environment variable: name cannot be empty") + } + key := kv[:i] + if saw[key] { // Duplicate. + continue + } + saw[key] = true + if strings.IndexByte(kv, 0) >= 0 { + return nil, false, fmt.Errorf("invalid environment variable %q: contains nul byte (\\x00)", key) + } + if key == "PATH" { + // Needs to be set as it is used for binary lookup. + if err := os.Setenv("PATH", kv[i+1:]); err != nil { + return nil, false, err + } + } + out = append(out, kv) + } + // Restore the original order. + slices.Reverse(out) + + return out, saw["HOME"], nil +} diff --git a/libcontainer/env_test.go b/libcontainer/env_test.go new file mode 100644 index 00000000000..72d63b4af23 --- /dev/null +++ b/libcontainer/env_test.go @@ -0,0 +1,40 @@ +package libcontainer + +import ( + "slices" + "testing" +) + +func TestPrepareEnvDedup(t *testing.T) { + tests := []struct { + env, wantEnv []string + }{ + { + env: []string{}, + wantEnv: []string{}, + }, + { + env: []string{"HOME=/root", "FOO=bar"}, + wantEnv: []string{"HOME=/root", "FOO=bar"}, + }, + { + env: []string{"A=a", "A=b", "A=c"}, + wantEnv: []string{"A=c"}, + }, + { + 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"}, + }, + } + + for _, tc := range tests { + env, _, err := prepareEnv(tc.env) + if err != nil { + t.Error(err) + continue + } + if !slices.Equal(env, tc.wantEnv) { + t.Errorf("want %v, got %v", tc.wantEnv, env) + } + } +} diff --git a/libcontainer/factory_linux_test.go b/libcontainer/factory_linux_test.go index dec21afb212..7e4fcecfde0 100644 --- a/libcontainer/factory_linux_test.go +++ b/libcontainer/factory_linux_test.go @@ -31,14 +31,14 @@ func TestFactoryLoadContainer(t *testing.T) { id = "1" expectedHooks = configs.Hooks{ configs.Prestart: configs.HookList{ - configs.CommandHook{Command: configs.Command{Path: "prestart-hook"}}, + configs.CommandHook{Command: &configs.Command{Path: "prestart-hook"}}, }, configs.Poststart: configs.HookList{ - configs.CommandHook{Command: configs.Command{Path: "poststart-hook"}}, + configs.CommandHook{Command: &configs.Command{Path: "poststart-hook"}}, }, configs.Poststop: configs.HookList{ unserializableHook{}, - configs.CommandHook{Command: configs.Command{Path: "poststop-hook"}}, + configs.CommandHook{Command: &configs.Command{Path: "poststop-hook"}}, }, } expectedConfig = &configs.Config{ diff --git a/libcontainer/init_linux.go b/libcontainer/init_linux.go index b218a6cb126..af62c54e5df 100644 --- a/libcontainer/init_linux.go +++ b/libcontainer/init_linux.go @@ -11,7 +11,6 @@ import ( "runtime" "runtime/debug" "strconv" - "strings" "syscall" "github.com/containerd/console" @@ -185,8 +184,8 @@ func startInitialization() (retErr error) { defer pidfdSocket.Close() } - // clear the current process's environment to clean any libcontainer - // specific env vars. + // From here on, we don't need current process environment. It is not + // used directly anywhere below this point, but let's clear it anyway. os.Clearenv() defer func() { @@ -209,9 +208,11 @@ func startInitialization() (retErr error) { } func containerInit(t initType, config *initConfig, pipe *syncSocket, consoleSocket, pidfdSocket, fifoFile, logPipe *os.File) error { - if err := populateProcessEnvironment(config.Env); err != nil { + env, homeSet, err := prepareEnv(config.Env) + if err != nil { return err } + config.Env = env // Clean the RLIMIT_NOFILE cache in go runtime. // Issue: https://github.com/opencontainers/runc/issues/4195 @@ -225,6 +226,7 @@ func containerInit(t initType, config *initConfig, pipe *syncSocket, consoleSock pidfdSocket: pidfdSocket, config: config, logPipe: logPipe, + addHome: !homeSet, } return i.Init() case initStandard: @@ -236,36 +238,13 @@ func containerInit(t initType, config *initConfig, pipe *syncSocket, consoleSock config: config, fifoFile: fifoFile, logPipe: logPipe, + addHome: !homeSet, } return i.Init() } return fmt.Errorf("unknown init type %q", t) } -// populateProcessEnvironment loads the provided environment variables into the -// current processes's environment. -func populateProcessEnvironment(env []string) error { - for _, pair := range env { - name, val, ok := strings.Cut(pair, "=") - if !ok { - return errors.New("invalid environment variable: missing '='") - } - if name == "" { - return errors.New("invalid environment variable: name cannot be empty") - } - if strings.IndexByte(name, 0) >= 0 { - return fmt.Errorf("invalid environment variable %q: name contains nul byte (\\x00)", name) - } - if strings.IndexByte(val, 0) >= 0 { - return fmt.Errorf("invalid environment variable %q: value contains nul byte (\\x00)", name) - } - if err := os.Setenv(name, val); err != nil { - return err - } - } - return nil -} - // verifyCwd ensures that the current directory is actually inside the mount // namespace root of the current process. func verifyCwd() error { @@ -294,8 +273,8 @@ func verifyCwd() error { // finalizeNamespace drops the caps, sets the correct user // and working dir, and closes any leaked file descriptors -// before executing the command inside the namespace -func finalizeNamespace(config *initConfig) error { +// before executing the command inside the namespace. +func finalizeNamespace(config *initConfig, addHome bool) error { // Ensure that all unwanted fds we may have accidentally // inherited are marked close-on-exec so they stay out of the // container @@ -341,7 +320,7 @@ func finalizeNamespace(config *initConfig) error { if err := system.SetKeepCaps(); err != nil { return fmt.Errorf("unable to set keep caps: %w", err) } - if err := setupUser(config); err != nil { + if err := setupUser(config, addHome); err != nil { return fmt.Errorf("unable to setup user: %w", err) } // Change working directory AFTER the user has been set up, if we haven't done it yet. @@ -459,8 +438,9 @@ func syncParentSeccomp(pipe *syncSocket, seccompFd int) error { return readSync(pipe, procSeccompDone) } -// setupUser changes the groups, gid, and uid for the user inside the container -func setupUser(config *initConfig) error { +// setupUser changes the groups, gid, and uid for the user inside the container, +// and appends user's HOME to config.Env if addHome is true. +func setupUser(config *initConfig, addHome bool) error { // Set up defaults. defaultExecUser := user.ExecUser{ Uid: 0, @@ -541,11 +521,9 @@ func setupUser(config *initConfig) error { return err } - // if we didn't get HOME already, set it based on the user's HOME - if envHome := os.Getenv("HOME"); envHome == "" { - if err := os.Setenv("HOME", execUser.Home); err != nil { - return err - } + // If we didn't get HOME already, set it based on the user's HOME. + if addHome { + config.Env = append(config.Env, "HOME="+execUser.Home) } return nil } diff --git a/libcontainer/integration/bench_test.go b/libcontainer/integration/bench_test.go index da95b20b0b7..841d3aa06dd 100644 --- a/libcontainer/integration/bench_test.go +++ b/libcontainer/integration/bench_test.go @@ -1,7 +1,10 @@ package integration import ( + "bytes" + "math/rand" "os" + "strings" "testing" "github.com/opencontainers/runc/libcontainer" @@ -27,9 +30,7 @@ func BenchmarkExecTrue(b *testing.B) { _ = stdinR.Close() defer func() { _ = stdinW.Close() - if _, err := process.Wait(); err != nil { - b.Log(err) - } + waitProcess(process, b) }() ok(b, err) @@ -42,10 +43,81 @@ func BenchmarkExecTrue(b *testing.B) { LogLevel: "0", // Minimize forwardChildLogs involvement. } err := container.Run(exec) - if err != nil { - b.Fatal("exec failed:", err) + ok(b, err) + waitProcess(exec, b) + } + b.StopTimer() +} + +func genBigEnv(count int) []string { + randStr := func(length int) string { + const charset = "abcdefghijklmnopqrstuvwxyz0123456789_" + b := make([]byte, length) + for i := range b { + b[i] = charset[rand.Intn(len(charset))] } + return string(b) + } + + envs := make([]string, count) + for i := 0; i < count; i++ { + key := strings.ToUpper(randStr(10)) + value := randStr(20) + envs[i] = key + "=" + value + } + + return envs +} + +func BenchmarkExecInBigEnv(b *testing.B) { + config := newTemplateConfig(b, nil) + container, err := newContainer(b, config) + ok(b, err) + defer destroyContainer(container) + + // Execute a first process in the container + stdinR, stdinW, err := os.Pipe() + ok(b, err) + process := &libcontainer.Process{ + Cwd: "/", + Args: []string{"cat"}, + Env: standardEnvironment, + Stdin: stdinR, + Init: true, + } + err = container.Run(process) + _ = stdinR.Close() + defer func() { + _ = stdinW.Close() + waitProcess(process, b) + }() + ok(b, err) + + const numEnv = 5000 + env := append(standardEnvironment, genBigEnv(numEnv)...) + // Construct the expected output. + var wantOut bytes.Buffer + for _, e := range env { + wantOut.WriteString(e + "\n") + } + + b.ResetTimer() + for i := 0; i < b.N; i++ { + buffers := newStdBuffers() + exec := &libcontainer.Process{ + Cwd: "/", + Args: []string{"env"}, + Env: env, + Stdin: buffers.Stdin, + Stdout: buffers.Stdout, + Stderr: buffers.Stderr, + } + err = container.Run(exec) + ok(b, err) waitProcess(exec, b) + if !bytes.Equal(buffers.Stdout.Bytes(), wantOut.Bytes()) { + b.Fatalf("unexpected output: %s (stderr: %s)", buffers.Stdout, buffers.Stderr) + } } b.StopTimer() } diff --git a/libcontainer/integration/exec_test.go b/libcontainer/integration/exec_test.go index e3d26468dde..c9f37ada4c7 100644 --- a/libcontainer/integration/exec_test.go +++ b/libcontainer/integration/exec_test.go @@ -1022,13 +1022,13 @@ func TestHook(t *testing.T) { }), }, configs.CreateContainer: configs.HookList{ - configs.NewCommandHook(configs.Command{ + configs.NewCommandHook(&configs.Command{ Path: "/bin/bash", Args: []string{"/bin/bash", "-c", fmt.Sprintf("touch ./%s", hookFiles[configs.CreateContainer])}, }), }, configs.StartContainer: configs.HookList{ - configs.NewCommandHook(configs.Command{ + configs.NewCommandHook(&configs.Command{ Path: "/bin/sh", Args: []string{"/bin/sh", "-c", fmt.Sprintf("touch /%s", hookFiles[configs.StartContainer])}, }), diff --git a/libcontainer/integration/execin_test.go b/libcontainer/integration/execin_test.go index b9683b76442..a81d11edb71 100644 --- a/libcontainer/integration/execin_test.go +++ b/libcontainer/integration/execin_test.go @@ -5,6 +5,7 @@ import ( "fmt" "io" "os" + "slices" "strconv" "strings" "testing" @@ -345,16 +346,20 @@ func TestExecInEnvironment(t *testing.T) { defer stdinW.Close() //nolint: errcheck ok(t, err) + execEnv := []string{ + "PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin", + // The below line is added to check the deduplication feature: + // if a variable with the same name appears more than once, + // only the last value should be added to the environment. + "DEBUG=true", + "DEBUG=false", + "ENV=test", + } buffers := newStdBuffers() process2 := &libcontainer.Process{ - Cwd: "/", - Args: []string{"env"}, - Env: []string{ - "PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin", - "DEBUG=true", - "DEBUG=false", - "ENV=test", - }, + Cwd: "/", + Args: []string{"/bin/env"}, + Env: execEnv, Stdin: buffers.Stdin, Stdout: buffers.Stdout, Stderr: buffers.Stderr, @@ -366,14 +371,24 @@ func TestExecInEnvironment(t *testing.T) { _ = stdinW.Close() waitProcess(process, t) - out := buffers.Stdout.String() - // check execin's process environment - if !strings.Contains(out, "DEBUG=false") || - !strings.Contains(out, "ENV=test") || - !strings.Contains(out, "HOME=/root") || - !strings.Contains(out, "PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin") || - strings.Contains(out, "DEBUG=true") { - t.Fatalf("unexpected running process, output %q", out) + // Check exec process environment. + t.Logf("exec output:\n%s", buffers.Stdout.String()) + out := strings.Fields(buffers.Stdout.String()) + // If not present in the Process.Env, runc should add $HOME, + // which is deduced by parsing container's /etc/passwd. + // We use a known image in the test, so we know its value. + for _, e := range append(execEnv, "HOME=/root") { + if e == "DEBUG=true" { // This one should be dedup'ed out. + continue + } + if !slices.Contains(out, e) { + t.Errorf("Expected env %s not found in exec output", e) + } + } + // Check that there are no extra variables. We have 1 env ("DEBUG=true") + // removed and 1 env ("HOME=root") added, resulting in the same number. + if got, want := len(out), len(execEnv); want != got { + t.Errorf("Mismatched number of variables: want %d, got %d", want, got) } } diff --git a/libcontainer/setns_init_linux.go b/libcontainer/setns_init_linux.go index 462662a84ed..b42b3be1a89 100644 --- a/libcontainer/setns_init_linux.go +++ b/libcontainer/setns_init_linux.go @@ -25,6 +25,7 @@ type linuxSetnsInit struct { pidfdSocket *os.File config *initConfig logPipe *os.File + addHome bool } func (l *linuxSetnsInit) getSessionRingName() string { @@ -101,7 +102,7 @@ func (l *linuxSetnsInit) Init() error { return err } } - if err := finalizeNamespace(l.config); err != nil { + if err := finalizeNamespace(l.config, l.addHome); err != nil { return err } if err := apparmor.ApplyProfile(l.config.AppArmorProfile); err != nil { @@ -154,5 +155,5 @@ func (l *linuxSetnsInit) Init() error { if err := utils.UnsafeCloseFrom(l.config.PassedFilesCount + 3); err != nil { return err } - return system.Exec(name, l.config.Args, os.Environ()) + return system.Exec(name, l.config.Args, l.config.Env) } diff --git a/libcontainer/specconv/spec_linux.go b/libcontainer/specconv/spec_linux.go index 79a9a790049..6e3e5bde371 100644 --- a/libcontainer/specconv/spec_linux.go +++ b/libcontainer/specconv/spec_linux.go @@ -1256,8 +1256,8 @@ func createHooks(rspec *specs.Spec, config *configs.Config) { } } -func createCommandHook(h specs.Hook) configs.Command { - cmd := configs.Command{ +func createCommandHook(h specs.Hook) *configs.Command { + cmd := &configs.Command{ Path: h.Path, Args: h.Args, Env: h.Env, diff --git a/libcontainer/standard_init_linux.go b/libcontainer/standard_init_linux.go index bb1eb300188..510f9483baa 100644 --- a/libcontainer/standard_init_linux.go +++ b/libcontainer/standard_init_linux.go @@ -27,6 +27,7 @@ type linuxStandardInit struct { fifoFile *os.File logPipe *os.File config *initConfig + addHome bool } func (l *linuxStandardInit) getSessionRingParams() (string, uint32, uint32) { @@ -186,7 +187,7 @@ func (l *linuxStandardInit) Init() error { return err } } - if err := finalizeNamespace(l.config); err != nil { + if err := finalizeNamespace(l.config, l.addHome); err != nil { return err } // finalizeNamespace can change user/group which clears the parent death @@ -194,6 +195,17 @@ func (l *linuxStandardInit) Init() error { if err := pdeath.Restore(); err != nil { return fmt.Errorf("can't restore pdeath signal: %w", err) } + + // In case we have any StartContainer hooks to run, and they don't + // have environment configured explicitly, make sure they will be run + // with the same environment as container's init. + // + // NOTE the above described behavior is not part of runtime-spec, but + // rather a de facto historical thing we afraid to change. + if h := l.config.Config.Hooks[configs.StartContainer]; len(h) > 0 { + h.SetDefaultEnv(l.config.Env) + } + // Compare the parent from the initial start of the init process and make // sure that it did not change. if the parent changes that means it died // and we were reparented to something else so we should just kill ourself @@ -285,5 +297,5 @@ func (l *linuxStandardInit) Init() error { if err := utils.UnsafeCloseFrom(l.config.PassedFilesCount + 3); err != nil { return err } - return system.Exec(name, l.config.Args, os.Environ()) + return system.Exec(name, l.config.Args, l.config.Env) } diff --git a/tests/integration/hooks.bats b/tests/integration/hooks.bats index 099337a2b2c..e9d62b0e6c5 100644 --- a/tests/integration/hooks.bats +++ b/tests/integration/hooks.bats @@ -42,3 +42,24 @@ function teardown() { [[ "$output" == *"error running $hook hook #1:"* ]] done } + +# While runtime-spec does not say what environment variables hooks should have, +# if not explicitly specified, historically the StartContainer hook inherited +# the process environment specified for init. +# +# Check this behavior is preserved. +@test "runc run [startContainer hook should inherit process environment]" { + cat >"rootfs/check-env.sh" <<-'EOF' + #!/bin/sh -ue + test $ONE = two + test $FOO = bar + echo $HOME # Test HOME is set w/o checking the value. + EOF + chmod +x "rootfs/check-env.sh" + + update_config ' .process.args = ["/bin/true"] + | .process.env = ["ONE=two", "FOO=bar"] + | .hooks |= {"startContainer": [{"path": "/check-env.sh"}]}' + runc run ct1 + [ "$status" -eq 0 ] +}