-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
fix(exec): resolve wrapper recursion when shims are in PATH #8560
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,45 @@ | ||
| #!/usr/bin/env bash | ||
|
|
||
| # Regression test: a wrapper script that calls `mise x -- tool` should not | ||
| # cause infinite recursion when BOTH the wrapper directory AND the shims | ||
| # directory are in PATH, with the wrapper directory appearing first. | ||
| # | ||
| # This is the scenario that occurs in devcontainers where .devcontainer/bin | ||
| # contains wrapper scripts and mise shims are also in PATH. The PathEnv | ||
| # ordering puts "pre-shims" paths (including the wrapper dir) before | ||
| # mise-managed tool paths, so `which` finds the wrapper instead of the | ||
| # real binary, causing infinite recursion until E2BIG. | ||
|
|
||
| # 1. Install a mise-managed tool | ||
| cat >mise.toml <<'EOF' | ||
| [tools] | ||
| dummy = "latest" | ||
| EOF | ||
| mise i | ||
|
|
||
| # 2. Create a wrapper script that calls `mise x -- dummy` (simulating | ||
| # .devcontainer/bin/dummy or similar) | ||
| wrapperdir="$HOME/wrapper_bin" | ||
| mkdir -p "$wrapperdir" | ||
| cat >"$wrapperdir/dummy" <<WRAPPER | ||
| #!/bin/sh | ||
| exec mise x -- dummy | ||
| WRAPPER | ||
| chmod +x "$wrapperdir/dummy" | ||
|
|
||
| # 3. Put the wrapper directory BEFORE shims in PATH. This is the key | ||
| # difference from test_exec_wrapper_recursion: with shims in PATH, | ||
| # PathEnv classifies the wrapper dir as "pre" (before shims), and | ||
| # tool bins go into "mise" which comes after "pre". So the wrapper | ||
| # is found before the real binary. | ||
| shimdir="$MISE_DATA_DIR/shims" | ||
| mkdir -p "$shimdir" | ||
| export PATH="$wrapperdir:$shimdir:$PATH" | ||
|
|
||
| output="$(run_with_timeout 10 mise x -- dummy)" || { | ||
| echo "ERROR: mise x -- dummy timed out or failed (likely infinite recursion)" | ||
| exit 1 | ||
| } | ||
|
|
||
| # Should get real dummy output, not hang | ||
| assert_contains "echo '$output'" "This is Dummy" |
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -189,24 +189,37 @@ where | |||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||
| let args = args.into_iter().map(Into::into).collect::<Vec<_>>(); | ||||||||||||||||||||||||||||||||||||||||||||||||||
| let program = program.to_executable(); | ||||||||||||||||||||||||||||||||||||||||||||||||||
| // Strip shims directory from PATH for program resolution only, to prevent | ||||||||||||||||||||||||||||||||||||||||||||||||||
| // recursive shim execution. Wrapper scripts may call `mise x -- tool`, | ||||||||||||||||||||||||||||||||||||||||||||||||||
| // which re-enters Exec. If shims remain in PATH (due to | ||||||||||||||||||||||||||||||||||||||||||||||||||
| // not_found_auto_install), the wrapper is found again instead of the real | ||||||||||||||||||||||||||||||||||||||||||||||||||
| // tool, causing an infinite loop that grows PATH until E2BIG. | ||||||||||||||||||||||||||||||||||||||||||||||||||
| // The child process still inherits the full PATH (with shims) so | ||||||||||||||||||||||||||||||||||||||||||||||||||
| // subprocesses can find tools via shims. | ||||||||||||||||||||||||||||||||||||||||||||||||||
| let program = if program.to_string_lossy().contains('/') { | ||||||||||||||||||||||||||||||||||||||||||||||||||
| // Already a path, no need to resolve | ||||||||||||||||||||||||||||||||||||||||||||||||||
| program | ||||||||||||||||||||||||||||||||||||||||||||||||||
| } else { | ||||||||||||||||||||||||||||||||||||||||||||||||||
| let cwd = crate::dirs::CWD.clone().unwrap_or_default(); | ||||||||||||||||||||||||||||||||||||||||||||||||||
| let lookup_path = env.get(&*env::PATH_KEY).map(|path_val| { | ||||||||||||||||||||||||||||||||||||||||||||||||||
| // For program resolution, reorder PATH so that paths added by mise | ||||||||||||||||||||||||||||||||||||||||||||||||||
| // (tool bins, _.path entries) come before paths from the original | ||||||||||||||||||||||||||||||||||||||||||||||||||
| // system PATH. This prevents wrapper scripts in the system PATH | ||||||||||||||||||||||||||||||||||||||||||||||||||
| // (e.g. .devcontainer/bin/tool) from being found before the real | ||||||||||||||||||||||||||||||||||||||||||||||||||
| // tool binary, which would cause infinite recursion and E2BIG. | ||||||||||||||||||||||||||||||||||||||||||||||||||
| // | ||||||||||||||||||||||||||||||||||||||||||||||||||
| // User-configured paths (_.path/venv) maintain their position | ||||||||||||||||||||||||||||||||||||||||||||||||||
| // relative to tool paths since both are "mise-added". | ||||||||||||||||||||||||||||||||||||||||||||||||||
| // The child process still inherits the full unmodified PATH. | ||||||||||||||||||||||||||||||||||||||||||||||||||
| let shims_dir = &*crate::dirs::SHIMS; | ||||||||||||||||||||||||||||||||||||||||||||||||||
| let filtered: Vec<_> = std::env::split_paths(&OsString::from(path_val)) | ||||||||||||||||||||||||||||||||||||||||||||||||||
| .filter(|p| p != shims_dir) | ||||||||||||||||||||||||||||||||||||||||||||||||||
| let pristine: std::collections::HashSet<_> = crate::env::PATH.iter().collect(); | ||||||||||||||||||||||||||||||||||||||||||||||||||
| let all_paths: Vec<_> = std::env::split_paths(&OsString::from(path_val)).collect(); | ||||||||||||||||||||||||||||||||||||||||||||||||||
| // Mise-added paths first (preserving relative order) | ||||||||||||||||||||||||||||||||||||||||||||||||||
| let mise_added: Vec<_> = all_paths | ||||||||||||||||||||||||||||||||||||||||||||||||||
| .iter() | ||||||||||||||||||||||||||||||||||||||||||||||||||
| .filter(|p| !pristine.contains(p)) | ||||||||||||||||||||||||||||||||||||||||||||||||||
| .cloned() | ||||||||||||||||||||||||||||||||||||||||||||||||||
| .collect(); | ||||||||||||||||||||||||||||||||||||||||||||||||||
| std::env::join_paths(&filtered).unwrap() | ||||||||||||||||||||||||||||||||||||||||||||||||||
| // Then original system paths (minus shims) | ||||||||||||||||||||||||||||||||||||||||||||||||||
| let original: Vec<_> = all_paths | ||||||||||||||||||||||||||||||||||||||||||||||||||
| .iter() | ||||||||||||||||||||||||||||||||||||||||||||||||||
| .filter(|p| pristine.contains(p) && *p != shims_dir) | ||||||||||||||||||||||||||||||||||||||||||||||||||
| .cloned() | ||||||||||||||||||||||||||||||||||||||||||||||||||
| .collect(); | ||||||||||||||||||||||||||||||||||||||||||||||||||
| std::env::join_paths(mise_added.iter().chain(original.iter())).unwrap() | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
207
to
+222
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shims dir not filtered from
In practice this is safe today because mise's PATH construction puts tool-bin directories before the shims directory, so the real binary is still resolved first. However, a defensive
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||||||||||||||||||||
| match which::which_in(&program, lookup_path, cwd) { | ||||||||||||||||||||||||||||||||||||||||||||||||||
| Ok(resolved) => resolved.into_os_string(), | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -229,27 +242,50 @@ where | |||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||
| let cwd = crate::dirs::CWD.clone().unwrap_or_default(); | ||||||||||||||||||||||||||||||||||||||||||||||||||
| let program = program.to_executable(); | ||||||||||||||||||||||||||||||||||||||||||||||||||
| // Strip shims directory from PATH for program resolution only, to prevent | ||||||||||||||||||||||||||||||||||||||||||||||||||
| // recursive shim execution. On Windows, "file" mode shim scripts call | ||||||||||||||||||||||||||||||||||||||||||||||||||
| // `mise x -- tool`, which re-enters Exec. If shims remain in PATH (due to | ||||||||||||||||||||||||||||||||||||||||||||||||||
| // not_found_auto_install), which::which_in resolves "tool" back to the shim, | ||||||||||||||||||||||||||||||||||||||||||||||||||
| // causing an infinite loop. The child process still inherits the full PATH | ||||||||||||||||||||||||||||||||||||||||||||||||||
| // (with shims) so subprocesses can find tools via shims. | ||||||||||||||||||||||||||||||||||||||||||||||||||
| // Reorder PATH for program resolution: mise-added paths first, then | ||||||||||||||||||||||||||||||||||||||||||||||||||
| // original system paths (minus shims). See Unix version for full rationale. | ||||||||||||||||||||||||||||||||||||||||||||||||||
| let lookup_path = env.get(&*env::PATH_KEY).map(|path_val| { | ||||||||||||||||||||||||||||||||||||||||||||||||||
| // Compare with ~ expansion, normalized separators, and case-insensitive | ||||||||||||||||||||||||||||||||||||||||||||||||||
| // to handle Windows path variations (e.g. ~/.local/share/mise\shims vs | ||||||||||||||||||||||||||||||||||||||||||||||||||
| // C:\Users\user\.local\share\mise\shims) | ||||||||||||||||||||||||||||||||||||||||||||||||||
| let shims_normalized = crate::dirs::SHIMS | ||||||||||||||||||||||||||||||||||||||||||||||||||
| .to_string_lossy() | ||||||||||||||||||||||||||||||||||||||||||||||||||
| .to_lowercase() | ||||||||||||||||||||||||||||||||||||||||||||||||||
| .replace('/', "\\"); | ||||||||||||||||||||||||||||||||||||||||||||||||||
| let filtered: Vec<_> = std::env::split_paths(&OsString::from(path_val)) | ||||||||||||||||||||||||||||||||||||||||||||||||||
| let is_shims = |p: &std::path::PathBuf| { | ||||||||||||||||||||||||||||||||||||||||||||||||||
| let expanded = crate::file::replace_path(p); | ||||||||||||||||||||||||||||||||||||||||||||||||||
| expanded.to_string_lossy().to_lowercase().replace('/', "\\") == shims_normalized | ||||||||||||||||||||||||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||||||||||||||||||||||||
| let pristine: std::collections::HashSet<_> = crate::env::PATH | ||||||||||||||||||||||||||||||||||||||||||||||||||
| .iter() | ||||||||||||||||||||||||||||||||||||||||||||||||||
| .map(|p| { | ||||||||||||||||||||||||||||||||||||||||||||||||||
| crate::file::replace_path(p) | ||||||||||||||||||||||||||||||||||||||||||||||||||
| .to_string_lossy() | ||||||||||||||||||||||||||||||||||||||||||||||||||
| .to_lowercase() | ||||||||||||||||||||||||||||||||||||||||||||||||||
| .replace('/', "\\") | ||||||||||||||||||||||||||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||||||||||||||||||||||||||
| .collect(); | ||||||||||||||||||||||||||||||||||||||||||||||||||
| let all_paths: Vec<_> = std::env::split_paths(&OsString::from(path_val)).collect(); | ||||||||||||||||||||||||||||||||||||||||||||||||||
| let mise_added: Vec<_> = all_paths | ||||||||||||||||||||||||||||||||||||||||||||||||||
| .iter() | ||||||||||||||||||||||||||||||||||||||||||||||||||
| .filter(|p| { | ||||||||||||||||||||||||||||||||||||||||||||||||||
| let normalized = crate::file::replace_path(p) | ||||||||||||||||||||||||||||||||||||||||||||||||||
| .to_string_lossy() | ||||||||||||||||||||||||||||||||||||||||||||||||||
| .to_lowercase() | ||||||||||||||||||||||||||||||||||||||||||||||||||
| .replace('/', "\\"); | ||||||||||||||||||||||||||||||||||||||||||||||||||
| !pristine.contains(&normalized) | ||||||||||||||||||||||||||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||||||||||||||||||||||||||
| .cloned() | ||||||||||||||||||||||||||||||||||||||||||||||||||
| .collect(); | ||||||||||||||||||||||||||||||||||||||||||||||||||
| let original: Vec<_> = all_paths | ||||||||||||||||||||||||||||||||||||||||||||||||||
| .iter() | ||||||||||||||||||||||||||||||||||||||||||||||||||
| .filter(|p| { | ||||||||||||||||||||||||||||||||||||||||||||||||||
| let expanded = crate::file::replace_path(p); | ||||||||||||||||||||||||||||||||||||||||||||||||||
| expanded.to_string_lossy().to_lowercase().replace('/', "\\") != shims_normalized | ||||||||||||||||||||||||||||||||||||||||||||||||||
| let normalized = crate::file::replace_path(p) | ||||||||||||||||||||||||||||||||||||||||||||||||||
| .to_string_lossy() | ||||||||||||||||||||||||||||||||||||||||||||||||||
| .to_lowercase() | ||||||||||||||||||||||||||||||||||||||||||||||||||
| .replace('/', "\\"); | ||||||||||||||||||||||||||||||||||||||||||||||||||
| pristine.contains(&normalized) && !is_shims(p) | ||||||||||||||||||||||||||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||||||||||||||||||||||||||
| .cloned() | ||||||||||||||||||||||||||||||||||||||||||||||||||
| .collect(); | ||||||||||||||||||||||||||||||||||||||||||||||||||
| std::env::join_paths(&filtered).unwrap() | ||||||||||||||||||||||||||||||||||||||||||||||||||
| std::env::join_paths(mise_added.iter().chain(original.iter())).unwrap() | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+265
to
+288
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Windows: normalization logic duplicated three times The let normalize = |p: &std::path::PathBuf| {
crate::file::replace_path(p)
.to_string_lossy()
.to_lowercase()
.replace('/', "\\")
};
let pristine: std::collections::HashSet<_> = crate::env::PATH
.iter()
.map(|p| normalize(p))
.collect();
// ...then reuse normalize(p) in each filter |
||||||||||||||||||||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||||||||||||||||||||
| let program = which::which_in(program, lookup_path, cwd)?; | ||||||||||||||||||||||||||||||||||||||||||||||||||
| let cmd = cmd::cmd(program, args); | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shims not filtered from mise-added lookup paths
Low Severity
The old code unconditionally filtered the shims directory from the entire lookup PATH. The new code only filters shims from the
originalpartition (pristine.contains(p) && *p != shims_dir), but themise_addedpartition has no shims filtering at all. If the shims directory ever appears inall_pathsbut is not present in thepristineset (e.g., added by a cached env, a plugin, or a future code path), it would pass throughmise_addedunfiltered, re-enabling the recursive shim execution the fix is meant to prevent. This is a loss of defensive filtering compared to the previous implementation.Additional Locations (1)
src/cli/exec.rs#L265-L276