fix(ping): make IPv4 and IPv6 ICMP polling safe - #619
Open
somethingwithproof wants to merge 19 commits into
Open
fix(ping): make IPv4 and IPv6 ICMP polling safe#619somethingwithproof wants to merge 19 commits into
somethingwithproof wants to merge 19 commits into
Conversation
Member
Author
|
@copilot review |
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
It changes privilege/locking behavior in a security-sensitive (SUID-root) ICMP path, which warrants final human audit despite strong unit coverage.
Pull request overview
This PR hardens Spine’s ICMP ping path by making raw-socket privilege handling and resource cleanup deterministic, and by ensuring resolver failures don’t corrupt caller-owned state. It adds a focused unit test suite to cover the previously fragile/error-prone ICMP ownership and retry logic.
Changes:
- Refactors
ping_icmp()to use a single cleanup path and to avoid retry deadlocks by releasingLOCK_SETEUIDbefore sleeping/retrying. - Fixes
init_sockaddr()to avoid freeing indeterminategetaddrinfo()output by initializinghostinfoand only freeing on success. - Introduces
set.icmp_uses_capsto cache Linux capability mode decisions and adds a dedicated ICMP unit test binary.
File summaries
| File | Description |
|---|---|
| util.c | Ensures capability handles are freed on all paths; records whether ICMP uses Linux capabilities. |
| ping.c | Centralizes ICMP resource cleanup, fixes retry deadlock risk around LOCK_SETEUID, and makes resolver failure handling safe. |
| spine.h | Adds icmp_uses_caps config field to track capability-based ICMP operation. |
| spine.c | Initializes set.icmp_uses_caps at startup. |
| tests/unit/test_ping_icmp.c | Adds deterministic ICMP ownership/cleanup/retry/lock-balance unit tests by intercepting sinks. |
| tests/unit/test_linked.c | Adds portability shim for ICMP unreachable constant naming. |
| Makefile.am | Registers the new ICMP unit test program in check_PROGRAMS. |
| CHANGELOG | Documents the ICMP/resolver/capability safety fixes. |
Review details
- Files reviewed: 8/8 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Member
Author
|
@copilot review |
A bare IPv6 literal never reached an ICMP socket, so spine reported the device as unpingable instead of probing it. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Spine asked only for SOCK_RAW and treated ICMP as unavailable whenever it was neither root nor capability-endowed, so net.ipv4.ping_group_range had no effect however the sysctl was set. Both the availability probe and the two ping paths now try SOCK_DGRAM first and fall back to the raw socket, which also means the IPv4 receive path has to allow for the stripped header and the IPv6 one for the kernel-assigned echo id. Reported in Cacti#250. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
…sifier The target was dropped between Cacti#542 and Cacti#563: it could not go with the test split because it calls spine_icmp_classify_reply(), and the rebuild of Cacti#542 carried source files only. An input byte now selects which classifier runs, so the datagram path added here is fuzzed alongside the raw one. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Six exits each released the two resources on their own terms and had drifted: the FD_SETSIZE guard closed the socket and returned without freeing the packet, leaking packet_len bytes per affected device per cycle. Closes Cacti#593. One cleanup label now. Four frees and five closes become one of each, which is what makes the next early return safe by construction rather than by review. Three of those exits also re-entered root and took LOCK_SETEUID to call close(), which needs no privileges. The raw socket needs root to open, which happens once above and drops immediately. Removing that shortens the elevated window and stops every poller thread serialising on a global mutex to close a descriptor. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
The review named this the least-tested and highest-consequence change in the branch, and it was right: ping_icmp() runs in a SUID-root binary, its five exits were collapsed onto one cleanup label, and the seteuid(0) wrapper around close() was removed, with nothing covering any of it. A raw ICMP socket is available under docker, so the function is reachable. The tests skip rather than fail where it is not. The third case drives the exit that leaked: hold descriptors until a new socket would land at or above FD_SETSIZE, then ping. Against the ping.c from before ff2c252, LeakSanitizer reports 31 bytes leaked in 1 allocation. Against this one it is clean. That is Cacti#593 demonstrated rather than argued. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
The retry slept and looped back still holding the lock, so the second attempt relocked a non-recursive process-global mutex from its own owner and wedged the thread at euid 0 with every other poller thread behind it. Each attempt is now a complete lock/elevate/open/drop/unlock block, which also narrows the elevated window to the socket() call. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
getaddrinfo() leaves the result pointer unspecified on error, so the five failure paths in init_sockaddr() were passing whatever the stack held to freeaddrinfo(). Initialise it and drop those frees, since nothing was allocated. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Map the BSD ICMP_UNREACH name to the Linux ICMP_DEST_UNREACH spelling used by the linked-object regression test so the focused branch remains portable. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Keep each IPv6 raw-socket retry inside one cached capability and privilege-transition decision, matching the consolidated IPv4 path. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Build the controlled reply in the format selected by the socket type so the consolidated unprivileged ICMP path exercises its successful cleanup contract. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
somethingwithproof
force-pushed
the
fix/597-icmp-resolver-safety
branch
from
September 6, 2026 07:18
0084b8e to
b06dd8e
Compare
somethingwithproof
changed the title
This was referenced Sep 6, 2026
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
Consolidates the overlapping ICMP work from #555 and #619 into one review of the complete socket and resolver state machine.
The consolidation also fixed an integration defect found locally: the controlled successful-reply test constructed only a raw IPv4 packet even when the production path selected SOCK_DGRAM. It now models the selected socket format, and IPv6 raw retries use the same privilege-safety invariant as IPv4.
Verification
Closes #127.
Closes #250.
Closes #593.
Closes #611.
Supersedes #555.
Refs #597 and #618.