Prevent mutating the global environment pointer in CommandExt::exec and opt to use execve and resolve path manually - #157144
Open
asder8215 wants to merge 7 commits into
Open
Conversation
rustbot
added
S-waiting-on-review
Collaborator
|
r? @joboet rustbot has assigned @joboet. Use Why was this reviewer chosen?The reviewer was selected based on:
|
This comment has been minimized.
This comment has been minimized.
joboet
requested changes
May 30, 2026
Member
There was a problem hiding this comment.
I highly recommend having a look at the POSIX specification – GNU/Linux mostly follows that, but the specification sometimes leaves some things unspecified and we should be careful not to break those.
Also, this is currently unsound, we cannot perform any memory allocation in do_exec, since it's called after fork for the standard spawn path. You probably need to preallocate some memory when creating the Command (or when setting certain parameters).
| None => c"/bin:/usr/bin".to_bytes_with_nul(), | ||
| } | ||
| } | ||
| None => parent_path.as_bytes_with_nul(), |
Member
There was a problem hiding this comment.
I recommend moving the environment variable lookup here so that it's only performed when actually needed.
rustbot
added
S-waiting-on-author
Collaborator
|
Reminder, once the PR becomes ready for a review, use |
rustbot
added
S-waiting-on-review
asder8215
commented
May 31, 2026
joboet
requested changes
Jun 6, 2026
rustbot
added
S-waiting-on-author
rustbot
added
S-waiting-on-review
asder8215
commented
Jun 8, 2026
This comment has been minimized.
This comment has been minimized.
asder8215
force-pushed
the
commandext_exec_bug_fix
branch
from
June 8, 2026 09:23
213bb02 to
8319b09
Compare
This comment has been minimized.
This comment has been minimized.
asder8215
commented
Jun 8, 2026
joboet
requested changes
Jun 8, 2026
| assert_failed(index, len); | ||
| } | ||
|
|
||
| self.ptrs.insert(index, item.into_raw()); |
Member
There was a problem hiding this comment.
This leaks the string if insert panics (see #155748, I made this mistake before).
rustbot
added
S-waiting-on-author
joboet
reviewed
Jun 8, 2026
asder8215
force-pushed
the
commandext_exec_bug_fix
branch
from
June 12, 2026 16:19
e3d0663 to
9851b0a
Compare
asder8215
force-pushed
the
commandext_exec_bug_fix
branch
2 times, most recently
from
June 12, 2026 16:21
b4723b8 to
df00000
Compare
Member
|
Hey T-libs, This PR makes
@rustbot label +I-libs-nominated |
rustbot
added
the
I-libs-nominated
Member
|
Sounds reasonable to me. |
Member
|
Based on the discussion in the meeting, I'm going to propose FCP on the following:
@rfcbot merge libs |
Collaborator
|
@Amanieu has proposed to merge this. The next step is review by the rest of the tagged team members: No concerns currently listed. Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up! See this document for info about what commands tagged team members can give me. |
rust-rfcbot
added
proposed-final-comment-period
This comment has been minimized.
This comment has been minimized.
asder8215
force-pushed
the
commandext_exec_bug_fix
branch
from
July 7, 2026 22:06
df00000 to
4ca0e7b
Compare
This comment has been minimized.
This comment has been minimized.
Contributor
Author
|
Code's pretty much the same, documentation for the |
nia-e
removed
the
I-libs-nominated
Member
|
I started going through the mainstream UNIX systems to see whether this will change behaviour: So there's a case to be made for using Also, quite some systems violate the UNIX spec (well, the requirement only got added in issue 8, the implementations simply predate 2024) in that they do use the shell on |
Contributor
Author
I can put up a PR to introduce |
tgross35
reviewed
Aug 19, 2026
Comment on lines
450
to
512
| let file = self.get_program_cstr(); | ||
| let file_as_bytes = file.to_bytes_with_nul(); | ||
| let argv = self.get_argv(); | ||
| let envp_ptr = | ||
| maybe_envp.map(|envp| envp.as_ptr()).unwrap_or(unsafe { *sys::env::environ() }); | ||
| let paths = paths.unwrap_or(self.get_default_path().as_encoded_bytes()); | ||
|
|
||
| // Path searching does not occur when our file contains a `/` | ||
| if file_as_bytes.contains(&b'/') { | ||
| libc::execve(file.as_ptr(), argv.as_ptr(), envp_ptr); | ||
| } else { | ||
| let mut got_perm_denied = false; | ||
| for path in paths.split(|b| *b == b':') { | ||
| let path_len = path.len(); | ||
|
|
||
| // Copy path to execute in the pre-allocated buffer accordingly | ||
| // Use the current path entry, plus a '/' if nonempty, plus the file to | ||
| // execute. | ||
| if path_len != 0 { | ||
| buf[0..path_len].write_copy_of_slice(path); | ||
| if path[path_len - 1] != b'/' { | ||
| buf[path_len].write(b'/'); | ||
| buf[path_len + 1..path_len + 1 + file_as_bytes.len()] | ||
| .write_copy_of_slice(file_as_bytes); | ||
| } else { | ||
| buf[path_len..path_len + file_as_bytes.len()] | ||
| .write_copy_of_slice(file_as_bytes); | ||
| } | ||
| } else { | ||
| buf[0..file_as_bytes.len()].write_copy_of_slice(file_as_bytes); | ||
| } | ||
|
|
||
| // Try executing on path | ||
| libc::execve(buf.as_ptr().cast(), argv.as_ptr(), envp_ptr); | ||
| let err = crate::sys::io::errno(); | ||
|
|
||
| match err { | ||
| // Record that we got a 'Permission Denied' error in the event that if we | ||
| // find no executable to use, we should report that there was a usable | ||
| // executable but we were denied access to it. | ||
| libc::EACCES => got_perm_denied = true, | ||
| // Try the next path. | ||
| libc::ENOENT | ||
| | libc::ESTALE | ||
| | libc::ENOTDIR | ||
| | libc::ENODEV | ||
| | libc::ETIMEDOUT => {} | ||
| // Any other error returned by execve should be returned. For example, | ||
| // ENAMETOOLONG is an error that is returned due to security risks on | ||
| // executing the wrong program through setting a very long path | ||
| _ => return Err(io::Error::from_raw_os_error(err)), | ||
| } | ||
| } | ||
|
|
||
| _reset = Some(Reset(*sys::env::environ())); | ||
| *sys::env::environ() = envp.as_ptr(); | ||
| // At least one failure was due to lack of permissions, so we should | ||
| // report that failure first | ||
| if got_perm_denied { | ||
| return Err(io::Error::from_raw_os_error(libc::EACCES)); | ||
| } | ||
|
|
||
| // No paths were executable | ||
| return Err(io::Error::from_raw_os_error(libc::ENOENT)); | ||
| } |
Member
There was a problem hiding this comment.
Can this go in its own function forkless_evecvpe or so? So it can get its own documentation, including why we're not just using execvpe on platforms where that exists #t-libs/crates > Including <paths.h> macros into libc? @ 💬
…nd opt to use execve and resolve path manually
…confstr_as_cstring added to get a CString value returned by confstr, added CStringArray::from_ptr, added default_path and shell_argv fields to unix Command, and refactored do_exec code
…om a single passthrough in envp, match on errno values directly instead of ErrorKind, fix shell_argv CStringArray value.
…remove confstr_as_cstr, remove unnecessary shell_argv field/funtions + unused functions from CStringArray, use slice::split to delimit on colons
…cumentation for returning an error from execve
asder8215
force-pushed
the
commandext_exec_bug_fix
branch
from
August 23, 2026 18:11
a288cc9 to
71e457f
Compare
Collaborator
|
This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
Collaborator
|
The job Click to see the possible cause of the failure (guessed by this bot) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
9 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
View all comments
This PR fixes the bug described in #156951 where
CommandExt::execmutating the global environment pointer could cause correctness issue with concurrentexeccalls (also this function holds an environment read lock, so it shouldn't cause any writes to the global environment pointer anyways). We do everything withinCommandExt::execviaexecvethanexecvp, which means we have to resolve the path manually.I took reference to what was done in an archived (Feb 2026) upstream mirror of glibc's
execvpandexecvpe. I also took reference to #55359 on how they implemented the concerned portion ofCommandExt::execwithexecve. There's also this OpenBSD libc implementation ofexecvpethat I saw, but I didn't deeply take a look at this one and saw how it differ.Other than that, I'm unsure how to handle the error
ETIMEOUT/TimedOutas glibc breaks parsing through thePATHenvironment variable value and returns the error while #55359 continues parsing.Let me know if you also want me to put some of
execvecode in a helper function or refactor it in other ways.