Use `libc::sigaction()` instead of `sys::signal()` to prevent a deadlock #88828

FabianWolff · 2021-09-10T17:56:08Z

Fixes #88585. POSIX specifies that after forking,

to avoid errors, the child process may only execute async-signal-safe operations until such time as one of the exec functions is called.

Rust's standard library does not currently adhere to this, as evidenced by #88585. The child process calls sys::signal(), which on Android calls libc::dlsym(), which is not async-signal-safe, and in fact causes a deadlock in the example in #88585.

I think the easiest solution here would be to just call libc::sigaction() instead, which is async-signal-safe, provides the functionality we need, and is apparently available on all Android versions because it is also used e.g. here.

rust-highfive · 2021-09-10T17:56:11Z

r? @dtolnay

(rust-highfive has picked a reviewer for you, use r? to override)

hacker-volodya · 2021-09-15T13:40:12Z

I've tested the fix on Android Emulator x86-64 (Android 10), and on real devices (Android from 6.0.1 to 11). It works!

Locking g_dl_mutex...
Spawning ps...
ps spawned!
USER            PID   PPID     VSZ    RSS WCHAN            ADDR S NAME
shell          5002  25748 10790496  2692 futex_wait_queue_me 0 S linker-deadlock
shell          5005   5002 10774564  3368 0                   0 R ps
dlsym'ing done
Done

FabianWolff · 2021-09-15T16:29:29Z

I've tested the fix on Android Emulator x86-64 (Android 10), and on real devices (Android from 6.0.1 to 11). It works!

Happy to hear that! And thanks a lot for testing this @d3fl4t3!

dtolnay

Thanks! This looks good.

library/std/src/sys/unix/process/process_unix.rs

The reference automatically coerces to a pointer. Writing an explicit cast here is slightly misleading because that's most commonly used when a pointer needs to be converted from one pointer type to another, e.g. `*const c_void` to `*const sigaction` or vice versa.

dtolnay · 2021-09-29T04:23:10Z

@bors r+

bors · 2021-09-29T04:23:11Z

📌 Commit e3e5ae9 has been approved by dtolnay

Use `libc::sigaction()` instead of `sys::signal()` to prevent a deadlock Fixes rust-lang#88585. POSIX [specifies](https://man7.org/linux/man-pages/man3/fork.3p.html) that after forking, > to avoid errors, the child process may only execute async-signal-safe operations until such time as one of the exec functions is called. Rust's standard library does not currently adhere to this, as evidenced by rust-lang#88585. The child process calls [`sys::signal()`](https://github.com/rust-lang/rust/blob/7bf0736e130e2203c58654f7353dbf9575e49d5c/library/std/src/sys/unix/android.rs#L76), which on Android calls [`libc::dlsym()`](https://github.com/rust-lang/rust/blob/7bf0736e130e2203c58654f7353dbf9575e49d5c/library/std/src/sys/unix/weak.rs#L101), which is [**not**](https://man7.org/linux/man-pages/man7/signal-safety.7.html) async-signal-safe, and in fact causes a deadlock in the example in rust-lang#88585. I think the easiest solution here would be to just call `libc::sigaction()` instead, which [is](https://man7.org/linux/man-pages/man7/signal-safety.7.html) async-signal-safe, provides the functionality we need, and is apparently available on all Android versions because it is also used e.g. [here](https://github.com/rust-lang/rust/blob/7bf0736e130e2203c58654f7353dbf9575e49d5c/library/std/src/sys/unix/stack_overflow.rs#L112-L114).

Manishearth · 2021-09-30T20:23:13Z

@bors r-

#89402 (comment)

warning: dropping unsupported crate type `dylib` for target `x86_64-unknown-redox`

error[E0609]: no field `sa_sigaction` on type `sigaction`
    |
    |
337 |             action.sa_sigaction = libc::SIG_DFL;
    |
    |
    = note: available fields are: `sa_handler`, `sa_flags`, `sa_restorer`, `sa_mask`
For more information about this error, try `rustc --explain E0609`.
[RUSTC-TIMING] std test:false 2.497
warning: `std` (lib) generated 1 warning

FabianWolff · 2021-10-01T19:31:43Z

So neither .sa_sigaction nor .sa_handler are universally available, and there doesn't seem to be an easy way to detect which platform uses which field name. I have therefore restricted my original fix to just Android for now (which is the platform that #88585 was discovered on).

@rustbot ready

dtolnay · 2021-10-03T04:35:13Z

@bors r+

bors · 2021-10-03T04:35:14Z

📌 Commit 65ef265 has been approved by dtolnay

