Avoid allocating in pre_exec closure

[purplesyringa]

The documentation for pre_exec specifies that allocating in the pre_exec closure is not guaranteed to work.

Specifically, this is bad in multi-threaded programs: 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.

zaun doesn't seem to be multi-threaded at a first glance, but I'm filing this anyway as part of going through the list at rust-lang/rust#148971. If it's single-threaded, you don't need to merge this, but I'd play safe.

nix provides a non-allocating impl From<Errno> for std::io::Error, which can be used instead. This doesn't allow an additional message to be added, but since error messages aren't transmitted across pre_exec boundary anyway, this doesn't make visible behavior any worse. On the flip side, this ensures that the correct error code is forwarded to the parent process, instead of the default -EINVAL.

[gemini-code-assist]

Summary of Changes

Hello @purplesyringa, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request refactors error handling within a pre_exec closure to prevent memory allocations, which can lead to deadlocks in multi-threaded environments. By replacing an allocating map_err with direct error propagation, the change adheres to pre_exec safety recommendations and ensures more robust and compliant process spawning, even if the zaun project currently appears single-threaded.

Highlights

• Allocation Avoidance: Eliminated a potential memory allocation within the pre_exec closure by removing a map_err call that used anyhow! and std::io::Error::other.
• Improved Error Handling: Switched to direct error propagation using the ? operator, leveraging nix's non-allocating impl From<Errno> for std::io::Error to ensure correct error code forwarding.
• Enhanced Stability: Addressed a potential deadlock scenario in multi-threaded programs that could occur if fork happens while an allocator lock is held, aligning with pre_exec safety guidelines.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description
Code Review	/gemini review	Performs a code review for the current pull request in its current state.
Pull Request Summary	/gemini summary	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help	/gemini help	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

[gemini-code-assist]

Code Review

This pull request correctly addresses a potential deadlock issue in multi-threaded programs by removing memory allocation from the pre_exec closure. The use of ? to leverage nix's non-allocating From<Errno> for std::io::Error conversion is the right approach here. This change improves the robustness and safety of the code, following best practices for writing pre_exec closures. As you noted in the description, this also has the benefit of preserving the original errno, which is great for debugging. The change is well-explained and correctly implemented. Great job!