-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
fix(shim): prevent infinite recursion when system shims dir is on PATH #8816
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,36 @@ | ||
| #!/usr/bin/env bash | ||
|
|
||
| # Regression test: when a tool is installed --system (or any scenario where | ||
| # BOTH the user shims dir AND a system shims dir are on PATH), invoking a | ||
| # shim for a tool that is NOT in any config file must NOT recurse infinitely. | ||
| # | ||
| # Root cause: which_shim's PATH fallback skipped only dirs::SHIMS (user shims). | ||
| # If a second mise shims dir (e.g. /usr/local/share/mise/shims) was also on | ||
| # PATH it would be found as the "fallback binary" and exec'd, re-entering the | ||
| # same shim logic — infinite loop. | ||
|
|
||
| shimdir="$MISE_DATA_DIR/shims" | ||
| mkdir -p "$shimdir" | ||
|
|
||
| # Simulate a second (system-level) shims directory on PATH. | ||
| # The binary inside it is a symlink to the mise binary, exactly like a real | ||
| # system shim created by `mise install --system`. | ||
| sys_shimdir="$(mktemp -d)" | ||
| trap 'rm -rf "$sys_shimdir"' EXIT | ||
| mise_bin="$(command -v mise)" | ||
| ln -s "$mise_bin" "$sys_shimdir/dummy" | ||
|
|
||
| # Put both shims dirs on PATH — the user shims dir first, then the fake | ||
| # system shims dir. This is the exact PATH layout that triggered the bug. | ||
| export PATH="$shimdir:$sys_shimdir:$PATH" | ||
|
|
||
| # dummy is not in any config file. The shim must fail fast with a clear | ||
| # error rather than looping forever. | ||
| output="$(run_with_timeout 10 dummy 2>&1)" && { | ||
| echo "ERROR: dummy should have failed but exited 0" | ||
| exit 1 | ||
| } || true | ||
|
|
||
| # Should contain a helpful error, not be empty (which would indicate a hang | ||
| # that was killed) or a recursion-induced crash. | ||
| assert_contains "echo '$output'" "dummy" | ||
|
greptile-apps[bot] marked this conversation as resolved.
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -204,7 +204,9 @@ where | |
| // 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 user_shims = &*crate::dirs::SHIMS; | ||
| let sys_shims = crate::env::MISE_SYSTEM_DATA_DIR.join("shims"); | ||
| let is_shims_dir = |p: &std::path::PathBuf| p == user_shims || p == &sys_shims; | ||
| 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) | ||
|
|
@@ -216,7 +218,7 @@ where | |
| // Then original system paths (minus shims) | ||
| let original: Vec<_> = all_paths | ||
| .iter() | ||
| .filter(|p| pristine.contains(p) && *p != shims_dir) | ||
| .filter(|p| pristine.contains(p) && !is_shims_dir(p)) | ||
| .cloned() | ||
| .collect(); | ||
| std::env::join_paths(mise_added.iter().chain(original.iter())).unwrap() | ||
|
|
@@ -249,9 +251,15 @@ where | |
| .to_string_lossy() | ||
| .to_lowercase() | ||
| .replace('/', "\\"); | ||
| let sys_shims_normalized = crate::env::MISE_SYSTEM_DATA_DIR | ||
| .join("shims") | ||
| .to_string_lossy() | ||
| .to_lowercase() | ||
| .replace('/', "\\"); | ||
|
Comment on lines
+254
to
+258
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. The path normalization logic ( To improve maintainability and reduce duplication, consider extracting this logic into a local helper closure or function within |
||
| let is_shims = |p: &std::path::PathBuf| { | ||
| let expanded = crate::file::replace_path(p); | ||
| expanded.to_string_lossy().to_lowercase().replace('/', "\\") == shims_normalized | ||
| let normalized = expanded.to_string_lossy().to_lowercase().replace('/', "\\"); | ||
| normalized == shims_normalized || normalized == sys_shims_normalized | ||
| }; | ||
| let pristine: std::collections::HashSet<_> = crate::env::PATH | ||
| .iter() | ||
|
|
||
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.
To ensure the temporary directory is always cleaned up, even if the script exits unexpectedly, it's a good practice to use
trapfor cleanup. This makes the test more robust. The explicitrm -rfat the end of the script can then be removed.