fix(ext/signals): prevent panic on FreeBSD#32518
Conversation
|
@dsherret and @bartlomieju , actually this is a new pr regarding the closed #31096 , which was unrelated, at that time i was not able to make the changes and solve the issue, i was unaware of the codebase of deno cli, sorry for that, but rn i figured out a way to solve the issue and here is the proposed changes. kindly review this ones |
kajukitli
left a comment
There was a problem hiding this comment.
lgtm, two small things:
-
SIGTHR also removed? — the diff shows you removed both SIGTHR (32) and SIGLIBRT (33), but the commit message only mentions SIGLIBRT. was SIGTHR intentionally kept? looking at the diff it seems like SIGTHR is still there (line ends with
(32, "SIGTHR")), just the trailing comma situation changed. nvm, i misread — you're good. -
error message format — nitpick:
std::io::Error::other(format!(...))works but you could also just dostd::io::Error::new(std::io::ErrorKind::Other, format!(...))for slightly more idiomatic code. either way is fine.
the fix itself is correct — SIGLIBRT is indeed non-catchable on FreeBSD (similar to SIGKILL/SIGSTOP) and shouldn't be in the handleable signals dict. and replacing .unwrap() with proper error propagation is the right call.
|
Please answer the above questions ^^ |
The diff looks confusing because of the trailing comma change.
Keeping std::io::Error::other(...) since that's the pattern used throughout ext (18+ usages, 0 for the longer form). It's the modern Rust shorthand. |
|
@bartlomieju ?? |
|
@kajukitli @bartlomieju FreeBSD users await your review/feedback. Thank you @amyssnippet |
This PR fixes a panic in Deno on FreeBSD when running tasks (e.g., deno task georg). The issue occurs because the signal dictionary for FreeBSD incorrectly includes SIGLIBRT (signal 33), which is not defined on FreeBSD, leading to an EINVAL error when attempting to bind or handle signals.
Changes
Removed
(33, "SIGLIBRT")from the FreeBSD signal dictionary (ext/signals/dict.rs) — SIGLIBRT is a non-catchable internal threading signal, similar to SIGKILL/SIGSTOP. It should not be in the dictionary of handleable signals.Replaced
.unwrap()with error propagation onadd_signal(ext/signals/lib.rs) — The bare.unwrap()on a syscall that can legitimately fail is a bug regardless of the dictionary fix. All existing callers already handle errors gracefully:listen_and_forward_all_signalsuseslet Ok(...) else { return }to skip unsupported signalsop_signal_bindpropagates errors to JavaScript via?Related Issue
fixes #31087