Disabling allocations in `pre_exec` (and signal handlers)

[sylfn]

Proposal

Problem statement

Allocating inside CommandExt::pre_exec¹ is a footgun.

According to POSIX, malloc is not async-signal-safe, meaning that it's not guaranteed to work after fork in a multi-thread process. In practice, this sometimes leads to deadlocks, although some libcs take liberty in making it fine to call malloc/free after fork.

2024 revision of POSIX says that "the application shall ensure that the child process only executes async-signal-safe operations until such time as one of the exec functions is successful". This does not clearly indicate that executing them is UB. However, the previous revision says that, under the same conditions, it is undefined behavior to invoke fork if async-signal-unsafe pthread_atfork handlers are registered. Since this is just invoking async-signal-unsafe functions with a layer of indirection, I believe this wording implies executing async-signal-unsafe operations after fork is UB in general. Musl authors seemingly agree with this interpretation.

Yet it can be tricky to know if the function is allocating, or at least not immediately obvious, as allocations are hidden and every Rust function can allocate. This problem is exacerbated by lack of tools to detect allocations inside pre_exec: Miri can't analyze pre_exec hooks, clippy does not lint against using these functions and Valgrind remains silent. We can't make pre_exec safe, but we can at least help people recognise bugs in their code.

Motivating examples or use cases

During efforts to make panicking work normally after fork it was discovered that a simple catch_unwind solution does not reliably work across systems because of allocations and it was suggested to make global allocator abort after fork. Though std moved forward with the other solution to "don't unwind past fork() in child", it still could be helpful to know where these allocations were.

The pre_exec allocations footgun is mostly hit by external code, though. @purplesyringa recently found many projects that unknowingly use allocating Error::other or Error::new in pre_exec hooks. It is not clear how developers could find this bug.

1. Even though chdir(2) is async-signal-safe, Rust's std::env::set_current_dir is not, because it has to allocate to append NUL-byte to path. This applies to all filesystem-related functions. (Note that Command::current_dir could not be used in Non async-signal-safe closure in pre_exec alacritty/alacritty#8751 because the intention was to swallow errors.)
2. Allocating functions can be deeply nested in pre_exec, like filesystem accesses inside Cgroup::add_task in Allocations in pre_exec openanolis/cryptpilot#54, or could be abstracted away, like in logger in Fix non-async-signal-safety in pre_exec alacritty/alacritty#8756, so simple clippy lints could not help detect allocations.
3. Project maintainers showed interest in such functionality: Avoid allocating in pre_exec closure reubeno/brush#777 (comment).

Solution sketch

The idea is to make allocations inside spawn variant pre_exec a library UB and integrate checks into ub-checks efforts. Although POSIX mentions only multi-threaded processes, std could possibly have created helper threads, and lack of helper threads should probably be considered an implementation detail by treating all programs as multi-threaded in the pre_exec case.

While malloc(3) is async-signal-unsafe, certain custom allocators may be guaranteed to work correctly after fork (e.g. bump allocators). Therefore the check must be opt-in for the allocator.

Since async-signal-safe contexts also appear in signal handlers, it would be prudent to make the API useful for that purpose as well. We don't have to expose it right away, but the developed solution should be forward-compatible.

All in all, the plan is to expose a function to query whether we are in an "async signal" context (like std::thread::panicking) and a "guard" (like DropGuard).

// Names are up to bikeshed
mod thread {
    #[thread_local]
    static ASYNC_SIGNAL_NESTING: Cell<usize> = const { Cell::new(0) };

    pub fn is_async_signal_context() -> bool {
        ASYNC_SIGNAL_NESTING.get() != 0
    }

    pub struct AsyncSignalGuard;

    impl AsyncSignalGuard {
        fn new() -> Self {
            ASYNC_SIGNAL_NESTING.set(ASYNC_SIGNAL_NESTING.get() + 1);
            Self
        }
    }

    impl Drop for AsyncSignalGuard {
        fn drop(&mut self) {
            ASYNC_SIGNAL_NESTING.set(ASYNC_SIGNAL_NESTING.get() - 1);
        }
    }
}

Allocators would use this API to make sure they were not called inside an async signal context:

debug_assert!(
    !std::thread::is_async_signal_context(),
    "This allocator does not support allocating in async-signal-safe context."
);

The allocator, or a custom wrapper, could alternatively choose to use assert! to run such checks even in release mode. The System allocator would have to use a different mechanism than debug_assert! if std is pre-built.

Users of raw fork, like Command::spawn, and signal handlers may enter the async signal context with AsyncSignalGuard:

let pid = unsafe { cvt(libc::fork())? };
if pid == 0 {
    std::panic::always_abort();
    let _guard = std::thread::AsyncSignalGuard::new();
    // remaining code here
}

