-
Notifications
You must be signed in to change notification settings - Fork 283
fix: Prevent fork bomb on Windows #1761
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
base: main
Are you sure you want to change the base?
Changes from 11 commits
0c8b573
d163d5d
a26bd63
23a60cf
346d638
7b65151
9aa40f0
e6ba4e5
524bb72
4aff127
d38e7ef
f9cdf57
6dacd20
1c1dea8
63e4626
bcbc200
5422932
b355dfa
196c0f2
3264122
764bcd5
37a9b2d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,6 +2,9 @@ use std::ffi::OsStr; | |
use std::process::Command; | ||
|
||
use cfg_if::cfg_if; | ||
use log::debug; | ||
|
||
use crate::error::Fallible; | ||
|
||
cfg_if! { | ||
if #[cfg(windows)] { | ||
|
@@ -28,3 +31,52 @@ 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<S: AsRef<OsStr>>(command: Command, path: S) -> Fallible<Command> { | ||
debug!("PATH: {}", path.as_ref().to_string_lossy()); | ||
|
||
#[cfg(not(windows))] | ||
{ | ||
let mut command = command; | ||
command.env("PATH", path.as_ref()); | ||
Ok(command) | ||
} | ||
|
||
#[cfg(windows)] | ||
{ | ||
let args = command.get_args().collect::<Vec<_>>(); | ||
// cmd /c <name> [...other] | ||
// args_idx 0 1 2.. | ||
let name = args.get(1).expect("should have the name"); | ||
chawyehsu marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
if let Ok(mut paths) = which::which_in_global(name, Some(&path)) { | ||
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. Given that the trigger for this (as I understand it) is the It's relatively minor, but the |
||
if let Some(exe) = paths.next() { | ||
chawyehsu marked this conversation as resolved.
Show resolved
Hide resolved
|
||
let mut new_command = create_command(exe); | ||
let envs = command | ||
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.
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. Agreed on adding a comment! I believe this needs to filter because the output of for (key, maybe_value) in command.get_envs() {
match maybe_value {
Some(value) => command.env(key, value),
None => command.env_remove(key),
}
} 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.
This was the reason to filter out envs, envs with Do you think it's better to use for (key, maybe_value) in command.get_envs() {
match maybe_value {
Some(value) => command.env(key, value),
None => command.env_remove(key),
}
} to apply envs to the let envs = command
.get_envs()
.filter_map(|(k, maybe_v)| Some(k).zip(maybe_v))
.collect::<Vec<_>>();
new_command.envs(envs); |
||
.get_envs() | ||
.filter(|(_, v)| v.is_some()) | ||
.map(|(k, v)| (k, v.unwrap())) | ||
chawyehsu marked this conversation as resolved.
Show resolved
Hide resolved
|
||
.collect::<Vec<_>>(); | ||
|
||
new_command.args(&args[2..]); | ||
chawyehsu marked this conversation as resolved.
Show resolved
Hide resolved
|
||
new_command.envs(envs); | ||
new_command.env("PATH", path.as_ref()); | ||
|
||
return Ok(new_command); | ||
} | ||
} | ||
|
||
Err(crate::error::ErrorKind::BinaryNotFound { | ||
name: name.to_string_lossy().to_string(), | ||
} | ||
.into()) | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -408,6 +408,9 @@ pub enum ErrorKind { | |
file: PathBuf, | ||
}, | ||
|
||
/// Throw when recursion limit is reached | ||
chawyehsu marked this conversation as resolved.
Show resolved
Hide resolved
|
||
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." | ||
), | ||
Comment on lines
+1259
to
+1262
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. I don't have a great suggestion on the top of my head, but is it impossible to create a call-to-action for this error? We have generally tried to always have a CTA included with any error message, so that we guide users to how to fix the problems they run into. |
||
#[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, | ||
|
Uh oh!
There was an error while loading. Please reload this page.