-
Notifications
You must be signed in to change notification settings - Fork 13.5k
Add Command::resolve_in_parent_path
#142035
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: master
Are you sure you want to change the base?
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 |
---|---|---|
|
@@ -378,27 +378,89 @@ impl Command { | |
callback()?; | ||
} | ||
|
||
let mut _reset = None; | ||
// Although we're performing an exec here we may also return with an | ||
// error from this function (without actually exec'ing) in which case we | ||
// want to be sure to restore the global environment back to what it | ||
// once was, ensuring that our temporary override, when free'd, doesn't | ||
// corrupt our process's environment. | ||
let mut _reset = None; | ||
if let Some(envp) = maybe_envp { | ||
struct Reset(*const *const libc::c_char); | ||
struct Reset(*const *const libc::c_char); | ||
|
||
impl Drop for Reset { | ||
fn drop(&mut self) { | ||
unsafe { | ||
*sys::env::environ() = self.0; | ||
} | ||
impl Drop for Reset { | ||
fn drop(&mut self) { | ||
unsafe { | ||
*sys::env::environ() = self.0; | ||
} | ||
} | ||
} | ||
|
||
if let Some(envp) = maybe_envp | ||
&& self.get_resolve_in_parent_path() | ||
&& self.env_saw_path() | ||
&& self.get_program_kind() == ProgramKind::PathLookup | ||
{ | ||
use crate::ffi::CStr; | ||
// execvpe is a gnu extension... | ||
#[cfg(all(target_os = "linux", target_env = "gnu"))] | ||
unsafe fn exec_with_env( | ||
program: &CStr, | ||
args: &CStringArray, | ||
envp: &CStringArray, | ||
) -> io::Error { | ||
unsafe { libc::execvpe(program.as_ptr(), args.as_ptr(), envp.as_ptr()) }; | ||
io::Error::last_os_error() | ||
} | ||
|
||
// ...so if we're not gnu then use our own implementation. | ||
#[cfg(not(all(target_os = "linux", target_env = "gnu")))] | ||
unsafe fn exec_with_env( | ||
program: &CStr, | ||
args: &CStringArray, | ||
envp: &CStringArray, | ||
) -> io::Error { | ||
unsafe { | ||
let name = program.to_bytes(); | ||
let mut buffer = | ||
[const { mem::MaybeUninit::<u8>::uninit() }; libc::PATH_MAX as usize]; | ||
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 wonder how accurate |
||
let mut environ = *sys::env::environ(); | ||
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 guess we can rely on environ always being defined since otherwise it's a linkage error, yeah, maybe. |
||
// Search the environment for PATH and, if found, | ||
// search the paths for the executable by trying to execve each candidate. | ||
while !(*environ).is_null() { | ||
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.
|
||
let kv = CStr::from_ptr(*environ); | ||
if let Some(value) = kv.to_bytes().strip_prefix(b"PATH=") { | ||
for path in value.split(|&b| b == b':') { | ||
if buffer.len() - 2 >= path.len().saturating_add(name.len()) { | ||
let buf_ptr = buffer.as_mut_ptr().cast::<u8>(); | ||
let mut offset = 0; | ||
if !path.is_empty() { | ||
buf_ptr.copy_from(path.as_ptr(), path.len()); | ||
offset += path.len(); | ||
if path.last() != Some(&b'/') { | ||
*buf_ptr.add(path.len()) = b'/'; | ||
offset += 1; | ||
} | ||
} | ||
buf_ptr.add(offset).copy_from(name.as_ptr(), name.len()); | ||
offset += name.len(); | ||
*buf_ptr.add(offset) = 0; | ||
libc::execve(buf_ptr.cast(), args.as_ptr(), envp.as_ptr()); | ||
} | ||
} | ||
break; | ||
} | ||
environ = environ.add(1); | ||
} | ||
// If execve is successful then it'll never return, | ||
// thus we only reach this point on failure.. | ||
io::Error::from_raw_os_error(libc::ENOENT) | ||
} | ||
} | ||
_reset = Some(Reset(*sys::env::environ())); | ||
return Err(exec_with_env(self.get_program_cstr(), self.get_argv(), envp)); | ||
} else if let Some(envp) = maybe_envp { | ||
Comment on lines
+397
to
+460
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 looks like the actual key part of the diff to review, I guess. |
||
_reset = Some(Reset(*sys::env::environ())); | ||
*sys::env::environ() = envp.as_ptr(); | ||
} | ||
|
||
libc::execvp(self.get_program_cstr().as_ptr(), self.get_argv().as_ptr()); | ||
Err(io::Error::last_os_error()) | ||
} | ||
|
@@ -453,7 +515,9 @@ impl Command { | |
|
||
if self.get_gid().is_some() | ||
|| self.get_uid().is_some() | ||
|| (self.env_saw_path() && !self.program_is_path()) | ||
|| (!self.get_resolve_in_parent_path() | ||
&& self.env_saw_path() | ||
&& !self.program_is_path()) | ||
Comment on lines
+518
to
+520
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. hrm. do we ever examine this bool individually? feels like nah. |
||
|| !self.get_closures().is_empty() | ||
|| self.get_groups().is_some() | ||
|| self.get_chroot().is_some() | ||
|
Original file line number | Diff line number | Diff line change | ||||||
---|---|---|---|---|---|---|---|---|
|
@@ -158,6 +158,7 @@ pub struct Command { | |||||||
startupinfo_fullscreen: bool, | ||||||||
startupinfo_untrusted_source: bool, | ||||||||
startupinfo_force_feedback: Option<bool>, | ||||||||
force_parent_path: bool, | ||||||||
} | ||||||||
|
||||||||
pub enum Stdio { | ||||||||
|
@@ -192,6 +193,7 @@ impl Command { | |||||||
startupinfo_fullscreen: false, | ||||||||
startupinfo_untrusted_source: false, | ||||||||
startupinfo_force_feedback: None, | ||||||||
force_parent_path: false, | ||||||||
} | ||||||||
} | ||||||||
|
||||||||
|
@@ -224,6 +226,10 @@ impl Command { | |||||||
self.force_quotes_enabled = enabled; | ||||||||
} | ||||||||
|
||||||||
pub fn resolve_in_parent_path(&mut self, use_parent: bool) { | ||||||||
self.force_parent_path = use_parent; | ||||||||
} | ||||||||
|
||||||||
pub fn raw_arg(&mut self, command_str_to_append: &OsStr) { | ||||||||
self.args.push(Arg::Raw(command_str_to_append.to_os_string())) | ||||||||
} | ||||||||
|
@@ -274,7 +280,10 @@ impl Command { | |||||||
let env_saw_path = self.env.have_changed_path(); | ||||||||
let maybe_env = self.env.capture_if_changed(); | ||||||||
|
||||||||
let child_paths = if env_saw_path && let Some(env) = maybe_env.as_ref() { | ||||||||
let child_paths = if env_saw_path | ||||||||
&& !self.force_parent_path | ||||||||
&& let Some(env) = maybe_env.as_ref() | ||||||||
{ | ||||||||
Comment on lines
+283
to
+286
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. hm. this means that rust/library/std/src/sys/process/windows.rs Lines 536 to 538 in ee32374
|
||||||||
env.get(&EnvKey::new("PATH")).map(|s| s.as_os_str()) | ||||||||
} else { | ||||||||
None | ||||||||
|
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.
So this was a point without agreement on the libs-api side. E.g. what should happen if
chroot
etc is set so the paths inPATH
essentially point somewhere different. I decided to implement it the easy way for the sake of experimentation and hope that someone will bug me if they think we must do something different and explain their reasons.