These functions should be stubbed on #[cfg(not(unix))] so that allocators don't have to always use #[cfg(unix)] (e.g. because we may want to introduce such checks on other platforms).

Note that panic! inside pre_exec will not show traceback now (playground), and using this approach with allocator will produce a near-useless message "you allocate in async-signal-unsafe context" without telling where exactly that allocation happened. Though I believe one can add a breakpoint to panic and view traceback in a debugger, it has a less nicer experience than desired.

To sum up, this proposal consists of two parts:

• A new public API, and
• Updating spawn to enter an async signal context and the System allocator to panic when invoked in an async signal context.

Alternatives

Custom allocator published on crates.io

It is possible to write a custom allocator that would wrap System and register pthread_atfork handler to set a flag to disable allocations (this only detects fork and not _Fork; though another fork detection mechanism may not have this issue). However:

• Users would need to actively opt in this allocator, and third-party tools lack good discoverability compared to first-party tools like Miri. To use this debug mechanism, you'd have to be aware of the possibility of allocations, but the main reason this problem is wide-spread is lack of this knowledge.
• It does not open opportunity to mark other, non-allocating std functions as async-signal-unsafe, limiting the applicability.
• There is a many-to-many correspondence between async signal contexts (fork, manual syscalls, manually set up signal handlers) and async-signal-unsafe functions (allocators, libc wrappers, unwinders). For the checks to work, both have to use the same registry to enter async signal contexts and assert safety, so there would have to be exactly one such crate in the ecosystem, with no possibility of switching without losing functionality. That crate might as well be std.

Replace "async signal context" with "no-alloc context"

Focusing on "no-alloc context" will make it impossible to track non-allocating async-signal-unsafe functions, e.g. functions using global mutexes or non-reentrant functions. However, "no-alloc context" is a platform-independent idea and might be useful outside of cfg(unix). It is not clear which approach is better.

Just wait for effects

Suggested in bootc-dev/containers-image-proxy-rs#109 (comment)

Effects in Rust are not coming soon, whereas the proposed solution is simple enough for an incremental improvement.

Clippy lint against certain functions

Suggested in jamesmcm/vopono#340 (comment)

Clippy can't see through functions, and a lot of APIs unknowingly allocate. The lint would have a lot of false negatives and therefore be near-useless. A more complicated analysis would just be an ad-hoc implementation of effects.

Links and related work

• Aborting on panic after fork: std::process::unix: Command: Do not unwind past fork(), in child rust#80263, Do not allocate or unwind after fork rust#81858, Tracking Issue for panic::always_abort() rust#84438
• Similar proposal idea: std::process::unix: Command: Do not unwind past fork(), in child rust#80263 (comment), Do not allocate or unwind after fork rust#81858 (review)
• Unsafe uses of Error::new/Error::other: Document Error::{new,other} as to be avoided in pre_exec rust#148971

What happens now?

This issue contains an API change proposal (or ACP) and is part of the libs-api team feature lifecycle. Once this issue is filed, the libs-api team will review open proposals as capability becomes available. Current response times do not have a clear estimate, but may be up to several months.

Possible responses

