fix(ping): ping IPv6 devices with ICMPv6 echo requests - #555
fix(ping): ping IPv6 devices with ICMPv6 echo requests#555somethingwithproof wants to merge 3 commits into
Conversation
f0abd52 to
521204f
Compare
Poller correctness and memory-safety fixes, with the ICMP hardening from #543 folded in. Verified in a clean `ubuntu:24.04` container: builds at **38 warnings** against develop's 39, and the unit tests build and pass on Linux (39 assertions across three binaries). **Fixes** - `php_readpipe` off-by-one — the old bound let `bptr` reach `result_string + 2048` and then wrote the terminator one byte past a 2048-byte allocation. - `get_namebyhost` used `strtok`, which keeps one process-wide save pointer, and runs unlocked on every poller thread. Two devices could be polled at each other's addresses. Now `strtok_r`. - The log date format was read from `default_dateformat`; Cacti's key is `default_date_format`, so the lookup never matched and the format was never loaded. - `db_query` returns `mysql_store_result()` directly, which is NULL on store failure. The NULL check ran *after* `mysql_fetch_row`, so the crash happened one line before the guard. - `snmp_count` logged a Net-SNMP `errstat` without setting either loop flag, rebuilding the identical GETNEXT forever and pinning a thread until the poll interval ended. - `php_close` skipped the `close()` when the quit write failed with EPIPE, leaking a descriptor per script-server restart. - The reap loop's delay was compiled out on `SOLAR_THREAD` builds, so both phases burned twenty `WNOHANG` polls in nanoseconds and the child was never reaped. - ICMP replies are validated for length and matched on id/sequence; on Cygwin the peeked datagram is now drained, which previously busy-spun until host timeout. - The fatal signal handler no longer calls `malloc`/`localtime_r`/`fprintf` before `_exit` — on a SIGSEGV caused by heap corruption that deadlocked on the allocator's lock instead of dying. - The unit test Makefile was hardcoded to `/opt/homebrew` paths and `-std=gnu23`, so it could not build on Linux at all. Now pkg-config driven. Supersedes #543. **Added since the first revision** - `snmp_get_multi()` now checks the varbind type before formatting. Under v2c an agent reports a per-OID failure as an inline exception varbind while the PDU `errstat` stays `NOERROR`, so `snprint_value()` rendered the exception as literal text and stored it as a value. - Two warning sources removed: a dangling line continuation ending `sql.h`, and `strncopy()` now using `memcpy` where the byte count is already exact. Warnings drop from **39 to 7**. Closes #551 Closes #232 - `read_config_options()` issued 23 separate `SELECT ... FROM settings WHERE name = ?` round trips at startup; they are now one bulk read served from memory. Call sites are unchanged. Closes #330 **Test infrastructure** - The SNMPv3 integration fixture was broken: the image shipped a binary that could not start (`libmysqlclient.so.21: cannot open shared object file`). The build context carried `*.o`, `spine`, `Makefile` and `config.status`, so `make` treated them as current, skipped compiling, and packaged a binary linked against libraries the runtime image does not have. `.dockerignore` now excludes build output. The fixture runs end to end: spine polls the SNMPv3 device, reads `uptime`, writes to `poller_output` and exits 0. - Two libFuzzer targets, run per PR and weekly: the host string parser and the ICMP reply parser. The second drives `spine_icmp_classify_reply()`, extracted from `ping_icmp` so the bounds walk over a shared raw socket is reachable by a test. 15M executions under ASan/UBSan, no crashes. **Added in this revision** - `output_regex` was applied only in the in-loop OID drain, not the pass after the loop that handles whatever did not fill a `MAX_OIDS` batch, so a device whose OID count missed a boundary stored raw values. A single data source is always the leftover case. `tests/integration/test_output_regex.sh` now asserts on the value: it fails on the parent commit with the raw reading and passes here with the filtered one. Addresses #210; the feature stays dormant until Cacti populates `poller_item.output_regex`. Merge before #555, which needs the `get_namebyhost` fix above to resolve a device hostname at all. --------- Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
521204f to
a5fe36f
Compare
|
Rebased onto develop, conflicts resolved. The conflict came from #542 landing Also restored Verified in a clean |
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>
a5fe36f to
5be9c78
Compare
|
Conflicts are resolved; this merges cleanly against develop now. |
|
The merge conflict this was blocked on is gone. GitHub agrees: One thing worth knowing before this is mergedIt conflicts with #597, in Both change Whichever lands first, the other needs a rebase. I would rather that be this one, since it is the smaller rebase and #597 is the one carrying the larger diff. Happy to do it either way, or to rebase this onto #597 now if you would prefer them ordered. |
|
Coordination update: dependency #542 merged on 2026-08-31, and GitHub now |
|
Rebased onto develop. Conflicts are resolved and the branch shows mergeable. |
There was a problem hiding this comment.
🟡 Changes recommended
The ICMPv6 implementation in ping.c is currently compiled out because SPINE_HAVE_ICMPV6 is only defined in ping.h (which ping.c does not include), so the core IPv6 feature won’t be built as-is.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR extends Spine’s ping implementation to support IPv6 targets when using the ICMP method (ICMPv6 echo requests), and updates the ICMP availability logic to work for unprivileged users on Linux via net.ipv4.ping_group_range (preferring SOCK_DGRAM ICMP sockets before falling back to raw sockets).
Changes:
- Add an ICMPv6 ping path and dispatch to it from
ping_host()when the resolved address is IPv6 and the ping method is ICMP. - Prefer unprivileged
SOCK_DGRAMICMP sockets (IPv4 + IPv6) when possible, improving behavior underping_group_range. - Add integration coverage for unprivileged ICMP behavior and a new fuzz target for ICMP reply classification.
File summaries
| File | Description |
|---|---|
| util.c | Probes unprivileged ICMP datagram socket availability before disabling ICMP for non-root users. |
| ping.c | Adds ICMP datagram reply classifier, tries SOCK_DGRAM before raw sockets, and implements ICMPv6 echo pinging. |
| ping.h | Introduces SPINE_HAVE_ICMPV6 gating and exports the datagram-reply classifier prototype. |
| common.h | Adds conditional includes for net/if.h and ifaddrs.h used by IPv6 scope-id helper. |
| configure.ac | Adds header/function checks for ifaddrs / interface-index helpers. |
| tests/integration/test_unprivileged_icmp.sh | New integration test covering ICMP availability across sysctl + privilege combinations. |
| tests/fuzz/Makefile | Builds the new ICMP reply fuzz target. |
| tests/fuzz/fuzz_icmp_reply.c | New libFuzzer target for raw vs datagram ICMP reply classifiers. |
| tests/fuzz/corpus/fuzz_icmp_reply/c4f87a6290aee1acfc1f26083974ce94621fca64 | Seed corpus input for the new fuzz target. |
Review details
Suppressed comments (1)
tests/fuzz/fuzz_icmp_reply.c:45
- The relational pointer comparisons here are only defined when both pointers refer into the same array object. If a classifier bug returns a pointer outside the input buffer (what this fuzzer is trying to detect), these comparisons can become undefined behavior and mask the bug. Compare as integers instead.
if (verdict == SPINE_ICMP_REPLY_OK) {
if (pkt == NULL) abort();
if ((const uint8_t *) pkt < data) abort();
if ((const uint8_t *) pkt + ICMP_HDR_SIZE > data + size) abort();
} else if (pkt != NULL) {
- Files reviewed: 9/19 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| #include <stdint.h> | ||
| #include <string.h> | ||
|
|
||
| #include "common.h" | ||
| #include "spine.h" | ||
| #include "ping.h" |
| #include "common.h" | ||
| #include "spine.h" | ||
|
|
||
| #ifdef SPINE_HAVE_ICMPV6 | ||
| static int ping_icmp_ipv6(host_t *host, ping_t *ping); |
Spine opens its ICMP socket as
AF_INET/SOCK_RAWonly, so a device with an IPv6 address never reaches a working code path and reportsICMP: Destination hostname invalid. Reported in #127 in December 2019.This adds an ICMPv6 echo path and dispatches to it in
ping_host()when the address resolves to IPv6 and the ping method is ICMP. IPv4 is untouched:ping_icmp(),ping_udp()andping_tcp()are unmodified, and IPv6 with a UDP or TCP ping method still falls through to the existing message.Verified in a clean ubuntu:24.04 container at 39 warnings, matching develop.
Evidence. The ping path was extracted verbatim into a harness that stubs only the spine runtime, then run against real addresses:
The second line is the reporter's exact case: the literal now produces a real echo request and an honest timeout instead of being rejected as an invalid hostname.
Portability. Everything sits behind
SPINE_HAVE_ICMPV6, defined only whenAF_INET6,IPPROTO_ICMPV6,ICMP6_FILTER,ICMP6_ECHO_REQUESTandICMP6_ECHO_REPLYare all present and the target is not Cygwin, matching howcommon.halready excludes<netinet/icmp6.h>there. Without it the file compiles exactly as before. Fourconfigure.acchecks were added for the scope-id helper (getifaddrs,if_nametoindex).Note for #523. The version of this code on that branch sets
sin6_portto 7 on the destinationsockaddr_in6. On a raw IPv6 socket that field is the protocol number, not a port, so Linux rejects everysendto()withEINVAL— it fails on the first send. Fixed here by setting it to 0. The same bug is on thefeat/spine-2.0-behavioralbranch.Closes #127
Added in this revision
cap_net_rawin two places:set.icmp_availinutil.c, which diverts to UDP beforeping_icmp()runs, and the socket call itself. Both now trySOCK_DGRAMfirst and fall back to the raw socket, sonet.ipv4.ping_group_rangedecides the outcome. The IPv4 receive path allows for the header the kernel strips on a datagram socket, and the IPv6 reply match drops the echo id, which the kernel reassigns, keeping sequence, source address and payload as the discriminators.tests/integration/test_unprivileged_icmp.shcovers all three privilege and sysctl combinations. Closes Support kernel parameter ping_group_range allowing unprivileged users to use ICMP (ping) #250.Depends on #542:
get_namebyhosttruncates its input by one byte on this branch, so192.168.166.2resolves as192.168.166.and no ICMP ping succeeds until that fix lands. Merge #542 first.