fix(emerg-shutdown): close leaked input-device fds and swallow SIGSYS#370
Open
maybebyte wants to merge 2 commits into
Open
fix(emerg-shutdown): close leaked input-device fds and swallow SIGSYS#370maybebyte wants to merge 2 commits into
maybebyte wants to merge 2 commits into
Conversation
The input-device enumeration loop opens a descriptor for every /dev/input/eventN but only stores it for devices that support the panic-key combo. The two early-continue paths -- non-key devices and keyboards lacking the panic keys -- returned without closing tmp_fd, leaking one descriptor per non-qualifying device. The leak is bounded and one-time (the loop runs once at startup, not in the poll loop) so impact is low, but the descriptors are held for the whole process lifetime and never reclaimed -- the daemon never execs, so O_CLOEXEC does not cover them. Close tmp_fd before both continues.
max_sig_num is 31, but the SIG_IGN loop used a strict "<" bound, so it covered signals 1..30 and never touched signal 31 (SIGSYS on Linux). The real-time loop starts at SIGRTMIN, leaving SIGSYS covered by neither loop, so it kept its default terminate disposition despite the "Swallow all signals that we can." intent. Use "<=" so the loop reaches 31; SIGSYS is ignorable, and the bound still stops before the glibc-reserved signals 32 and 33. This is a robustness fix, not a hardening gain: SIGKILL and SIGSTOP are unblockable anyway, so an actor able to send SIGSYS can already kill the monitor -- the value is surviving an internally generated SIGSYS (e.g. a bad syscall).
Contributor
Author
|
AI assisted. |
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
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.
Summary
Two independent, low-severity correctness fixes in the
emerg-shutdownpanic-key monitor (the statically-linked C daemon that watches input devices
and triggers emergency shutdown): a file-descriptor leak during device
enumeration, and a signal-ignore loop that misses SIGSYS.
Changes
opens a descriptor for every
/dev/input/eventNbut only stores it fordevices that support the panic-key combo. The two early-
continuepaths —non-key devices and keyboards lacking the panic keys — returned without
closing
tmp_fd, leaking one descriptor per non-qualifying device. Theleak is bounded and one-time (the loop runs once at startup, not in the
poll loop), but the fds are held for the daemon's lifetime and never
reclaimed — the daemon never
execs, soO_CLOEXECdoes not cover them.max_sig_numis 31, but theSIG_IGNloop used a strict<, covering signals 1–30 and never touchingsignal 31 (SIGSYS on Linux). The real-time loop starts at
SIGRTMIN, soSIGSYS was ignored by neither and kept its default terminate disposition,
contrary to the "Swallow all signals that we can." intent. Changed
<to<=; the loop still stops before the glibc-reserved signals 32/33.Testing
compile-emerg-shutdownafter each commit —full hardening set (
-Wall -Wextra -Werror=implicit -Wshadow … -static),clean build, valid ELF, no warnings-as-errors.
-Werror=implicitconfirmsclose()is properly declared (<unistd.h>is already included).SIGSYS and is ignorable (
sigaction(SIGSYS, SIG_IGN)returns 0), while 32and 33 return
EINVAL, so<=correctly stops at 31.Notes for reviewers
(a handful of fds, once, well under
RLIMIT_NOFILE); the SIGSYS change is arobustness/intent-alignment fix, not a hardening gain — SIGKILL and SIGSTOP
are unblockable anyway, so anyone able to send SIGSYS can already kill the
monitor. Its value is surviving an internally generated SIGSYS (e.g. a bad
syscall).