The libs team may respond in various different ways. First, the team will consider the problem (this doesn't require any concrete solution or alternatives to have been proposed):

• We think this problem seems worth solving, and the standard library might be the right place to solve it.
• We think that this probably doesn't belong in the standard library.

Second, if there's a concrete solution:

• We think this specific solution looks roughly right, approved, you or someone else should implement this. (Further review will still happen on the subsequent implementation PR.)
• We're not sure this is the right solution, and the alternatives or other materials don't give us enough information to be sure about that. Here are some questions we have that aren't answered, or rough ideas about alternatives we'd want to see discussed.

Footnotes

1. Command can be materialized on unix in two ways -- Command::spawn and CommandExt::exec. All pre_exec madness about async-signal-safety applies only to spawn variant. ↩

[the8472]

Although POSIX mentions only multi-threaded processes, std could possibly have created helper threads, and lack of helper threads should probably be considered an implementation detail by treating all programs as multi-threaded in the pre_exec case.

Imo forking single-threaded programs is a legitimate use-case (e.g. prewarmed forkserver processes). I don't think we should outright deny them. And it seems unlikely that std would gain a general feature that would rely on background threads because some platforms don't even have threads (e.g. some wasm flavors).

[purplesyringa]

The thing I'm worried about is that basically anything can suddenly become multi-threaded, so you could be facing a situation where an unrelated change uncovers accidental allocations in pre_exec closures, say because the change brought in rayon, or a temporary thread terminates later than expected. I suppose this is at least partially mitigated by the fact that unit tests typically run in multiple threads, but introspecting the global process state to determine whether an error should be thrown makes me uneasy.

Another point is that all safe Rust code is supposed to support multi-threading to boot -- even if a function receives a !Sync type by reference, it can only assume that cross-thread races on that type are absent, not that there is a single thread. We also don't have a knob that allows static variables to store !Sync types at the expense of disabling threading -- you have to use TLS and pay for the increased cost.

Basically, we don't make any provisions to allow single-threaded code to do more than multi-thread code can, and this is consistent. I agree that supporting preforking is useful, but it doesn't seem to me like it fits in Rust's model, so naturally all fixes will be unergonomic.

Something to keep in mind is that preforking doesn't usually rely on pre_exec. So if instead of registering pthread_atfork, we (as std) only entered async signal contexts in pre_exec and maybe a couple other places, user code would still be able to use libc::fork without triggering panics. This change would only negatively impact allocations within pre_exec from a single-thread process, which seems rarer.

[the8472]

This change would only negatively impact allocations within pre_exec from a single-thread process, which seems rarer.

Afaik some large processes like databases and browsers use a small spawn helper process so they don't have to pay the cost of the virtual memory cloning. Those could legitimately alloc in their pre_exec.
But yeah, I don't know if they actually make use of this possibility.

[the8472]

If this is just for ub checks it can be best-effort, so we could try to figure out if the parent process is multi-threaded before forking and then use that information in the child to inform the allocator whether to panic or not.

[hanna-kruppe]

I would love more tooling to make it less practically-impossible to write async-signal-safe (AS-safe) code. However, I’m not sure if manually adding UB checks in individual functions is the best way to go about this:

• Virtually nothing is AS-safe, so the number of ways you can accidentally do something non-AS-safe is limitless. Catching the most common problems would already be much better than nothing, but I worry that even getting to decent coverage of the most common problems will require an excessive number of such checks.
• Checking only in a few low-level functions like the global allocator impl reduces annotation burden, but doesn’t help with functions that are definitely not AS-safe but only sometimes call into the specific functions that do the check.

I don’t necessarily have a better idea. The opposite, static analysis that requires all functions to be annotated as AS-safe (and rejects indirect calls), is hardly practical even if that’s how AS-safe code should ideally be written. I don’t think anyone’s interested in haggling over which parts of std and core are guaranteed to be AS-safe. And ThreadSanitizer has proven that even just flagging basic issues like “does your signal handler call malloc” can catch real bugs in mature software.

Aside: TLS access isn’t guaranteed to be AS-signal-safe, so the proposed flag probably can’t be thread-local. Is there any reason why it shouldn’t be global? Most sources of AS-unsafety are process-global anyway.

---

FWIW, there’s also some important differences between pre_exec and signal handlers. Signal handlers aren’t allowed to clobber errno (ThreadSanitizer also checks this). Panics are terminated safely (with some restrictions on fmt arguments) in pre_exec but not in signal handlers, since panic::always_abort() is a one-way door. The single-threaded exceptions that @the8472 don't apply to since signal handlers mostly act like a separate thread. I would (selfishly) love if whatever comes out of this also helps with writing signal handlers, but these differences make me skeptical that a single mechanism can cover both signal handlers and pre_exec.

[hanna-kruppe]

Aside: TLS access isn’t guaranteed to be AS-signal-safe, so the proposed flag probably can’t be thread-local. Is there any reason why it shouldn’t be global? Most sources of AS-unsafety are process-global anyway.

Answering my own question: fork is global but signal handlers are thread local, duh. So that’s another case of signal handlers having different needs than pre_exec.

I believe most (all?) TLS implementations in ELF are AS-safe in practice if the thread already allocated TLS before a signal is handled on the respective thread. For pre_exec this can easily be ensured by std, but for signal handlers it seems virtually impossible to guarantee. std could spawn threads with signals blocked initially until TLS for this is initialized, but that doesn’t help with threads not created through std.

Of course, since this is a best effort way to detect already-erroneous programs, it may be reasonable to do the TLS access anyway and not worry too much about whether it’s bulletproof. However, using TLS for this check makes it harder to flag TLS accesses in pre_exec/signal handlers since there would need to be an exception for the flag itself.

[Amanieu]

We discussed this in the @rust-lang/libs-api meeting yesterday. While we appreciate that proper use of pre_exec can be difficult, we think that exposing a complex public API just to catch issues with its usage is overkill. We would be amenable to adding debug-only checks on the global allocator to detect its use in pre_exec, but only if this can be done without any performance overhead since the allocator is a very hot code path. We consider signal handlers to be outside the scope of the standard library.

Finally, the documentation for pre_exec could be made more explicit by specifically pointing people to only use libc functions that are documented as being async-signal-safe and avoid any standard library functions.