Add `Command::resolve_in_parent_path` #142035

ChrisDenton · 2025-06-04T18:35:12Z

This is part of #141976 (ACP: rust-lang/libs-team#591). libs-api could not come to a consensus on what the final API should look like so they've agreed to experiment with a few different approaches.

This PR just implements the resolve_in_parent_path method which forces Command to ignore PATH set on the child for the purposes of resolving the executable. There was no consensus on what should happen if, for example, chroot is set (which can change the meaning of paths) so for now I've just done the easy thing. Figuring out the correct behaviour here will need to be done before this is considered for stabilisation but hopefully having it on nightly will nudge people in to giving their opinions.

rustbot · 2025-06-04T18:35:17Z

r? @workingjubilee

rustbot has assigned @workingjubilee.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

ChrisDenton · 2025-06-04T19:47:32Z

Ah right, execvpe is a gnu extension. Bah.

workingjubilee · 2025-06-04T21:52:22Z

@rustbot author

ChrisDenton · 2025-06-05T13:19:34Z

@bors2 try jobs=aarch64-apple,x86_64-msvc-*,dist-various-*

rust-bors · 2025-06-05T13:19:37Z

⌛ Trying commit 57a631f with merge 5b34fd8…

To cancel the try build, run the command @bors2 try cancel.

Add `Command::resolve_in_parent_path` This is part of #141976 (ACP: rust-lang/libs-team#591). libs-api could not come to a consensus on what the final API should look like so they've agreed to experiment with a few different approaches. This PR just implements the `resolve_in_parent_path` method which forces `Command` to ignore `PATH` set on the child for the purposes of resolving the executable. There was no consensus on what should happen if, for example, `chroot` is set (which can change the meaning of paths) so for now I've just done the easy thing. Figuring out the correct behaviour here will need to be done before this is considered for stabilisation but hopefully having it on nightly will nudge people in to giving their opinions. try-job: `aarch64-apple` try-job: `x86_64-msvc-*` try-job: `dist-various-*`

ChrisDenton · 2025-06-05T13:34:19Z

library/std/src/sys/process/unix/unix.rs

