Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[musl] use posix_spawn if a directory change was requested #131851

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
+56 −12
Open

[musl] use posix_spawn if a directory change was requested #131851

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions library/std/src/process/tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,9 +96,23 @@ fn stdout_works() {
#[test]
#[cfg_attr(any(windows, target_os = "vxworks"), ignore)]
fn set_current_dir_works() {
// On many Unix platforms this will use the posix_spawn path.
workingjubilee marked this conversation as resolved.
Show resolved Hide resolved
let mut cmd = shell_cmd();
cmd.arg("-c").arg("pwd").current_dir("/").stdout(Stdio::piped());
assert_eq!(run_output(cmd), "/\n");

// Also test the fork/exec path by setting a pre_exec function.
#[cfg(unix)]
{
use crate::os::unix::process::CommandExt;

let mut cmd = shell_cmd();
cmd.arg("-c").arg("pwd").current_dir("/").stdout(Stdio::piped());
unsafe {
cmd.pre_exec(|| Ok(()));
}
assert_eq!(run_output(cmd), "/\n");
}
}

#[test]
Expand Down
54 changes: 42 additions & 12 deletions library/std/src/sys/pal/unix/process/process_unix.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -448,7 +448,6 @@ impl Command {
use core::sync::atomic::{AtomicU8, Ordering};

use crate::mem::MaybeUninit;
use crate::sys::weak::weak;
use crate::sys::{self, cvt_nz, on_broken_pipe_flag_used};

if self.get_gid().is_some()
Expand All @@ -462,6 +461,8 @@ impl Command {

cfg_if::cfg_if! {
if #[cfg(target_os = "linux")] {
use crate::sys::weak::weak;

weak! {
fn pidfd_spawnp(
*mut libc::c_int,
Expand Down Expand Up @@ -575,16 +576,6 @@ impl Command {
}
}

// Solaris, glibc 2.29+, and musl 1.24+ can set a new working directory,
// and maybe others will gain this non-POSIX function too. We'll check
// for this weak symbol as soon as it's needed, so we can return early
// otherwise to do a manual chdir before exec.
weak! {
fn posix_spawn_file_actions_addchdir_np(
*mut libc::posix_spawn_file_actions_t,
*const libc::c_char
) -> libc::c_int
}
let addchdir = match self.get_cwd() {
Some(cwd) => {
if cfg!(target_vendor = "apple") {
Expand All @@ -597,7 +588,10 @@ impl Command {
return Ok(None);
}
}
match posix_spawn_file_actions_addchdir_np.get() {
// Check for the availability of the posix_spawn addchdir
// function now. If it isn't available, bail and use the
// fork/exec path.
match get_posix_spawn_addchdir() {
Some(f) => Some((f, cwd)),
None => return Ok(None),
}
Expand Down Expand Up @@ -871,6 +865,42 @@ impl Command {
}
}

type PosixSpawnAddChdirFn =
unsafe extern "C" fn(*mut libc::posix_spawn_file_actions_t, *const libc::c_char) -> libc::c_int;

/// Get the function pointer for adding a chdir action to a
/// `posix_spawn_file_actions_t`, if available, assuming a dynamic libc.
///
/// Some platforms can set a new working directory for a spawned process in the
/// `posix_spawn` path. This function looks up the function pointer for adding
/// such an action to a `posix_spawn_file_actions_t` struct.
#[cfg(not(all(target_os = "linux", target_env = "musl")))]
fn get_posix_spawn_addchdir() -> Option<PosixSpawnAddChdirFn> {
Comment on lines +877 to +878
Copy link
Member

@workingjubilee workingjubilee Oct 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You've moved this to the top of scope in a sensible-seeming fashion. Unfortunately, this is only called within posix_spawn, which not every platform calls to begin with. Indeed, many cfg it away! So this will not be called at all on platforms that don't match this cfg:

    #[cfg(any(
        target_os = "freebsd",
        all(target_os = "linux", target_env = "gnu"),
        all(target_os = "linux", target_env = "musl"),
        target_os = "nto",
        target_vendor = "apple",
    ))]

But it will still be defined for those. For those which aren't in the above set, but do use process_unix.rs, then they will see an unused function, and then libstd will not build.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, right...

I guess I could probably move the function item to be function-local.

use crate::sys::weak::weak;

weak! {
fn posix_spawn_file_actions_addchdir_np(
*mut libc::posix_spawn_file_actions_t,
*const libc::c_char
) -> libc::c_int
}

posix_spawn_file_actions_addchdir_np.get()
}

/// Get the function pointer for adding a chdir action to a
/// `posix_spawn_file_actions_t`, if available, on platforms where the function
/// is known to exist.
///
/// Weak symbol lookup doesn't work with statically linked libcs, so in cases
/// where static linking is possible we need to either check for the presence
/// of the symbol at compile time or know about it upfront.
#[cfg(all(target_os = "linux", target_env = "musl"))]
fn get_posix_spawn_addchdir() -> Option<PosixSpawnAddChdirFn> {
// Our minimum required musl supports this function, so we can just use it.
Some(libc::posix_spawn_file_actions_addchdir_np)
}

////////////////////////////////////////////////////////////////////////////////
// Processes
////////////////////////////////////////////////////////////////////////////////
Expand Down
Loading