std::process::Command doesn't follow Unix signal safety in forked child

[ericyoungblut]

Update (2022-12-12): The issue description is outdated, but some problems remain.

These lines from spawn can cause a deadlock. It's wrong to assume that you can lock in the parent and the unlock in the child. Read man 2 fork and man 7 signal-safety on Linux.

    // Whatever happens after the fork is almost for sure going to touch or
    // look at the environment in one way or another (PATH in `execvp` or
    // accessing the `environ` pointer ourselves). Make sure no other thread
    // is accessing the environment when we do the fork itself.
    //
    // Note that as soon as we're done with the fork there's no need to hold
    // a lock any more because the parent won't do anything and the child is
    // in its own process.
    let result = unsafe {
        let _env_lock = sys::os::env_lock();
        cvt(libc::fork())?
    };

Qumulo has a custom thread library which exposes this issue.

[steveklabnik]

Triage: I am not sure that the standard library intends to be fork safe.

[Mark-Simulacrum]

Cc @joshtriplett

We try to make sure std code itself is fork safe, though that may not apply to std APIs (e.g., forking in Cell::update is probably a bad idea).

I am unsure what exactly is a good solution in this case, though.

[RalfJung]

Triage: I am not sure that the standard library intends to be fork safe.

I mean, the operations it provides should be implemented properly. From how I understand this post, using std::process::Command can lead to violating the contract of the underlying APIs by calling something non-async-signal-safe after fork().

We also made before_exec unsafe partially because of signal safety concerns.

[RalfJung]

Also it seems like execvp is not signal-safe (which is quite surprising, as fork+exec is such a standard operation), so the cases where Command uses that function are also violating POSIX.

[tmiasko]

See #55359 for an approach that doesn't have the issues described here. It also avoids execvp (note that the pull request description isn't up to date with respect to the final version of the code).

Although, it didn't handle the case when PATH was unset, where excevp uses a default search path _CS_PATH, introduced a search path regression, and was subsequently reverted.

[RalfJung]

Oh wow I had already successfully forgotten this...

[RalfJung]

@joshtriplett wrote elsewhere:

To the best of my knowledge, execvp is only considered "not async signal safe" because all functions that read the environment aren't safe against concurrent modifications: if you change PATH concurrently from another thread, you have no guarantees whether it'll use the old PATH, the new, or some combination thereof. execvp is listed as MT-Safe env, meaning "it's multi-thread safe as long as you don't expect environment variables to do any synchronization for you".

However, it is not clear to me how "multi-thread safe" and "signal safe" (aka can-be-used-after-fork) relate.

[joshtriplett]

We also can't rely on any particular safety property of execvp that happens to be true in a particular implementation, since it isn't guaranteed by the standard.

[apiraino]

Assigning P-medium as discussed as part of the Prioritization Working Group procedure and removing I-prioritize.

[joshtriplett]

Following up on this, here's a summary of the changes that would need to happen:

• We need to forget or perhaps poison the lock in the child process, rather than unlock it when we weren't the one that locked it.
• We should extend the documentation for pre_exec to note that it must not touch the environment at all, in addition to its various other restrictions.
• We need to stop calling execvp entirely. (This is surprising, as it's widely used after fork, and we should investigate if we're understanding the requirements correctly, but it does look from the documentation like we can't use it.) This means we need to do the PATH search ourselves. Traditionally, this is done by repeatedly calling execve on the possible locations of the command, in order, along with some care to capture and return the appropriate error code.
• Since we're doing the PATH search ourselves, we don't need to set the global environ; we can just call execve and pass the environment we want. This will let us simplify the recovery logic for a failed exec.
• We should extend the documentation of Command to elaborate further on how setting PATH via the Command environment functions affects the search for the specified command itself. People may not expect this, as it's not the same behavior as execvpe or posix_spawnp, which use the caller's PATH rather than the one from the envp argument. There's a hint about this in the documentation for Command::env, but we should expand on that and mention it elsewhere in the documentation
• Once we have our own PATH search, we could use that rather than calling posix_spawnp as well, which will allow us to call posix_spawn in more cases (we could drop all the env_saw_path logic).
• We need to avoid touching the memory allocator in the child. Any memory we need to allocate (including for the PATH search) needs to be allocated in the parent before forking; the child can then use that memory. (We can also do more of the work in the parent before forking.)
• We need to avoid calling any other functions that aren't async-signal-safe in the child, after forking.

[ehuss]

we don't respect the PATH the user passes in via the Command's environment functions, which is inconsistent.

Just caution that changing this could break existing code (though unlikely). #37868 is a similar issue where current_dir() behaves differently on different platforms, but fixing it would mean breaking one or the other behavior. (FWIW, I think consistency would be better.) If changing things, it might be strange if PATH worked differently from current_dir() (that is, whether or not Command uses the calling environment or the environment specified), so that is something to keep in mind.

[RalfJung]

What is the current status of this?

At least as far as the env lock is concerned, the relevant code changed quite a bit:

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

Lines 71 to 78 in 9e77211

	let env_lock = sys::os::env_read_lock();
	let (pid, pidfd) = unsafe { self.do_fork()? };
	if pid == 0 {
	crate::panic::always_abort();
	mem::forget(env_lock);
	drop(input);
	let Err(err) = unsafe { self.do_exec(theirs, envp.as_ref()) };

This looks to me like it will avoid calling the non-signal-safe 'unlock' in the child. However there is no comment next to the mem::forget so it is hard to say whether that is a proper solution here. Looks like #82949 is the PR where this happened, and judging from that PR indeed it sounds like the env access issue here has been resolved?

We are still calling execvp though, so there still is some issue here. But the issue description is very outdated.