Avoid allocating in pre_exec closure#5533
Conversation
There was a problem hiding this comment.
Code Review
This pull request correctly identifies and fixes a potential deadlock issue by avoiding memory allocation within a pre_exec closure. The use of a non-allocating error conversion is a good improvement for safety in multi-threaded contexts. My review includes a suggestion to make the implementation more idiomatic, consistent with other parts of the codebase.
| Ok(rustix::process::set_parent_process_death_signal(Some( | ||
| rustix::process::Signal::TERM, | ||
| )) | ||
| .map_err(|e| std::io::Error::new(std::io::ErrorKind::Other, e)) | ||
| ))?) |
There was a problem hiding this comment.
While this change correctly avoids allocation in the pre_exec closure, it can be written more idiomatically using .map_err(Into::into). This achieves the same non-allocating error conversion from rustix::io::Errno to std::io::Error in a more direct and arguably clearer way. This pattern is also used elsewhere in the codebase (see rust/src/cmdutils.rs:103).
rustix::process::set_parent_process_death_signal(Some(
rustix::process::Signal::TERM,
))
.map_err(Into::into)There was a problem hiding this comment.
Yeah, and rust/src/extensions.rs:171, rust/src/countme/cookie.rs:148, rust/src/treefile.rs:4732, rust/src/treefile.rs:4749, and a ton of other locations use Ok(result?). I can't with these guys.
Though I guess it's a similar snippet. Not sure if that matters for consistency, I'll wait for a review.
Error::newallocates memory (see rust-lang/rust#148971). This is bad in multi-threaded programs, which rpm-ostree AFAIK is. 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.rustixprovides a non-allocatingimpl 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.