+            unsafe {
+                let name = program.to_bytes();
+                let mut buffer =
+                    [const { mem::MaybeUninit::<u8>::uninit() }; libc::PATH_MAX as usize];


I'm aware that paths can be longer than PATH_MAX but I can't allocate here. I could allocate a buffer before fork but this is an experimental feature so I really want it to be as self-contained as possible so it can be removed or radically changed without too much disruption. ~~Unsized locals would be another alternative...~~

In any case, if this feature ever approaches stabilisation, I can fix it then.

ChrisDenton · 2025-06-05T13:38:16Z

library/std/src/process.rs

+    /// On Unix the process is executed after fork and after setting up the rest of the new environment.
+    /// So if `chroot`, `uid`, `gid` or `groups` are used then the way `PATH` is interpreted may be changed.


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 in PATH 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.

rust-bors · 2025-06-05T14:08:51Z

💔 Test failed

CI ❌

ChrisDenton · 2025-06-05T17:28:40Z

@bors2 try jobs=aarch64-apple,x86_64-msvc-,dist-various-

rust-bors · 2025-06-05T17:28:44Z

⌛ Trying commit c8a279b with merge 3432b11…

To cancel the try build, run the command @bors2 try cancel.

Add `Command::resolve_in_parent_path` This is part of #141976 (ACP: rust-lang/libs-team#591). libs-api could not come to a consensus on what the final API should look like so they've agreed to experiment with a few different approaches. This PR just implements the `resolve_in_parent_path` method which forces `Command` to ignore `PATH` set on the child for the purposes of resolving the executable. There was no consensus on what should happen if, for example, `chroot` is set (which can change the meaning of paths) so for now I've just done the easy thing. Figuring out the correct behaviour here will need to be done before this is considered for stabilisation but hopefully having it on nightly will nudge people in to giving their opinions. try-job: aarch64-apple try-job: x86_64-msvc-* try-job: dist-various-*

rust-bors · 2025-06-05T18:29:32Z

💔 Test failed

CI ❌

ChrisDenton · 2025-06-05T18:37:03Z

@bors2 try jobs=x86_64-msvc-*,dist-various-*

rust-bors · 2025-06-05T18:37:07Z

⌛ Trying commit c8a279b with merge 0f9a1f9…

To cancel the try build, run the command @bors2 try cancel.

Add `Command::resolve_in_parent_path` This is part of #141976 (ACP: rust-lang/libs-team#591). libs-api could not come to a consensus on what the final API should look like so they've agreed to experiment with a few different approaches. This PR just implements the `resolve_in_parent_path` method which forces `Command` to ignore `PATH` set on the child for the purposes of resolving the executable. There was no consensus on what should happen if, for example, `chroot` is set (which can change the meaning of paths) so for now I've just done the easy thing. Figuring out the correct behaviour here will need to be done before this is considered for stabilisation but hopefully having it on nightly will nudge people in to giving their opinions. try-job: `x86_64-msvc-*` try-job: `dist-various-*`

ChrisDenton · 2025-06-05T19:53:44Z

@bors2 try jobs=x86_64-msvc-*,dist-various-*

rust-bors · 2025-06-05T19:53:47Z

❗ A try build is currently in progress. You can cancel it using @bors2 try cancel.

ChrisDenton · 2025-06-05T19:54:06Z

@bors2 try cancel

rust-bors · 2025-06-05T19:54:09Z

Try build cancelled. Cancelled workflows:

https://github.com/rust-lang/rust/actions/runs/15474693782

ChrisDenton · 2025-06-05T19:54:32Z

@bors2 try jobs=x86_64-msvc-*,dist-various-*

rust-bors · 2025-06-05T19:54:39Z

⌛ Trying commit 5a42e16 with merge c6b4698…

To cancel the try build, run the command @bors2 try cancel.

Add `Command::resolve_in_parent_path` This is part of #141976 (ACP: rust-lang/libs-team#591). libs-api could not come to a consensus on what the final API should look like so they've agreed to experiment with a few different approaches. This PR just implements the `resolve_in_parent_path` method which forces `Command` to ignore `PATH` set on the child for the purposes of resolving the executable. There was no consensus on what should happen if, for example, `chroot` is set (which can change the meaning of paths) so for now I've just done the easy thing. Figuring out the correct behaviour here will need to be done before this is considered for stabilisation but hopefully having it on nightly will nudge people in to giving their opinions. try-job: `x86_64-msvc-*` try-job: `dist-various-*`

ChrisDenton · 2025-06-05T19:56:07Z

So it turns out the apple failures (after the first) where all due to a try-job issue and not genuine failures. That's good because I was deeply confused about why it was failing. That's bad because I did waste time trying to figure it out.

rust-bors · 2025-06-05T20:58:18Z

💔 Test failed

CI ❌

ChrisDenton · 2025-06-05T21:07:52Z

@bors2 try jobs=x86_64-msvc-*,dist-various-*

rust-bors · 2025-06-05T21:07:56Z

⌛ Trying commit f892c4c with merge b895bfd…

To cancel the try build, run the command @bors2 try cancel.

Add `Command::resolve_in_parent_path` This is part of #141976 (ACP: rust-lang/libs-team#591). libs-api could not come to a consensus on what the final API should look like so they've agreed to experiment with a few different approaches. This PR just implements the `resolve_in_parent_path` method which forces `Command` to ignore `PATH` set on the child for the purposes of resolving the executable. There was no consensus on what should happen if, for example, `chroot` is set (which can change the meaning of paths) so for now I've just done the easy thing. Figuring out the correct behaviour here will need to be done before this is considered for stabilisation but hopefully having it on nightly will nudge people in to giving their opinions. try-job: `x86_64-msvc-*` try-job: `dist-various-*`

ChrisDenton · 2025-06-05T21:14:21Z

I'm basically just fixing the unimplemented platforms at this point so I think this is ready for review

@rustbot ready

rust-bors · 2025-06-05T23:06:00Z

💔 Test failed

CI ❌

ChrisDenton · 2025-06-05T23:40:32Z

@bors2 try jobs=x86_64-msvc-*,dist-various-*

Add `Command::resolve_in_parent_path` This is part of #141976 (ACP: rust-lang/libs-team#591). libs-api could not come to a consensus on what the final API should look like so they've agreed to experiment with a few different approaches. This PR just implements the `resolve_in_parent_path` method which forces `Command` to ignore `PATH` set on the child for the purposes of resolving the executable. There was no consensus on what should happen if, for example, `chroot` is set (which can change the meaning of paths) so for now I've just done the easy thing. Figuring out the correct behaviour here will need to be done before this is considered for stabilisation but hopefully having it on nightly will nudge people in to giving their opinions. try-job: `x86_64-msvc-*` try-job: `dist-various-*`

rust-bors · 2025-06-05T23:40:36Z

⌛ Trying commit ee32374 with merge e211489…

To cancel the try build, run the command @bors2 try cancel.

rust-bors · 2025-06-06T02:12:04Z

☀️ Try build successful (CI)
Build commit: e211489 (e2114896a1c09ea2c35cbddba98ee4c816032eea)

workingjubilee

If my ability to read code has not completely left me then I have found a tiny problem.

workingjubilee · 2025-06-17T00:48:44Z

library/std/src/sys/process/unix/unix.rs

+        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];
+                    let mut environ = *sys::env::environ();
+                    // Search the environment for PATH and, if found,
+                    // search the paths for the executable by trying to execve each candidate.
+                    while !(*environ).is_null() {
+                        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 {


this looks like the actual key part of the diff to review, I guess.

workingjubilee · 2025-06-17T00:50:46Z

library/std/src/sys/process/unix/unix.rs

+                unsafe {
+                    let name = program.to_bytes();
+                    let mut buffer =
+                        [const { mem::MaybeUninit::<u8>::uninit() }; libc::PATH_MAX as usize];


I wonder how accurate PATH_MAX actually is in practice...

workingjubilee · 2025-06-17T00:51:33Z

library/std/src/sys/process/windows.rs

+        let child_paths = if env_saw_path
+            && !self.force_parent_path
+            && let Some(env) = maybe_env.as_ref()
+        {


hm. this means that force_parent_path removes child_paths from consideration on line

rust/library/std/src/sys/process/windows.rs

Lines 536 to 538 in ee32374

// 1. Child paths

// This is for consistency with Rust's historic behavior.

if let Some(paths) = child_paths {

which... is only for historical consistency? really? damn.

workingjubilee · 2025-06-17T00:57:56Z

library/std/src/sys/process/unix/unix.rs

+            || (!self.get_resolve_in_parent_path()
+                && self.env_saw_path()
+                && !self.program_is_path())


hrm. do we ever examine this bool individually? feels like nah.

workingjubilee · 2025-06-17T01:01:31Z

library/std/src/sys/process/unix/unix.rs

+                    let name = program.to_bytes();
+                    let mut buffer =
+                        [const { mem::MaybeUninit::<u8>::uninit() }; libc::PATH_MAX as usize];
+                    let mut environ = *sys::env::environ();


I guess we can rely on environ always being defined since otherwise it's a linkage error, yeah, maybe.

workingjubilee · 2025-06-17T01:04:09Z

library/std/src/sys/process/unix/unix.rs

+                    let mut environ = *sys::env::environ();
+                    // Search the environment for PATH and, if found,
+                    // search the paths for the executable by trying to execve each candidate.
+                    while !(*environ).is_null() {


environ itself can have the value of a null pointer, however, thanks to clearenv.

rustbot assigned workingjubilee Jun 4, 2025

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Jun 4, 2025

This comment has been minimized.

Sign in to view

ChrisDenton force-pushed the parent-path branch from 350ba73 to dc6b054 Compare June 4, 2025 18:49

This comment has been minimized.

Sign in to view

rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 4, 2025

ChrisDenton force-pushed the parent-path branch from dc6b054 to 57a631f Compare June 5, 2025 12:41

ChrisDenton commented Jun 5, 2025

View reviewed changes

ChrisDenton force-pushed the parent-path branch from ff6b398 to 87a9383 Compare June 5, 2025 14:51

This comment has been minimized.

Sign in to view

ChrisDenton force-pushed the parent-path branch from 87a9383 to fa213a7 Compare June 5, 2025 15:07

This comment has been minimized.

Sign in to view

ChrisDenton force-pushed the parent-path branch 3 times, most recently from f1663e5 to c8a279b Compare June 5, 2025 17:28

ChrisDenton force-pushed the parent-path branch from c8a279b to 5a42e16 Compare June 5, 2025 19:53

ChrisDenton force-pushed the parent-path branch from 5a42e16 to f892c4c Compare June 5, 2025 21:06

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 5, 2025

Add Command::resolve_in_parent_path

ee32374

ChrisDenton force-pushed the parent-path branch from f892c4c to ee32374 Compare June 5, 2025 23:38

workingjubilee requested changes Jun 17, 2025

View reviewed changes

rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 17, 2025

		/// On Unix the process is executed after fork and after setting up the rest of the new environment.
		/// So if `chroot`, `uid`, `gid` or `groups` are used then the way `PATH` is interpreted may be changed.

	// 1. Child paths
	// This is for consistency with Rust's historic behavior.
	if let Some(paths) = child_paths {

Add Command::resolve_in_parent_path #142035

Are you sure you want to change the base?

Add Command::resolve_in_parent_path #142035

Conversation

ChrisDenton commented Jun 4, 2025

Uh oh!

rustbot commented Jun 4, 2025

Uh oh!

This comment has been minimized.

This comment has been minimized.

ChrisDenton commented Jun 4, 2025

Uh oh!

workingjubilee commented Jun 4, 2025

Uh oh!

ChrisDenton commented Jun 5, 2025

Uh oh!

rust-bors bot commented Jun 5, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rust-bors bot commented Jun 5, 2025

Uh oh!

This comment has been minimized.

This comment has been minimized.

ChrisDenton commented Jun 5, 2025

Uh oh!

rust-bors bot commented Jun 5, 2025

Uh oh!

rust-bors bot commented Jun 5, 2025

Uh oh!

ChrisDenton commented Jun 5, 2025

Uh oh!

rust-bors bot commented Jun 5, 2025

Uh oh!

ChrisDenton commented Jun 5, 2025

Uh oh!

rust-bors bot commented Jun 5, 2025

Uh oh!

ChrisDenton commented Jun 5, 2025

Uh oh!

rust-bors bot commented Jun 5, 2025

Uh oh!

ChrisDenton commented Jun 5, 2025

Uh oh!

rust-bors bot commented Jun 5, 2025

Uh oh!

ChrisDenton commented Jun 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rust-bors bot commented Jun 5, 2025

Uh oh!

ChrisDenton commented Jun 5, 2025

Uh oh!

rust-bors bot commented Jun 5, 2025

Uh oh!

ChrisDenton commented Jun 5, 2025

Uh oh!

rust-bors bot commented Jun 5, 2025

Uh oh!

ChrisDenton commented Jun 5, 2025

Uh oh!

rust-bors bot commented Jun 5, 2025

Uh oh!

rust-bors bot commented Jun 6, 2025

Uh oh!

workingjubilee left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Add `Command::resolve_in_parent_path` #142035

Add `Command::resolve_in_parent_path` #142035

ChrisDenton commented Jun 5, 2025 •

edited

Loading