From 6b8c3476bce895ee5d7a07071ad215051987fed1 Mon Sep 17 00:00:00 2001 From: Dmitry Meyer Date: Wed, 19 Aug 2026 09:33:46 +0000 Subject: [PATCH] [runner] Remove `BASH_FUNC_*%%` variables * Don't propagate exported Bash functions to `commands` and SSH sessions (`/dstack/profile`) * Skip variables with bad names while generating `/dstack/profile` Fixes: https://github.com/dstackai/dstack/issues/4161 --- runner/internal/runner/executor/env.go | 56 ++++++++++++++ runner/internal/runner/executor/env_test.go | 76 +++++++++++++++++++ runner/internal/runner/executor/executor.go | 11 ++- .../internal/runner/executor/executor_test.go | 27 ++++++- 4 files changed, 167 insertions(+), 3 deletions(-) diff --git a/runner/internal/runner/executor/env.go b/runner/internal/runner/executor/env.go index ff91a0c8c4..dad7f53a4c 100644 --- a/runner/internal/runner/executor/env.go +++ b/runner/internal/runner/executor/env.go @@ -1,8 +1,11 @@ package executor import ( + "context" "fmt" "strings" + + "github.com/dstackai/dstack/runner/internal/common/log" ) type EnvMap map[string]string @@ -47,6 +50,59 @@ func ParseEnvList(list []string) EnvMap { return em } +// sanitizeEnv removes variables that must not be propagated to `commands` and SSH sessions. +func sanitizeEnv(ctx context.Context, env map[string]string) { + for name := range env { + // Exported Bash functions are automatically "imported" by child shells, even in + // POSIX mode. We remove such variables for consistency: + // + // * Even if we preserved them in `/dstack/profile`, they wouldn't work with + // non-login SSH sessions (`ssh run-name command arg1 arg2 ...`), + // as `/dstack/profile` is not sourced by non-login shells. + // * Swapping `image` (e.g, `ubuntu` to `fedora`) or `shell` (e.g., `bash` to `sh`) + // in the run configuration should not change which functions are available in `commands`. + // If functions are essential for `commands`, the user should source them explicitly. + // + // See: https://github.com/dstackai/dstack/issues/4161 + if isBashFuncName(name) { + log.Info(ctx, "Removed Bash exported function variable", "var", name) + delete(env, name) + } + } +} + +// isBashFuncName reports whether the variable name holds a body of a Bash function exported +// via `export -f foo`. Bash mangles the name to keep such variables out of the way of regular +// ones, the exact encoding depends on the version: +// +// - `BASH_FUNC_foo%%` -- upstream Bash, that is, any reasonably modern distro. +// - `BASH_FUNC_foo()` -- Red Hat's Bash 4.1/4.2 patch, that is, RHEL/CentOS 7 and older. +// +// Both encodings were introduced by the Shellshock patches. Bash without them exports the +// function as `foo`, which we deliberately don't detect: the name is indistinguishable from +// a regular variable, and, being a valid shell identifier, it doesn't break `/dstack/profile`. +func isBashFuncName(name string) bool { + if !strings.HasPrefix(name, "BASH_FUNC_") { + return false + } + return strings.HasSuffix(name, "%%") || strings.HasSuffix(name, "()") +} + +// isShellIdentifier reports whether the variable name is a valid shell identifier, that is, +// whether `export NAME=value` is a valid command. Variables with other names are perfectly +// valid as far as execve(2) is concerned, but cannot be exported in `/dstack/profile`. +func isShellIdentifier(name string) bool { + if name == "" || !isAlpha(name[0]) { + return false + } + for i := 1; i < len(name); i++ { + if !isAlphaNum(name[i]) { + return false + } + } + return true +} + // interpolateVariables expands variables as follows: // `$VARNAME` -> literal `$VARNAME` (curly brackets are mandatory, bare $ means nothing) // `${VARNAME}` -> getter("VARNAME") return value diff --git a/runner/internal/runner/executor/env_test.go b/runner/internal/runner/executor/env_test.go index 10cfc25fb4..68324abc12 100644 --- a/runner/internal/runner/executor/env_test.go +++ b/runner/internal/runner/executor/env_test.go @@ -1,6 +1,7 @@ package executor import ( + "context" "testing" "github.com/stretchr/testify/assert" @@ -113,3 +114,78 @@ func TestEnvMapUpdate_Merge_Expand(t *testing.T) { } assert.Equal(t, expected, envMap) } + +func TestIsBashFuncName(t *testing.T) { + testCases := []struct { + name string + expected bool + }{ + // upstream Bash + {"BASH_FUNC_foo%%", true}, + {"BASH_FUNC_ml%%", true}, + // Bash function names are not limited to shell identifiers + {"BASH_FUNC_foo-bar%%", true}, + {"BASH_FUNC_foo.bar()", true}, + // Red Hat's Bash 4.1/4.2 (RHEL/CentOS 7 and older) + {"BASH_FUNC_module()", true}, + // no function name, still not a valid identifier + {"BASH_FUNC_%%", true}, + {"BASH_FUNC_()", true}, + // regular variables + {"", false}, + {"PATH", false}, + {"BASH_FUNC_foo", false}, + {"BASH_FUNC_", false}, + {"bash_func_foo%%", false}, + {"FOO%%", false}, + {"FOO()", false}, + } + for _, tc := range testCases { + assert.Equal(t, tc.expected, isBashFuncName(tc.name), tc.name) + } +} + +func TestIsShellIdentifier(t *testing.T) { + testCases := []struct { + name string + expected bool + }{ + {"VAR", true}, + {"_", true}, + {"_var1", true}, + {"VAR_1_2", true}, + {"", false}, + {"1VAR", false}, + {"VAR-1", false}, + {"VAR 1", false}, + {"VAR.1", false}, + {"BASH_FUNC_foo%%", false}, + {"BASH_FUNC_foo()", false}, + } + for _, tc := range testCases { + assert.Equal(t, tc.expected, isShellIdentifier(tc.name), tc.name) + } +} + +func TestSanitizeEnv(t *testing.T) { + env := EnvMap{ + "PATH": "/bin:/sbin", + "BASH_FUNC_NOT_A_FUNC": "just a variable", + "BASH_FUNC_ml%%": "() { eval $($LMOD_DIR/ml_cmd \"$@\")\n}", + "BASH_FUNC_module()": "() { eval $($LMOD_CMD bash \"$@\")\n}", + } + + sanitizeEnv(context.Background(), env) + + expected := EnvMap{ + "PATH": "/bin:/sbin", + "BASH_FUNC_NOT_A_FUNC": "just a variable", + } + assert.Equal(t, expected, env) +} + +func TestSanitizeEnv_Empty(t *testing.T) { + env := EnvMap{} + sanitizeEnv(context.Background(), env) + assert.Equal(t, EnvMap{}, env) +} diff --git a/runner/internal/runner/executor/executor.go b/runner/internal/runner/executor/executor.go index c1533f4730..45cb429636 100644 --- a/runner/internal/runner/executor/executor.go +++ b/runner/internal/runner/executor/executor.go @@ -543,10 +543,11 @@ func (ex *RunExecutor) execJob(ctx context.Context, jobLogFile io.Writer) error envMap := NewEnvMap(ParseEnvList(os.Environ()), jobEnvs, ex.secrets) // `env` interpolation feature is postponed to some future release envMap.Update(ex.jobSpec.Env, false) + sanitizeEnv(ctx, envMap) const profilePath = "/etc/profile" dstackProfilePath := path.Join(ex.dstackDir, "profile") - if err := writeDstackProfile(envMap, dstackProfilePath); err != nil { + if err := writeDstackProfile(ctx, envMap, dstackProfilePath); err != nil { log.Warning(ctx, "failed to write dstack_profile", "path", dstackProfilePath, "err", err) } else if err := includeDstackProfile(profilePath, dstackProfilePath); err != nil { log.Warning(ctx, "failed to include dstack_profile", "path", profilePath, "err", err) @@ -815,7 +816,7 @@ func writeMpiHostfile(ctx context.Context, ips []string, slots []int, path strin return nil } -func writeDstackProfile(env map[string]string, pth string) error { +func writeDstackProfile(ctx context.Context, env map[string]string, pth string) error { if err := os.MkdirAll(path.Dir(pth), 0o755); err != nil { return fmt.Errorf("create dstack profile directory: %w", err) } @@ -829,6 +830,12 @@ func writeDstackProfile(env map[string]string, pth string) error { case "HOSTNAME", "USER", "HOME", "SHELL", "SHLVL", "PWD", "_": continue } + // `export not-an-identifier=value` is a syntax error that either pollutes stderr on + // every login or, depending on the shell, aborts the profile altogether. + if !isShellIdentifier(key) { + log.Warning(ctx, "Skipped env variable, name is not a valid shell identifier", "var", key) + continue + } line := fmt.Sprintf("export %s='%s'\n", key, strings.ReplaceAll(value, `'`, `'"'"'`)) if _, err = file.WriteString(line); err != nil { return fmt.Errorf("write dstack profile: %w", err) diff --git a/runner/internal/runner/executor/executor_test.go b/runner/internal/runner/executor/executor_test.go index d6878ea3ee..90a942328a 100644 --- a/runner/internal/runner/executor/executor_test.go +++ b/runner/internal/runner/executor/executor_test.go @@ -279,7 +279,7 @@ func TestWriteDstackProfile(t *testing.T) { script := fmt.Sprintf(`. '%s'; printf '%%s' "$VAR"`, path) for _, value := range testCases { env := map[string]string{"VAR": value} - writeDstackProfile(env, path) + writeDstackProfile(t.Context(), env, path) cmd := exec.CommandContext(t.Context(), "/bin/sh", "-c", script) out, err := cmd.Output() assert.NoError(t, err) @@ -287,6 +287,31 @@ func TestWriteDstackProfile(t *testing.T) { } } +func TestWriteDstackProfile_NotShellIdentifiers(t *testing.T) { + tmp := t.TempDir() + path := tmp + "/dstack_profile" + script := fmt.Sprintf(`. '%s'; printf '%%s' "$VAR"`, path) + env := map[string]string{ + "VAR": "value", + "NOT-AN-IDENTIFIER": "value", + "0NOTANIDENTIFIER": "value", + "NOT AN IDENTIFIER": "value", + "BASH_FUNC_foo%%": "() { echo hi\n}", + } + + require.NoError(t, writeDstackProfile(t.Context(), env, path)) + + cmd := exec.CommandContext(t.Context(), "/bin/sh", "-c", script) + var stderr bytes.Buffer + cmd.Stderr = &stderr + out, err := cmd.Output() + // Some shells only complain about a bad name, others abort the profile altogether, + // leaving VAR unset + assert.NoError(t, err) + assert.Empty(t, stderr.String()) + assert.Equal(t, "value", string(out)) +} + func TestWriteMpiHostfile(t *testing.T) { tmp := t.TempDir()