Repository navigation
Conversation
anastygnome
reviewed
Oct 18, 2026
Comment on lines
+62
to
+75
| let old_handler = unsafe { libc::signal(signal, libc::SIG_IGN) }; | ||
| if old_handler == libc::SIG_ERR { | ||
| return Err(io::Error::last_os_error()); | ||
| } | ||
| // SAFETY: the signal was validated above and group zero is our group. | ||
| let result = if unsafe { libc::kill(0, signal) } == -1 { | ||
| Err(io::Error::last_os_error()) | ||
| } else { | ||
| Ok(()) | ||
| }; | ||
| // Restore the old handler | ||
| let _ = unsafe { nix_signal::signal(signal, old_handler) }; | ||
| result.map_err(|e| io::Error::from_raw_os_error(e as i32)) | ||
| // SAFETY: old_handler was returned by a successful signal call. | ||
| let _ = unsafe { libc::signal(signal, old_handler) }; | ||
| result |
Contributor
There was a problem hiding this comment.
Do we really need unsafe here ? why not use nix ?
Comment on lines
+128
to
+134
| if unsafe { libc::sigaddset(&raw mut set, signal.as_raw()) } == -1 { | ||
| return Err(io::Error::last_os_error()); | ||
| } | ||
| // SAFETY: the set was initialized by sigemptyset and changed by sigaddset. | ||
| unsafe { SigSet::from_sigset_t_unchecked(set) } | ||
| .thread_unblock() | ||
| .map_err(Into::into) |
Comment on lines
+45
to
+57
| let signal = libc::c_int::try_from(signal) | ||
| .map_err(|_| io::Error::from_raw_os_error(libc::EINVAL))?; | ||
|
|
||
| // Signal 0 is special - it just checks if process exists, doesn't send anything. | ||
| // No need to manipulate signal handlers for it. | ||
| if signal == 0 { | ||
| return nix_signal::kill(Pid::from_raw(0), None) | ||
| .map_err(|e| io::Error::from_raw_os_error(e as i32)); | ||
| // SAFETY: process group zero denotes the caller's group; no signal | ||
| // is delivered by this existence check. | ||
| return if unsafe { libc::kill(0, 0) } == -1 { | ||
| Err(io::Error::last_os_error()) | ||
| } else { | ||
| Ok(()) | ||
| }; |
Contributor
There was a problem hiding this comment.
Why not use nix here ?
limadog9
force-pushed
the
fix/timeout-realtime-delivery-12975
branch
from
October 18, 2026 17:36
c48d867 to
644f35a
Compare
anastygnome
suggested changes
Oct 18, 2026
Comment on lines
+68
to
+91
| let old_handler = unsafe { signal::signal(signal, SigHandler::SigIgn) }?; | ||
| let result = signal::kill(Pid::from_raw(0), signal).map_err(Into::into); | ||
| // SAFETY: restore the handler returned by the successful call above. | ||
| let _ = unsafe { signal::signal(signal, old_handler) }; | ||
| return result; | ||
| } | ||
|
|
||
| // Ignore the signal temporarily so we don't receive it ourselves. | ||
| let old_handler = unsafe { nix_signal::signal(signal, SigHandler::SigIgn) } | ||
| .map_err(|e| io::Error::from_raw_os_error(e as i32))?; | ||
| let result = nix_signal::kill(Pid::from_raw(0), Some(signal)); | ||
| // nix cannot represent real-time signals in its Signal enum. | ||
| // SAFETY: SIG_IGN is a valid handler; signal validates the raw number. | ||
| let old_handler = unsafe { libc::signal(signal, libc::SIG_IGN) }; | ||
| if old_handler == libc::SIG_ERR { | ||
| return Err(io::Error::last_os_error()); | ||
| } | ||
| // SAFETY: the signal was validated above and group zero is our group. | ||
| let result = if unsafe { libc::kill(0, signal) } == -1 { | ||
| Err(io::Error::last_os_error()) | ||
| } else { | ||
| Ok(()) | ||
| }; | ||
| // Restore the old handler | ||
| let _ = unsafe { nix_signal::signal(signal, old_handler) }; | ||
| result.map_err(|e| io::Error::from_raw_os_error(e as i32)) | ||
| // SAFETY: old_handler was returned by a successful signal call. | ||
| let _ = unsafe { libc::signal(signal, old_handler) }; | ||
| result |
Contributor
There was a problem hiding this comment.
there's quite a lot of duplication there and I believe you can use the safe signal code here instead of unsafe (just need a cast for kill; which is fine)
| return Err(io::Error::last_os_error()); | ||
| } | ||
| // SAFETY: the set was initialized by sigemptyset and changed by sigaddset. | ||
| unsafe { SigSet::from_sigset_t_unchecked(set) } |
timeout accepted RTMIN/RTMAX names but ChildExt rejected their signal numbers, causing unnecessary KILL escalation. Send raw signal numbers to processes and process groups, and unblock real-time signals safely when preserving a child's termination. Add delivery and exit-signal regressions. Validation: 35 timeout integration tests pass, including real-time signal delivery and preservation of the child's exit signal. Closes uutils#12975
limadog9
force-pushed
the
fix/timeout-realtime-delivery-12975
branch
from
October 18, 2026 18:39
644f35a to
e82dbdd
Compare
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
timeout accepted RTMIN/RTMAX names but ChildExt rejected their signal numbers, causing unnecessary KILL escalation. Send raw signal numbers to processes and process groups, and unblock real-time signals safely when preserving a child's termination. Add delivery and exit-signal regressions.
Validation: 35 timeout integration tests pass, including real-time signal delivery and preservation of the child's exit signal.
Closes #12975