Merge pull request #4325 from kolyshkin/opt-env

libct: speedup process.Env handling

Author: lifubang

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
 }