Use `libc::sigaction()` instead of `sys::signal()` to prevent a deadlock Fixes rust-lang#88585. POSIX [specifies](https://man7.org/linux/man-pages/man3/fork.3p.html) that after forking, > to avoid errors, the child process may only execute async-signal-safe operations until such time as one of the exec functions is called. Rust's standard library does not currently adhere to this, as evidenced by rust-lang#88585. The child process calls [`sys::signal()`](https://github.com/rust-lang/rust/blob/7bf0736e130e2203c58654f7353dbf9575e49d5c/library/std/src/sys/unix/android.rs#L76), which on Android calls [`libc::dlsym()`](https://github.com/rust-lang/rust/blob/7bf0736e130e2203c58654f7353dbf9575e49d5c/library/std/src/sys/unix/weak.rs#L101), which is [**not**](https://man7.org/linux/man-pages/man7/signal-safety.7.html) async-signal-safe, and in fact causes a deadlock in the example in rust-lang#88585. I think the easiest solution here would be to just call `libc::sigaction()` instead, which [is](https://man7.org/linux/man-pages/man7/signal-safety.7.html) async-signal-safe, provides the functionality we need, and is apparently available on all Android versions because it is also used e.g. [here](https://github.com/rust-lang/rust/blob/7bf0736e130e2203c58654f7353dbf9575e49d5c/library/std/src/sys/unix/stack_overflow.rs#L112-L114).

…arth Rollup of 10 pull requests Successful merges: - rust-lang#88706 (Normalize associated type projections when checking return type of main) - rust-lang#88828 (Use `libc::sigaction()` instead of `sys::signal()` to prevent a deadlock) - rust-lang#88871 (Fix suggestion for nested struct patterns) - rust-lang#89317 (Move generic error message to separate branches) - rust-lang#89351 (for signed wrapping remainder, do not compare lhs with MIN) - rust-lang#89442 (Add check for duplicated doc aliases) - rust-lang#89502 (Fix Lower/UpperExp formatting for integers and precision zero) - rust-lang#89523 (Make `proc_macro_derive_resolution_fallback` a future-breakage lint) - rust-lang#89532 (Document behavior of `MaybeLiveLocals` regarding enums and field-senstivity) - rust-lang#89546 (Make an initial guess for metadata size to reduce buffer resizes) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup

rust-highfive assigned dtolnay Sep 10, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 10, 2021

This comment has been minimized.

Sign in to view

Use libc::sigaction() instead of sys::signal() to prevent a deadlock

f1c8acc

FabianWolff force-pushed the issue-88585 branch from a6c525d to f1c8acc Compare September 10, 2021 19:02

dtolnay reviewed Sep 29, 2021

View reviewed changes

library/std/src/sys/unix/process/process_unix.rs Outdated Show resolved Hide resolved

dtolnay force-pushed the issue-88585 branch from 7fb9162 to e3e5ae9 Compare September 29, 2021 04:22

dtolnay approved these changes Sep 29, 2021

View reviewed changes

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 29, 2021

Manishearth mentioned this pull request Sep 30, 2021

Rollup of 10 pull requests #89402

Closed

bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Sep 30, 2021

Call libc::sigaction() only on Android

65ef265

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 1, 2021

dtolnay approved these changes Oct 3, 2021

View reviewed changes

bors removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 3, 2021

bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Oct 3, 2021

Manishearth mentioned this pull request Oct 5, 2021

Rollup of 7 pull requests #89560

Closed

Manishearth mentioned this pull request Oct 5, 2021

Rollup of 10 pull requests #89572

Merged

bors merged commit eb86098 into rust-lang:master Oct 6, 2021

rustbot added this to the 1.57.0 milestone Oct 6, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use `libc::sigaction()` instead of `sys::signal()` to prevent a deadlock #88828

Use `libc::sigaction()` instead of `sys::signal()` to prevent a deadlock #88828

FabianWolff commented Sep 10, 2021 •

edited

Loading

rust-highfive commented Sep 10, 2021

This comment has been minimized.

hacker-volodya commented Sep 15, 2021

FabianWolff commented Sep 15, 2021

dtolnay left a comment

dtolnay commented Sep 29, 2021

bors commented Sep 29, 2021

Manishearth commented Sep 30, 2021

FabianWolff commented Oct 1, 2021

dtolnay commented Oct 3, 2021

bors commented Oct 3, 2021

Use libc::sigaction() instead of sys::signal() to prevent a deadlock #88828

Use libc::sigaction() instead of sys::signal() to prevent a deadlock #88828

Conversation

FabianWolff commented Sep 10, 2021 • edited Loading

rust-highfive commented Sep 10, 2021

This comment has been minimized.

hacker-volodya commented Sep 15, 2021

FabianWolff commented Sep 15, 2021

dtolnay left a comment

Choose a reason for hiding this comment

dtolnay commented Sep 29, 2021

bors commented Sep 29, 2021

Manishearth commented Sep 30, 2021

FabianWolff commented Oct 1, 2021

dtolnay commented Oct 3, 2021

bors commented Oct 3, 2021

Use `libc::sigaction()` instead of `sys::signal()` to prevent a deadlock #88828

Use `libc::sigaction()` instead of `sys::signal()` to prevent a deadlock #88828

FabianWolff commented Sep 10, 2021 •

edited

Loading