Avoid allocating in pre_exec closure

[purplesyringa]

Error::new allocates memory (see rust-lang/rust#148971). This is bad in multi-threaded programs, and containers-image-proxy-rs is a library which can be used in such. If the fork occurs while the allocator lock is held by another thread, deadlocks can occur, since there's no one left in the new process to unlock the mutex. I do not believe this is UB, and modern libc offer protections against this issue, but this isn't POSIX-compliant and should preferably be avoided.

rustix provides a non-allocating impl From<Errno> for std::io::Error, use it instead. This also ensures the correct error code is forwarded to the parent process, instead of the default -EINVAL.

[gemini-code-assist]

Code Review

This pull request correctly addresses a subtle but critical issue with memory allocation within a pre_exec closure, which could lead to deadlocks in a multi-threaded context. The fix is correct and effectively prevents this by using a non-allocating error conversion. My review includes a minor suggestion to improve code clarity and make the intent more explicit.

[gemini-code-assist]

While the Ok(...?) pattern works correctly here because the success type is (), it can be a bit subtle. A more direct and idiomatic approach is to use map_err to handle the error conversion explicitly. This makes the intent of the code clearer to future readers.

                    rustix::process::set_parent_process_death_signal(Some(
                        rustix::process::Signal::TERM,
                    ))
                    .map_err(Into::into)

[purplesyringa]

consider: nuh-uh

[allisonkarlitskaya]

I kinda agree with this comment. That ? is carrying an awful lot of weight here, and it's not clear to me what implementation we're going to end up using or if it's going to allocate. A more explicit constructor (more explicit even than what the bot recommended) and comment that we're trying to avoid allocations would also be nice....

[cgwalters]

In balance, since ? Does The Right Thing, I'm fine using it. If the situation were otherwise then it'd be far worse.

What we really need here is some notion of "this API may allocate" as an annotation/effect on functions, then we could deny those in pre_exec.

Or you know...rustix could have an allowlist of functions that are safe to call.

Actually also we should try to add this pdeathsig as an extension to std so the unsafety is centralized.

[purplesyringa]

I'd love to see some variation on pdeathsig in std, though not pdeathsig alone (you also need to check ppid for avoid race conditions). That would be universally useful, maybe even possible to implement in a cross-platform manner at least on some OSes.

I'm not opposed to using .map_err(Into::into) instead of ? if it's idiomatic for the project, I just don't like following questionable automatic suggestions without a person in the loop.