[runner] Remove BASH_FUNC_*%% variables

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

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

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)

runner/internal/runner/executor/executor_test.go

@@ -279,14 +279,39 @@ 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)
 		assert.Equal(t, value, string(out))
 	}
 }
 
+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()