Merge pull request #4325 from kolyshkin/opt-env

libct: speedup process.Env handling

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
 
 > できるときにできることをやるんだ。それが今だ。

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

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,

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
+}

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)
+		}
+	}
+}

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{

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
 }

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()
+}

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])},
 			}),

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)
 	}
 }
 

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)
 }

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,

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)
 }

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 ]
+}