Avoid allocating in pre_exec closure

[purplesyringa]

Error::other allocates memory (see rust-lang/rust#148971). This is bad in multi-threaded programs, which brush 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.

Luckily, nix provides a non-allocating impl From<Errno> for std::io::Error, and it passes the correct error code through to the parent process, instead of using the default -EINVAL like Error::other.

I'm not sure how to best integrate this into your project. I changed the return type of move_self_to_foreground to use std::io::Error, which works, but feels hacky. If you have a better idea, feel free to apply your own patch and close this PR.

[github-actions]

Public API changes for crate: brush-core

Changed items

-pub fn brush_core::sys::terminal::move_self_to_foreground() -> core::result::Result<(), brush_core::error::Error>
+pub fn brush_core::sys::terminal::move_self_to_foreground() -> core::result::Result<(), std::io::error::Error>

Performance Benchmark Report

Benchmark name	Baseline (μs)	Test/PR (μs)	Delta (μs)	Delta %
clone_shell_object	17.68 μs	17.84 μs	0.16 μs	🟠 +0.92%
eval_arithmetic	0.15 μs	0.14 μs	-0.00 μs	⚪ Unchanged
expand_one_string	1.67 μs	1.72 μs	0.05 μs	⚪ Unchanged
for_loop	22.46 μs	22.52 μs	0.06 μs	⚪ Unchanged
function_call	2.28 μs	2.38 μs	0.10 μs	⚪ Unchanged
instantiate_shell	58.54 μs	59.53 μs	0.99 μs	⚪ Unchanged
instantiate_shell_with_init_scripts	25257.57 μs	24614.01 μs	-643.56 μs	🟢 -2.55%
parse_bash_completion	2058.92 μs	2045.65 μs	-13.27 μs	⚪ Unchanged
parse_sample_script	2.04 μs	1.95 μs	-0.08 μs	🟢 -4.12%
run_echo_builtin_command	15.47 μs	15.40 μs	-0.07 μs	⚪ Unchanged
run_one_external_command	2258.58 μs	2392.19 μs	133.61 μs	🟠 +5.92%
tokenize_sample_script	3.47 μs	3.52 μs	0.05 μs	🟠 +1.32%

Code Coverage Report: Only Changed Files listed

Package	Base Coverage	New Coverage	Difference
brush-core/src/jobs.rs	🔴 37.16%	🔴 47.71%	🟢 10.55%
Overall Coverage	🟢 73.57%	🟢 73.7%	🟢 0.13%

Minimum allowed coverage is 70%, this run produced 73.7%

Test Summary: bash-completion test suite

Outcome	Count	Percentage
✅ Pass	1326	62.87
❗️ Error	216	10.24
❌ Fail	198	9.39
⏩ Skip	354	16.79
❎ Expected Fail	14	0.66
✔️ Unexpected Pass	1	0.05
📊 Total	2109	100.00

[reubeno]

Thanks for identifying this issue and contributing a fix, @purplesyringa! The latest version of your change seems reasonable given the constraints.

(I read your linked issue and see how many other projects you believe are affected. If you're willing to share, I'm curious to know how you came across this and what ideas you have on how we could mechanically catch these sorts of challenges in the future. (I know that pre_exec is unsafe so I recognize that some risk was already implied.))

[purplesyringa]

First of all, I'm confused why the bot mentions nix::Error -- I noticed that that wouldn't work on Windows and force-pushed a fix that replaced the type with std::io::Error, so that must be outdated. But yeah, this would require a public API change I'm afraid, unless you're willing to match on your own Error.

If you're willing to share, I'm curious to know how you came across this

My girlfriend asked me to review her code. I thought Error::other is an odd choice for pre_exec, but for a different reason, though I don't remember which reason specifically. Eventually I realized that this reason doesn't apply, but that Error::other allocates, and that's also a problem. And if I missed this bug on my first review, others might have missed it too. A quick GitHub search later showed that it's quite popular.

and what ideas you have on how we could mechanically catch these sorts of challenges in the future.

That's a good question. A clippy lint might help, but it's not clear how to decide which functions are allowed to be called, since we don't have effects and there's no annotations of that sort, so it'd just be a blacklist. Something that I think might be worthwhile is to disable the global allocator in std::process::Command after fork, at least in debug mode/with debug assertions on. I'm not sure if Rust project would be willing to have that in std, since a) some libc (try to) guarantee allocation is safe and b) this is actually safe in single-threaded programs, but I can ask. Barring that, we could have a nightly-only crate wrapping any passed allocator, to be used in #[global_allocator].

[reubeno]

First of all, I'm confused why the bot mentions nix::Error -- I noticed that that wouldn't work on Windows and force-pushed a fix that replaced the type with std::io::Error, so that must be outdated.

That's right. Once the checks re-ran, it updated the compat report accordingly. We really only use the sys module within this broader project, but did need it public. (We'll be looking into ways to place it behind a feature flag, or otherwise mark it as less stable.) For now, I think it's an acceptable change.

And if I missed this bug on my first review, others might have missed it too.

Absolutely--I'm one of those. I'm aware of the risks of allocation between fork and exec in a multithreaded program but didn't stop to think about the safety of constructing an error there. It's a good catch!

Something that I think might be worthwhile is to disable the global allocator in std::process::Command after fork, at least in debug mode/with debug assertions on. I'm not sure if Rust project would be willing to have that in std, since a) some libc (try to) guarantee allocation is safe and b) this is actually safe in single-threaded programs, but I can ask.

It sounds like it's worth a conversation, but agree that such checks may be overly restrictive for some platforms. I like the idea of a wrapper for debugging/validation. I haven't used any of them before, but I see on crates.io a handful of crates that provide debugging-oriented global allocators for verification/tracking/instrumentation. I could envision using that during testing to collect allocation metrics as well as look for anything that's... suspicious.