Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

uucore: disable default signal-handlers added by Rust #6806

Merged
merged 2 commits into from
Oct 30, 2024

Conversation

Ecordonnier
Copy link
Contributor

Fixes #6759

Test procedure (verifies that a single SIGSEGV stops a program):

ecordonnier@lj8k2dq3:~/dev/coreutils$ ./target/release/coreutils sleep 100 &
[1] 4175464
ecordonnier@lj8k2dq3:~/dev/coreutils$ kill -11 $(pidof coreutils)
ecordonnier@lj8k2dq3:~/dev/coreutils$
[1]+  Segmentation fault      (core dumped) ./target/release/coreutils sleep 100

@Ecordonnier Ecordonnier force-pushed the eco/signal-handler branch 2 times, most recently from e0b304b to 96d70d7 Compare October 21, 2024 12:14
@@ -24,6 +24,9 @@ pub fn main(_args: TokenStream, stream: TokenStream) -> TokenStream {

let new = quote!(
pub fn uumain(args: impl uucore::Args) -> i32 {
#[cfg(unix)]
uucore::signals::disable_rust_signal_handlers().expect("Disabling rust signal handlers failed");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please a comment explaining the why

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a comment as requested, however it is redundant with the comment above the function definition.

// Disables the custom signal handlers installed by Rust for stack-overflow handling. With those custom signal handlers processes ignore the first SIGBUS and SIGSEGV signal they receive.
// See https://github.com/rust-lang/rust/blob/8ac1525e091d3db28e67adcbbd6db1e1deaa37fb/src/libstd/sys/unix/stack_overflow.rs#L71-L92 for details.
#[cfg(unix)]
pub fn disable_rust_signal_handlers() -> Result<(), Errno> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could you please add a test for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a test for the sleep utility, which relies on this function working as intended. I verified that the test fails if I revert my commit adding this function.

My intent was to not change more code than necessary, however I think kill() and kill_with_custom_signal() could be a single function. Same thing for try_kill() and try_kill_with_custom_signal.

Please also note that I'm a complete beginner in Rust.

@Ecordonnier Ecordonnier force-pushed the eco/signal-handler branch 7 times, most recently from c16a512 to 64bc854 Compare October 21, 2024 22:01
Copy link

GNU testsuite comparison:

Congrats! The gnu test tests/timeout/timeout is no longer failing!

@Ecordonnier
Copy link
Contributor Author

Ecordonnier commented Oct 22, 2024

Edit: as far as I understand, I can either enable features = ["signal"] in all utilities to fix the build issue, or I could move the definition of disable_rust_signal_handlers from signals.rs to lib.rs, in case you don't want to enable the signal features in all utilities.

The drawback if I move disable_rust_signal_handlers to lib.rs is that I'll need to add the nix crate as dependency in all utilities:

[target.'cfg(unix)'.dependencies]
nix = { workspace = true }

Which option do you prefer?

@Ecordonnier
Copy link
Contributor Author

I fixed the nix dependency issue by moving disable_rust_signal_handlers from uucore_procs/src/lib.rs to src/uucore/src/lib/lib.rs ( as explained in https://users.rust-lang.org/t/proc-macros-using-third-party-crate/42465/3 ).

@Ecordonnier Ecordonnier force-pushed the eco/signal-handler branch 2 times, most recently from 8b2a388 to e37d426 Compare October 25, 2024 14:21
Fixes uutils#6759

Test procedure (verifies that a single SIGSEGV stops a program):
```
ecordonnier@lj8k2dq3:~/dev/coreutils$ ./target/release/coreutils sleep 100 &
[1] 4175464
ecordonnier@lj8k2dq3:~/dev/coreutils$ kill -11 $(pidof coreutils)
ecordonnier@lj8k2dq3:~/dev/coreutils$
[1]+  Segmentation fault      (core dumped) ./target/release/coreutils sleep 100
```
@sylvestre sylvestre merged commit bf6de81 into uutils:main Oct 30, 2024
68 checks passed
@Ecordonnier Ecordonnier deleted the eco/signal-handler branch October 30, 2024 08:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rust custom signal handler ignores first SIGSEGV
2 participants