From 0c8b573cbb93f89c6f589f8c50e0cfb173c28c4c Mon Sep 17 00:00:00 2001 From: Chawye Hsu Date: Thu, 23 May 2024 12:52:11 +0800 Subject: [PATCH 01/19] fix: fix fork bomb on Windows Signed-off-by: Chawye Hsu --- Cargo.lock | 1 + crates/volta-core/Cargo.toml | 1 + crates/volta-core/src/run/executor.rs | 43 ++++++++++++++++++++------- 3 files changed, 35 insertions(+), 10 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index c7165d7e4..b7a23a200 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1646,6 +1646,7 @@ dependencies = [ "validate-npm-package-name", "volta-layout", "walkdir", + "which", "winreg", ] diff --git a/crates/volta-core/Cargo.toml b/crates/volta-core/Cargo.toml index 0bc639be5..ae2c000fe 100644 --- a/crates/volta-core/Cargo.toml +++ b/crates/volta-core/Cargo.toml @@ -55,3 +55,4 @@ fs2 = "0.4.3" [target.'cfg(windows)'.dependencies] winreg = "0.52.0" +which = "6.0.1" diff --git a/crates/volta-core/src/run/executor.rs b/crates/volta-core/src/run/executor.rs index 175223ed4..322f3b35e 100644 --- a/crates/volta-core/src/run/executor.rs +++ b/crates/volta-core/src/run/executor.rs @@ -1,5 +1,5 @@ use std::collections::HashMap; -use std::ffi::OsStr; +use std::ffi::{OsStr, OsString}; #[cfg(unix)] use std::os::unix::process::ExitStatusExt; #[cfg(windows)] @@ -113,7 +113,9 @@ impl From> for Executor { /// Tracks the Platform as well as what kind of tool is being executed, to allow individual tools /// to customize the behavior before execution. pub struct ToolCommand { - command: Command, + exe: OsString, + args: Vec, + envs: HashMap, platform: Option, kind: ToolKind, } @@ -137,11 +139,13 @@ impl ToolCommand { A: IntoIterator, S: AsRef, { - let mut command = create_command(exe); - command.args(args); + let exe = exe.as_ref().into(); + let args = args.into_iter().map(|s| s.as_ref().into()).collect(); Self { - command, + exe, + args, + envs: HashMap::new(), platform, kind, } @@ -154,7 +158,9 @@ impl ToolCommand { K: AsRef, V: AsRef, { - self.command.envs(envs); + for (key, value) in envs { + self.env(key, value); + } } /// Adds or updates a single environment variable that the command will use @@ -163,7 +169,7 @@ impl ToolCommand { K: AsRef, V: AsRef, { - self.command.env(key, value); + self.envs.insert(key.as_ref().into(), value.as_ref().into()); } /// Updates the Platform for the command to include values from the command-line @@ -191,11 +197,28 @@ impl ToolCommand { ToolKind::Bypass(command) => (System::path()?, ErrorKind::BypassError { command }), }; - self.command.env(RECURSION_ENV_VAR, "1"); - self.command.env("PATH", path); + // On Windows, we need to explicitly use an absolute path to the executable, + // otherwise the executable will not be located properly, even if we've set the PATH. + // see: https://github.com/rust-lang/rust/issues/37519 + #[cfg(windows)] + { + let mut abs_paths = which::which_in_global(self.exe.clone(), Some(&path)) + .with_context(|| ErrorKind::BinaryExecError)?; + if let Some(abs_exe) = abs_paths.next() { + self.exe = abs_exe.into(); + } else { + return Err(ErrorKind::BinaryExecError.into()); + } + } + + let mut command = create_command(&self.exe); + command.args(&self.args); + command.envs(&self.envs); + command.env(RECURSION_ENV_VAR, "1"); + command.env("PATH", path); pass_control_to_shim(); - self.command.status().with_context(|| on_failure) + command.status().with_context(|| on_failure) } } From d163d5d65718ded55f29f6b392e9227f5bfbb505 Mon Sep 17 00:00:00 2001 From: Chawye Hsu Date: Thu, 23 May 2024 13:16:37 +0800 Subject: [PATCH 02/19] fix mut Signed-off-by: Chawye Hsu --- crates/volta-core/src/run/executor.rs | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/crates/volta-core/src/run/executor.rs b/crates/volta-core/src/run/executor.rs index 322f3b35e..67a73225f 100644 --- a/crates/volta-core/src/run/executor.rs +++ b/crates/volta-core/src/run/executor.rs @@ -181,7 +181,7 @@ impl ToolCommand { } /// Runs the command, returning the `ExitStatus` if it successfully launches - pub fn execute(mut self, session: &mut Session) -> Fallible { + pub fn execute(self, session: &mut Session) -> Fallible { let (path, on_failure) = match self.kind { ToolKind::Node => super::node::execution_context(self.platform, session)?, ToolKind::Npm => super::npm::execution_context(self.platform, session)?, @@ -197,21 +197,22 @@ impl ToolCommand { ToolKind::Bypass(command) => (System::path()?, ErrorKind::BypassError { command }), }; + let mut exe = self.exe; // On Windows, we need to explicitly use an absolute path to the executable, // otherwise the executable will not be located properly, even if we've set the PATH. // see: https://github.com/rust-lang/rust/issues/37519 #[cfg(windows)] { - let mut abs_paths = which::which_in_global(self.exe.clone(), Some(&path)) + let mut abs_paths = which::which_in_global(exe.clone(), Some(&path)) .with_context(|| ErrorKind::BinaryExecError)?; if let Some(abs_exe) = abs_paths.next() { - self.exe = abs_exe.into(); + exe = abs_exe.into(); } else { return Err(ErrorKind::BinaryExecError.into()); } } - let mut command = create_command(&self.exe); + let mut command = create_command(exe); command.args(&self.args); command.envs(&self.envs); command.env(RECURSION_ENV_VAR, "1"); From a26bd6377f380760f622b4d46df2f1d481ed6c07 Mon Sep 17 00:00:00 2001 From: Chawye Hsu Date: Thu, 23 May 2024 13:25:28 +0800 Subject: [PATCH 03/19] fix mut Signed-off-by: Chawye Hsu --- crates/volta-core/src/run/executor.rs | 31 ++++++++++++++++----------- 1 file changed, 18 insertions(+), 13 deletions(-) diff --git a/crates/volta-core/src/run/executor.rs b/crates/volta-core/src/run/executor.rs index 67a73225f..7b021998b 100644 --- a/crates/volta-core/src/run/executor.rs +++ b/crates/volta-core/src/run/executor.rs @@ -197,20 +197,25 @@ impl ToolCommand { ToolKind::Bypass(command) => (System::path()?, ErrorKind::BypassError { command }), }; - let mut exe = self.exe; - // On Windows, we need to explicitly use an absolute path to the executable, - // otherwise the executable will not be located properly, even if we've set the PATH. - // see: https://github.com/rust-lang/rust/issues/37519 - #[cfg(windows)] - { - let mut abs_paths = which::which_in_global(exe.clone(), Some(&path)) - .with_context(|| ErrorKind::BinaryExecError)?; - if let Some(abs_exe) = abs_paths.next() { - exe = abs_exe.into(); - } else { - return Err(ErrorKind::BinaryExecError.into()); + let exe = { + #[cfg(not(windows))] + { + self.exe } - } + // On Windows, we need to explicitly use an absolute path to the executable, + // otherwise the executable will not be located properly, even if we've set the PATH. + // see: https://github.com/rust-lang/rust/issues/37519 + #[cfg(windows)] + { + let mut abs_paths = which::which_in_global(self.exe.clone(), Some(&path)) + .with_context(|| ErrorKind::BinaryExecError)?; + if let Some(abs_exe) = abs_paths.next() { + abs_exe.into_os_string() + } else { + return Err(ErrorKind::BinaryExecError.into()); + } + } + }; let mut command = create_command(exe); command.args(&self.args); From 23a60cf8c083eaab3a98cc156981101fc88997b0 Mon Sep 17 00:00:00 2001 From: Chawye Hsu Date: Thu, 23 May 2024 13:40:27 +0800 Subject: [PATCH 04/19] propagate on_failure Signed-off-by: Chawye Hsu --- crates/volta-core/src/run/executor.rs | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/crates/volta-core/src/run/executor.rs b/crates/volta-core/src/run/executor.rs index 7b021998b..6e103bd9b 100644 --- a/crates/volta-core/src/run/executor.rs +++ b/crates/volta-core/src/run/executor.rs @@ -207,12 +207,17 @@ impl ToolCommand { // see: https://github.com/rust-lang/rust/issues/37519 #[cfg(windows)] { - let mut abs_paths = which::which_in_global(self.exe.clone(), Some(&path)) - .with_context(|| ErrorKind::BinaryExecError)?; + let name = self.exe.clone(); + let mut abs_paths = + which::which_in_global(&name, Some(&path)).with_context(|| { + ErrorKind::BinaryNotFound { + name: name.to_string_lossy().to_string(), + } + })?; if let Some(abs_exe) = abs_paths.next() { abs_exe.into_os_string() } else { - return Err(ErrorKind::BinaryExecError.into()); + return Err(on_failure.into()); } } }; From 346d638afef4d179da5750fbb1e7d6bdcd406496 Mon Sep 17 00:00:00 2001 From: Chawye Hsu Date: Thu, 23 May 2024 13:58:41 +0800 Subject: [PATCH 05/19] unified VOLTA_BYPASS test Signed-off-by: Chawye Hsu --- tests/acceptance/volta_bypass.rs | 9 --------- 1 file changed, 9 deletions(-) diff --git a/tests/acceptance/volta_bypass.rs b/tests/acceptance/volta_bypass.rs index 6cba3d10c..e2ab0b9c7 100644 --- a/tests/acceptance/volta_bypass.rs +++ b/tests/acceptance/volta_bypass.rs @@ -15,19 +15,10 @@ fn shim_skips_platform_checks_on_bypass() { ) .build(); - #[cfg(unix)] assert_that!( s.process(shim_exe()), execs() .with_status(ExitCode::ExecutionFailure as i32) .with_stderr_contains("VOLTA_BYPASS is enabled[..]") ); - - #[cfg(windows)] - assert_that!( - s.process(shim_exe()), - execs() - .with_status(ExitCode::UnknownError as i32) - .with_stderr_contains("[..]is not recognized as an internal or external command[..]") - ); } From 7b65151824274fc4fefa653946a0465b286fa991 Mon Sep 17 00:00:00 2001 From: Chawye Hsu Date: Thu, 23 May 2024 15:17:52 +0800 Subject: [PATCH 06/19] no args clone Signed-off-by: Chawye Hsu --- crates/volta-core/src/run/executor.rs | 45 +++++++++++++-------------- 1 file changed, 21 insertions(+), 24 deletions(-) diff --git a/crates/volta-core/src/run/executor.rs b/crates/volta-core/src/run/executor.rs index 6e103bd9b..bc84e1bc2 100644 --- a/crates/volta-core/src/run/executor.rs +++ b/crates/volta-core/src/run/executor.rs @@ -1,5 +1,5 @@ use std::collections::HashMap; -use std::ffi::{OsStr, OsString}; +use std::ffi::OsStr; #[cfg(unix)] use std::os::unix::process::ExitStatusExt; #[cfg(windows)] @@ -113,9 +113,7 @@ impl From> for Executor { /// Tracks the Platform as well as what kind of tool is being executed, to allow individual tools /// to customize the behavior before execution. pub struct ToolCommand { - exe: OsString, - args: Vec, - envs: HashMap, + command: Command, platform: Option, kind: ToolKind, } @@ -139,13 +137,11 @@ impl ToolCommand { A: IntoIterator, S: AsRef, { - let exe = exe.as_ref().into(); - let args = args.into_iter().map(|s| s.as_ref().into()).collect(); + let mut command = create_command(exe); + command.args(args); Self { - exe, - args, - envs: HashMap::new(), + command, platform, kind, } @@ -158,9 +154,7 @@ impl ToolCommand { K: AsRef, V: AsRef, { - for (key, value) in envs { - self.env(key, value); - } + self.command.envs(envs); } /// Adds or updates a single environment variable that the command will use @@ -169,7 +163,7 @@ impl ToolCommand { K: AsRef, V: AsRef, { - self.envs.insert(key.as_ref().into(), value.as_ref().into()); + self.command.env(key, value); } /// Updates the Platform for the command to include values from the command-line @@ -197,39 +191,42 @@ impl ToolCommand { ToolKind::Bypass(command) => (System::path()?, ErrorKind::BypassError { command }), }; - let exe = { + let mut cmd = { #[cfg(not(windows))] { - self.exe + self.command } + // On Windows, we need to explicitly use an absolute path to the executable, // otherwise the executable will not be located properly, even if we've set the PATH. // see: https://github.com/rust-lang/rust/issues/37519 #[cfg(windows)] { - let name = self.exe.clone(); + let args = self.command.get_args().collect::>(); + // cmd /c [...other] + // args_idx 0 1 2.. + let name = args.get(1).expect("should have the name"); let mut abs_paths = - which::which_in_global(&name, Some(&path)).with_context(|| { + which::which_in_global(name, Some(&path)).with_context(|| { ErrorKind::BinaryNotFound { name: name.to_string_lossy().to_string(), } })?; if let Some(abs_exe) = abs_paths.next() { - abs_exe.into_os_string() + let mut new_command = create_command(abs_exe); + new_command.args(&args[2..]); + new_command } else { return Err(on_failure.into()); } } }; - let mut command = create_command(exe); - command.args(&self.args); - command.envs(&self.envs); - command.env(RECURSION_ENV_VAR, "1"); - command.env("PATH", path); + cmd.env(RECURSION_ENV_VAR, "1"); + cmd.env("PATH", path); pass_control_to_shim(); - command.status().with_context(|| on_failure) + cmd.status().with_context(|| on_failure) } } From 9aa40f05ea2ad2a961661bae0d8c5c344e875cef Mon Sep 17 00:00:00 2001 From: Chawye Hsu Date: Fri, 24 May 2024 14:22:25 +0800 Subject: [PATCH 07/19] updated implementation Signed-off-by: Chawye Hsu --- crates/volta-core/src/command.rs | 49 +++++++++++ crates/volta-core/src/run/executor.rs | 82 ++++++------------- crates/volta-core/src/tool/package/install.rs | 7 +- 3 files changed, 80 insertions(+), 58 deletions(-) diff --git a/crates/volta-core/src/command.rs b/crates/volta-core/src/command.rs index ece081278..68fd3f1b6 100644 --- a/crates/volta-core/src/command.rs +++ b/crates/volta-core/src/command.rs @@ -3,6 +3,8 @@ use std::process::Command; use cfg_if::cfg_if; +use crate::error::{ErrorKind, Fallible}; + cfg_if! { if #[cfg(windows)] { pub fn create_command(exe: E) -> Command @@ -28,3 +30,50 @@ cfg_if! { } } } + +/// Rebuild command against given PATH +/// +/// On Windows, we need to explicitly use an absolute path to the executable, +/// otherwise the executable will not be located properly, even if we've set the PATH. +/// see: https://github.com/rust-lang/rust/issues/37519 +/// +/// This function will try to find the executable in the given path and rebuild +/// the command with the absolute path to the executable. +pub fn rebuild_command>(command: Command, path: S) -> Fallible { + #[cfg(not(windows))] + { + let mut command = command; + command.env("PATH", path.as_ref()); + Ok(command) + } + + #[cfg(windows)] + { + let args = command.get_args().collect::>(); + // cmd /c [...other] + // args_idx 0 1 2.. + let name = args.get(1).expect("should have the name"); + + if let Ok(mut paths) = which::which_in_global(name, Some(&path)) { + if let Some(exe) = paths.next() { + let mut new_command = create_command(exe); + let envs = command + .get_envs() + .filter(|(_, v)| v.is_some()) + .map(|(k, v)| (k, v.unwrap())) + .collect::>(); + + new_command.args(&args[2..]); + new_command.envs(envs); + new_command.env("PATH", path.as_ref()); + + return Ok(new_command); + } + } + + Err(ErrorKind::BinaryNotFound { + name: name.to_string_lossy().to_string(), + } + .into()) + } +} diff --git a/crates/volta-core/src/run/executor.rs b/crates/volta-core/src/run/executor.rs index bc84e1bc2..bde093e61 100644 --- a/crates/volta-core/src/run/executor.rs +++ b/crates/volta-core/src/run/executor.rs @@ -7,7 +7,7 @@ use std::os::windows::process::ExitStatusExt; use std::process::{Command, ExitStatus}; use super::RECURSION_ENV_VAR; -use crate::command::create_command; +use crate::command::{create_command, rebuild_command}; use crate::error::{Context, ErrorKind, Fallible}; use crate::layout::volta_home; use crate::platform::{CliPlatform, Platform, System}; @@ -191,42 +191,13 @@ impl ToolCommand { ToolKind::Bypass(command) => (System::path()?, ErrorKind::BypassError { command }), }; - let mut cmd = { - #[cfg(not(windows))] - { - self.command - } - - // On Windows, we need to explicitly use an absolute path to the executable, - // otherwise the executable will not be located properly, even if we've set the PATH. - // see: https://github.com/rust-lang/rust/issues/37519 - #[cfg(windows)] - { - let args = self.command.get_args().collect::>(); - // cmd /c [...other] - // args_idx 0 1 2.. - let name = args.get(1).expect("should have the name"); - let mut abs_paths = - which::which_in_global(name, Some(&path)).with_context(|| { - ErrorKind::BinaryNotFound { - name: name.to_string_lossy().to_string(), - } - })?; - if let Some(abs_exe) = abs_paths.next() { - let mut new_command = create_command(abs_exe); - new_command.args(&args[2..]); - new_command - } else { - return Err(on_failure.into()); - } - } - }; - - cmd.env(RECURSION_ENV_VAR, "1"); - cmd.env("PATH", path); - - pass_control_to_shim(); - cmd.status().with_context(|| on_failure) + rebuild_command(self.command, path) + .and_then(|mut cmd| { + cmd.env(RECURSION_ENV_VAR, "1"); + pass_control_to_shim(); + cmd.status().with_context(|| ErrorKind::BinaryExecError) + }) + .with_context(|| on_failure) } } @@ -305,17 +276,17 @@ impl PackageInstallCommand { /// Runs the install command, applying the necessary modifications to install into the Volta /// data directory - pub fn execute(mut self, session: &mut Session) -> Fallible { + pub fn execute(self, session: &mut Session) -> Fallible { let _lock = VoltaLock::acquire(); let image = self.platform.checkout(session)?; let path = image.path()?; - self.command.env(RECURSION_ENV_VAR, "1"); - self.command.env("PATH", path); - self.installer.setup_command(&mut self.command); + let mut command = rebuild_command(self.command, path)?; + + command.env(RECURSION_ENV_VAR, "1"); + self.installer.setup_command(&mut command); - let status = self - .command + let status = command .status() .with_context(|| ErrorKind::BinaryExecError)?; @@ -381,20 +352,19 @@ impl PackageLinkCommand { /// directory. /// /// This will also check for some common failure cases and alert the user - pub fn execute(mut self, session: &mut Session) -> Fallible { + pub fn execute(self, session: &mut Session) -> Fallible { self.check_linked_package(session)?; let image = self.platform.checkout(session)?; let path = image.path()?; - self.command.env(RECURSION_ENV_VAR, "1"); - self.command.env("PATH", path); + let mut command = rebuild_command(self.command, path)?; + + command.env(RECURSION_ENV_VAR, "1"); let package_root = volta_home()?.package_image_dir(&self.tool); - PackageManager::Npm.setup_global_command(&mut self.command, package_root); + PackageManager::Npm.setup_global_command(&mut command, package_root); - self.command - .status() - .with_context(|| ErrorKind::BinaryExecError) + command.status().with_context(|| ErrorKind::BinaryExecError) } /// Check for possible failure cases with the linked package: @@ -496,19 +466,19 @@ impl PackageUpgradeCommand { /// /// Will also check for common failure cases, such as non-existant package or wrong package /// manager - pub fn execute(mut self, session: &mut Session) -> Fallible { + pub fn execute(self, session: &mut Session) -> Fallible { self.upgrader.check_upgraded_package()?; let _lock = VoltaLock::acquire(); let image = self.platform.checkout(session)?; let path = image.path()?; - self.command.env(RECURSION_ENV_VAR, "1"); - self.command.env("PATH", path); - self.upgrader.setup_command(&mut self.command); + let mut command = rebuild_command(self.command, path)?; + + command.env(RECURSION_ENV_VAR, "1"); + self.upgrader.setup_command(&mut command); - let status = self - .command + let status = command .status() .with_context(|| ErrorKind::BinaryExecError)?; diff --git a/crates/volta-core/src/tool/package/install.rs b/crates/volta-core/src/tool/package/install.rs index 4416e0e34..eaa69e388 100644 --- a/crates/volta-core/src/tool/package/install.rs +++ b/crates/volta-core/src/tool/package/install.rs @@ -1,7 +1,7 @@ use std::path::PathBuf; use super::manager::PackageManager; -use crate::command::create_command; +use crate::command::{create_command, rebuild_command}; use crate::error::{Context, ErrorKind, Fallible}; use crate::platform::Image; use crate::style::progress_spinner; @@ -17,6 +17,7 @@ pub(super) fn run_global_install( staging_dir: PathBuf, platform_image: &Image, ) -> Fallible<()> { + let path = platform_image.path()?; let mut command = create_command("npm"); command.args([ "install", @@ -26,7 +27,9 @@ pub(super) fn run_global_install( "--no-audit", ]); command.arg(&package); - command.env("PATH", platform_image.path()?); + + command = rebuild_command(command, path)?; + PackageManager::Npm.setup_global_command(&mut command, staging_dir); debug!("Installing {} with command: {:?}", package, command); From e6ba4e58b8fd9c3529fd6968f971580890724926 Mon Sep 17 00:00:00 2001 From: Chawye Hsu Date: Fri, 24 May 2024 14:25:11 +0800 Subject: [PATCH 08/19] clippy fix Signed-off-by: Chawye Hsu --- crates/volta-core/src/command.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/volta-core/src/command.rs b/crates/volta-core/src/command.rs index 68fd3f1b6..70b8fe618 100644 --- a/crates/volta-core/src/command.rs +++ b/crates/volta-core/src/command.rs @@ -3,7 +3,7 @@ use std::process::Command; use cfg_if::cfg_if; -use crate::error::{ErrorKind, Fallible}; +use crate::error::Fallible; cfg_if! { if #[cfg(windows)] { @@ -71,7 +71,7 @@ pub fn rebuild_command>(command: Command, path: S) -> Fallible Date: Fri, 24 May 2024 15:27:52 +0800 Subject: [PATCH 09/19] debug log PATH Signed-off-by: Chawye Hsu --- crates/volta-core/src/command.rs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/crates/volta-core/src/command.rs b/crates/volta-core/src/command.rs index 70b8fe618..00a282700 100644 --- a/crates/volta-core/src/command.rs +++ b/crates/volta-core/src/command.rs @@ -2,6 +2,7 @@ use std::ffi::OsStr; use std::process::Command; use cfg_if::cfg_if; +use log::debug; use crate::error::Fallible; @@ -40,6 +41,8 @@ cfg_if! { /// This function will try to find the executable in the given path and rebuild /// the command with the absolute path to the executable. pub fn rebuild_command>(command: Command, path: S) -> Fallible { + debug!("PATH: {}", path.as_ref().to_string_lossy()); + #[cfg(not(windows))] { let mut command = command; From 4aff127ef4fc1f98ab9dff8863e458c89015ec3e Mon Sep 17 00:00:00 2001 From: Chawye Hsu Date: Fri, 24 May 2024 16:03:29 +0800 Subject: [PATCH 10/19] add recursion limit Signed-off-by: Chawye Hsu --- crates/volta-core/src/error/kind.rs | 8 ++++++++ crates/volta-core/src/run/executor.rs | 17 +++++++++++++---- crates/volta-core/src/run/mod.rs | 1 + 3 files changed, 22 insertions(+), 4 deletions(-) diff --git a/crates/volta-core/src/error/kind.rs b/crates/volta-core/src/error/kind.rs index 37e28d9f5..efd2f40b8 100644 --- a/crates/volta-core/src/error/kind.rs +++ b/crates/volta-core/src/error/kind.rs @@ -408,6 +408,9 @@ pub enum ErrorKind { file: PathBuf, }, + /// Throw when recursion limit is reached + RecursionLimit, + /// Thrown when unable to read the user Path environment variable from the registry #[cfg(windows)] ReadUserPathError, @@ -1246,6 +1249,10 @@ from {} file.display(), PERMISSIONS_CTA ), + ErrorKind::RecursionLimit => write!( + f, + "Recursive call limit reached." + ), #[cfg(windows)] ErrorKind::ReadUserPathError => write!( f, @@ -1549,6 +1556,7 @@ impl ErrorKind { ErrorKind::ReadNpmManifestError => ExitCode::UnknownError, ErrorKind::ReadPackageConfigError { .. } => ExitCode::FileSystemError, ErrorKind::ReadPlatformError { .. } => ExitCode::FileSystemError, + ErrorKind::RecursionLimit => ExitCode::ExecutionFailure, #[cfg(windows)] ErrorKind::ReadUserPathError => ExitCode::EnvironmentError, ErrorKind::RegistryFetchError { .. } => ExitCode::NetworkError, diff --git a/crates/volta-core/src/run/executor.rs b/crates/volta-core/src/run/executor.rs index bde093e61..1b2d1b4de 100644 --- a/crates/volta-core/src/run/executor.rs +++ b/crates/volta-core/src/run/executor.rs @@ -6,7 +6,7 @@ use std::os::unix::process::ExitStatusExt; use std::os::windows::process::ExitStatusExt; use std::process::{Command, ExitStatus}; -use super::RECURSION_ENV_VAR; +use super::{RECURSION_ENV_VAR, RECURSION_LIMIT}; use crate::command::{create_command, rebuild_command}; use crate::error::{Context, ErrorKind, Fallible}; use crate::layout::volta_home; @@ -191,11 +191,20 @@ impl ToolCommand { ToolKind::Bypass(command) => (System::path()?, ErrorKind::BypassError { command }), }; + // Recursive call limit + let mut recursion = std::env::var(RECURSION_ENV_VAR) + .map(|var| var.parse::().unwrap_or(1u8)) + .unwrap_or(1u8); + if recursion > RECURSION_LIMIT { + return Err(ErrorKind::RecursionLimit.into()); + } + recursion += 1; + rebuild_command(self.command, path) - .and_then(|mut cmd| { - cmd.env(RECURSION_ENV_VAR, "1"); + .and_then(|mut command| { + command.env(RECURSION_ENV_VAR, recursion.to_string()); pass_control_to_shim(); - cmd.status().with_context(|| ErrorKind::BinaryExecError) + command.status().with_context(|| ErrorKind::BinaryExecError) }) .with_context(|| on_failure) } diff --git a/crates/volta-core/src/run/mod.rs b/crates/volta-core/src/run/mod.rs index 89278eba7..bf371b1f6 100644 --- a/crates/volta-core/src/run/mod.rs +++ b/crates/volta-core/src/run/mod.rs @@ -32,6 +32,7 @@ mod yarn; /// Note: This is explicitly _removed_ when calling a command through `volta run`, as that will /// never happen due to the Volta environment. const RECURSION_ENV_VAR: &str = "_VOLTA_TOOL_RECURSION"; +const RECURSION_LIMIT: u8 = 10; const VOLTA_BYPASS: &str = "VOLTA_BYPASS"; /// Execute a shim command, based on the command-line arguments to the current process From f9cdf57549f803b6a14a0af1b1d296e44c68bb02 Mon Sep 17 00:00:00 2001 From: Chawye Hsu Date: Thu, 1 Aug 2024 22:16:12 +0800 Subject: [PATCH 11/19] typo Co-authored-by: Chris Krycho --- crates/volta-core/src/error/kind.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/volta-core/src/error/kind.rs b/crates/volta-core/src/error/kind.rs index cf86b6bdc..9f71f9d5f 100644 --- a/crates/volta-core/src/error/kind.rs +++ b/crates/volta-core/src/error/kind.rs @@ -408,7 +408,7 @@ pub enum ErrorKind { file: PathBuf, }, - /// Throw when recursion limit is reached + /// Thrown when recursion limit is reached RecursionLimit, /// Thrown when unable to read the user Path environment variable from the registry From 6dacd20ec14944b5ce421b7dae15a1534d3a8b7c Mon Sep 17 00:00:00 2001 From: Chawye Hsu Date: Thu, 1 Aug 2024 22:36:08 +0800 Subject: [PATCH 12/19] Update crates/volta-core/src/command.rs Co-authored-by: Chris Krycho --- crates/volta-core/src/command.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/volta-core/src/command.rs b/crates/volta-core/src/command.rs index 00a282700..d71c3052c 100644 --- a/crates/volta-core/src/command.rs +++ b/crates/volta-core/src/command.rs @@ -55,7 +55,7 @@ pub fn rebuild_command>(command: Command, path: S) -> Fallible>(); // cmd /c [...other] // args_idx 0 1 2.. - let name = args.get(1).expect("should have the name"); + let name = args.get(1).expect("A command always has a name"); if let Ok(mut paths) = which::which_in_global(name, Some(&path)) { if let Some(exe) = paths.next() { From 1c1dea838e4212fb8f426891cf4a84054ee27839 Mon Sep 17 00:00:00 2001 From: Chawye Hsu Date: Thu, 1 Aug 2024 22:36:54 +0800 Subject: [PATCH 13/19] Update crates/volta-core/src/command.rs Co-authored-by: Chris Krycho --- crates/volta-core/src/command.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/crates/volta-core/src/command.rs b/crates/volta-core/src/command.rs index d71c3052c..5efca5c7e 100644 --- a/crates/volta-core/src/command.rs +++ b/crates/volta-core/src/command.rs @@ -66,6 +66,7 @@ pub fn rebuild_command>(command: Command, path: S) -> Fallible>(); + // The args will be the command name and any additional args. new_command.args(&args[2..]); new_command.envs(envs); new_command.env("PATH", path.as_ref()); From 63e4626c9a3af986a72b2130a918e3a017008242 Mon Sep 17 00:00:00 2001 From: Chawye Hsu Date: Thu, 1 Aug 2024 22:33:18 +0800 Subject: [PATCH 14/19] Add `ErrorKind::ParseRecursionEnvError` Signed-off-by: Chawye Hsu --- crates/volta-core/src/error/kind.rs | 8 ++++++++ crates/volta-core/src/run/executor.rs | 14 ++++++++------ 2 files changed, 16 insertions(+), 6 deletions(-) diff --git a/crates/volta-core/src/error/kind.rs b/crates/volta-core/src/error/kind.rs index 9f71f9d5f..df736cee4 100644 --- a/crates/volta-core/src/error/kind.rs +++ b/crates/volta-core/src/error/kind.rs @@ -329,6 +329,9 @@ pub enum ErrorKind { /// Thrown when unable to parse the platform.json file ParsePlatformError, + /// Thrown when unable to parse the RECURSION_ENV_VAR value + ParseRecursionEnvError, + /// Thrown when unable to parse a tool spec (`[@]`) ParseToolSpecError { tool_spec: String, @@ -1117,6 +1120,10 @@ Please ensure the version of Node is correct." {}", REPORT_BUG_CTA ), + ErrorKind::ParseRecursionEnvError => write!( + f, + "Could not parse RECURSION_ENV_VAR value." + ), ErrorKind::ParseToolSpecError { tool_spec } => write!( f, "Could not parse tool spec `{}` @@ -1540,6 +1547,7 @@ impl ErrorKind { ErrorKind::ParseNpmManifestError => ExitCode::UnknownError, ErrorKind::ParsePackageConfigError => ExitCode::UnknownError, ErrorKind::ParsePlatformError => ExitCode::ConfigurationError, + ErrorKind::ParseRecursionEnvError => ExitCode::UnknownError, ErrorKind::PersistInventoryError { .. } => ExitCode::FileSystemError, ErrorKind::PnpmVersionNotFound { .. } => ExitCode::NoVersionMatch, ErrorKind::ProjectLocalBinaryExecError { .. } => ExitCode::ExecutionFailure, diff --git a/crates/volta-core/src/run/executor.rs b/crates/volta-core/src/run/executor.rs index 1b2d1b4de..da22552b7 100644 --- a/crates/volta-core/src/run/executor.rs +++ b/crates/volta-core/src/run/executor.rs @@ -191,18 +191,20 @@ impl ToolCommand { ToolKind::Bypass(command) => (System::path()?, ErrorKind::BypassError { command }), }; - // Recursive call limit - let mut recursion = std::env::var(RECURSION_ENV_VAR) - .map(|var| var.parse::().unwrap_or(1u8)) - .unwrap_or(1u8); + // Do recursive call limit check + let recursion = match std::env::var(RECURSION_ENV_VAR) { + Err(_) => 1u8, + Ok(var) => var + .parse::() + .with_context(|| ErrorKind::ParseRecursionEnvError)?, + }; if recursion > RECURSION_LIMIT { return Err(ErrorKind::RecursionLimit.into()); } - recursion += 1; rebuild_command(self.command, path) .and_then(|mut command| { - command.env(RECURSION_ENV_VAR, recursion.to_string()); + command.env(RECURSION_ENV_VAR, (recursion + 1).to_string()); pass_control_to_shim(); command.status().with_context(|| ErrorKind::BinaryExecError) }) From bcbc200f793ed8b17eaffa2b4f429f14b48d2d3f Mon Sep 17 00:00:00 2001 From: Chawye Hsu Date: Thu, 1 Aug 2024 22:42:20 +0800 Subject: [PATCH 15/19] code style tweak Signed-off-by: Chawye Hsu --- crates/volta-core/src/command.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/crates/volta-core/src/command.rs b/crates/volta-core/src/command.rs index 5efca5c7e..6f1c76029 100644 --- a/crates/volta-core/src/command.rs +++ b/crates/volta-core/src/command.rs @@ -62,8 +62,7 @@ pub fn rebuild_command>(command: Command, path: S) -> Fallible>(); // The args will be the command name and any additional args. From 54229320cd5ce0fa01e72814cad23dda1738687a Mon Sep 17 00:00:00 2001 From: Chawye Hsu Date: Thu, 1 Aug 2024 22:47:16 +0800 Subject: [PATCH 16/19] rename from `rerbuild_command` to `command_on_path` Signed-off-by: Chawye Hsu --- crates/volta-core/src/command.rs | 63 +++++++++---------- crates/volta-core/src/run/executor.rs | 10 +-- crates/volta-core/src/tool/package/install.rs | 4 +- 3 files changed, 38 insertions(+), 39 deletions(-) diff --git a/crates/volta-core/src/command.rs b/crates/volta-core/src/command.rs index 6f1c76029..efba03187 100644 --- a/crates/volta-core/src/command.rs +++ b/crates/volta-core/src/command.rs @@ -32,6 +32,15 @@ cfg_if! { } } +/// Rebuild command against given PATH +#[cfg(unix)] +fn command_on_path>(command: Command, path: S) -> Fallible { + debug!("PATH: {}", path.as_ref().to_string_lossy()); + let mut command = command; + command.env("PATH", path.as_ref()); + Ok(command) +} + /// Rebuild command against given PATH /// /// On Windows, we need to explicitly use an absolute path to the executable, @@ -40,43 +49,33 @@ cfg_if! { /// /// This function will try to find the executable in the given path and rebuild /// the command with the absolute path to the executable. -pub fn rebuild_command>(command: Command, path: S) -> Fallible { +#[cfg(windows)] +pub fn command_on_path>(command: Command, path: S) -> Fallible { debug!("PATH: {}", path.as_ref().to_string_lossy()); + let args = command.get_args().collect::>(); + // cmd /c [...other] + // args_idx 0 1 2.. + let name = args.get(1).expect("A command always has a name"); - #[cfg(not(windows))] - { - let mut command = command; - command.env("PATH", path.as_ref()); - Ok(command) - } + if let Ok(mut paths) = which::which_in_global(name, Some(&path)) { + if let Some(exe) = paths.next() { + let mut new_command = create_command(exe); + let envs = command + .get_envs() + .filter_map(|(k, maybe_v)| Some(k).zip(maybe_v)) + .collect::>(); - #[cfg(windows)] - { - let args = command.get_args().collect::>(); - // cmd /c [...other] - // args_idx 0 1 2.. - let name = args.get(1).expect("A command always has a name"); + // The args will be the command name and any additional args. + new_command.args(&args[2..]); + new_command.envs(envs); + new_command.env("PATH", path.as_ref()); - if let Ok(mut paths) = which::which_in_global(name, Some(&path)) { - if let Some(exe) = paths.next() { - let mut new_command = create_command(exe); - let envs = command - .get_envs() - .filter_map(|(k, maybe_v)| Some(k).zip(maybe_v)) - .collect::>(); - - // The args will be the command name and any additional args. - new_command.args(&args[2..]); - new_command.envs(envs); - new_command.env("PATH", path.as_ref()); - - return Ok(new_command); - } + return Ok(new_command); } + } - Err(crate::error::ErrorKind::BinaryNotFound { - name: name.to_string_lossy().to_string(), - } - .into()) + Err(crate::error::ErrorKind::BinaryNotFound { + name: name.to_string_lossy().to_string(), } + .into()) } diff --git a/crates/volta-core/src/run/executor.rs b/crates/volta-core/src/run/executor.rs index da22552b7..eeb13396d 100644 --- a/crates/volta-core/src/run/executor.rs +++ b/crates/volta-core/src/run/executor.rs @@ -7,7 +7,7 @@ use std::os::windows::process::ExitStatusExt; use std::process::{Command, ExitStatus}; use super::{RECURSION_ENV_VAR, RECURSION_LIMIT}; -use crate::command::{create_command, rebuild_command}; +use crate::command::{command_on_path, create_command}; use crate::error::{Context, ErrorKind, Fallible}; use crate::layout::volta_home; use crate::platform::{CliPlatform, Platform, System}; @@ -202,7 +202,7 @@ impl ToolCommand { return Err(ErrorKind::RecursionLimit.into()); } - rebuild_command(self.command, path) + command_on_path(self.command, path) .and_then(|mut command| { command.env(RECURSION_ENV_VAR, (recursion + 1).to_string()); pass_control_to_shim(); @@ -292,7 +292,7 @@ impl PackageInstallCommand { let image = self.platform.checkout(session)?; let path = image.path()?; - let mut command = rebuild_command(self.command, path)?; + let mut command = command_on_path(self.command, path)?; command.env(RECURSION_ENV_VAR, "1"); self.installer.setup_command(&mut command); @@ -369,7 +369,7 @@ impl PackageLinkCommand { let image = self.platform.checkout(session)?; let path = image.path()?; - let mut command = rebuild_command(self.command, path)?; + let mut command = command_on_path(self.command, path)?; command.env(RECURSION_ENV_VAR, "1"); let package_root = volta_home()?.package_image_dir(&self.tool); @@ -484,7 +484,7 @@ impl PackageUpgradeCommand { let image = self.platform.checkout(session)?; let path = image.path()?; - let mut command = rebuild_command(self.command, path)?; + let mut command = command_on_path(self.command, path)?; command.env(RECURSION_ENV_VAR, "1"); self.upgrader.setup_command(&mut command); diff --git a/crates/volta-core/src/tool/package/install.rs b/crates/volta-core/src/tool/package/install.rs index eaa69e388..13c53d35f 100644 --- a/crates/volta-core/src/tool/package/install.rs +++ b/crates/volta-core/src/tool/package/install.rs @@ -1,7 +1,7 @@ use std::path::PathBuf; use super::manager::PackageManager; -use crate::command::{create_command, rebuild_command}; +use crate::command::{create_command, command_on_path}; use crate::error::{Context, ErrorKind, Fallible}; use crate::platform::Image; use crate::style::progress_spinner; @@ -28,7 +28,7 @@ pub(super) fn run_global_install( ]); command.arg(&package); - command = rebuild_command(command, path)?; + command = command_on_path(command, path)?; PackageManager::Npm.setup_global_command(&mut command, staging_dir); From b355dfa0b4d63f1bbb3ef2ce950ad6c139ecfa5a Mon Sep 17 00:00:00 2001 From: Chawye Hsu Date: Thu, 1 Aug 2024 22:49:34 +0800 Subject: [PATCH 17/19] fix visibility Signed-off-by: Chawye Hsu --- crates/volta-core/src/command.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/volta-core/src/command.rs b/crates/volta-core/src/command.rs index efba03187..0852f36ad 100644 --- a/crates/volta-core/src/command.rs +++ b/crates/volta-core/src/command.rs @@ -34,7 +34,7 @@ cfg_if! { /// Rebuild command against given PATH #[cfg(unix)] -fn command_on_path>(command: Command, path: S) -> Fallible { +pub fn command_on_path>(command: Command, path: S) -> Fallible { debug!("PATH: {}", path.as_ref().to_string_lossy()); let mut command = command; command.env("PATH", path.as_ref()); From 196c0f272309ac76c5f247fd10b8ca1542efac2d Mon Sep 17 00:00:00 2001 From: Chawye Hsu Date: Thu, 1 Aug 2024 22:56:20 +0800 Subject: [PATCH 18/19] style: cargo fmt Signed-off-by: Chawye Hsu --- crates/volta-core/src/tool/package/install.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/volta-core/src/tool/package/install.rs b/crates/volta-core/src/tool/package/install.rs index 13c53d35f..de2ba16fc 100644 --- a/crates/volta-core/src/tool/package/install.rs +++ b/crates/volta-core/src/tool/package/install.rs @@ -1,7 +1,7 @@ use std::path::PathBuf; use super::manager::PackageManager; -use crate::command::{create_command, command_on_path}; +use crate::command::{command_on_path, create_command}; use crate::error::{Context, ErrorKind, Fallible}; use crate::platform::Image; use crate::style::progress_spinner; From 32641226899c463b4ef95a80e11d624f288fba5c Mon Sep 17 00:00:00 2001 From: Chawye Hsu Date: Thu, 1 Aug 2024 23:39:25 +0800 Subject: [PATCH 19/19] style: use `let...else` Signed-off-by: Chawye Hsu --- crates/volta-core/src/command.rs | 25 +++++++++++++++---------- 1 file changed, 15 insertions(+), 10 deletions(-) diff --git a/crates/volta-core/src/command.rs b/crates/volta-core/src/command.rs index 0852f36ad..780604391 100644 --- a/crates/volta-core/src/command.rs +++ b/crates/volta-core/src/command.rs @@ -57,8 +57,18 @@ pub fn command_on_path>(command: Command, path: S) -> Fallible>(command: Command, path: S) -> Fallible