feat(netprobe): IPv6 (ICMPv6) ping support (#624)#785
Draft
leoparente wants to merge 15 commits into
Draft
Conversation
…and IPv6 family selection
…ore socket create
…sarial review) Fixes blockers found reviewing the branch against develop: - Heap corruption: the v6 receive lambda built a freestanding ICMPv6EchoLayer over the PingReceiver::_array6 std::array; pcpp ~Layer() does `if (!isAllocatedToPacket()) delete[] m_Data`, so it delete[]'d non-heap storage on every reply. Replaced with build_icmpv6_reply_carrier(), which synthesizes an IPv6 header so a self-owning pcpp::Packet parses IPv6->ICMPv6. - Every v6 reply silently dropped: the carrier Packet used the default ETHERNET link type, so the fan-out deep-copy re-parsed the 8-byte buffer as Ethernet (PayloadLayer), and getLayerOfType<ICMPv6EchoLayer>() returned null in the handler -> 100% timeouts. The DLT_RAW1 + synthetic IPv6 header now survives the copy. Added a deterministic test that deep-copies the carrier and asserts the layer + id/seq survive (the prior tests bypassed this by calling the handler with a live layer). - v4 regression: _get_addr set _ip_set on DNS targets, pinning them to their first resolution; develop re-resolved each interval. Removed _ip_set from the DNS branches so round-robin/failover/short-TTL names re-resolve. Minors: zero-init _sa/_sa6 (link-local scope_id), null-check spdlog::get, clarify the host-order id/seq comment. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Windows raw sockets do not auto-fill the mandatory ICMPv6 checksum (Linux/macOS kernels do), so v6 echo requests would egress with checksum 0 and be dropped. Enable IPV6_CHECKSUM (offset 2) on the v6 send socket on Windows so the kernel computes it over the IPv6 pseudo-header. POSIX has this implicitly on for IPPROTO_ICMPV6 and rejects setting it, so the call is #ifdef _WIN32 only. Must be verified end-to-end on Windows (incl. inbound raw ICMPv6 delivery). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ernel Re-verification (RFC 3542 sec 3.1) showed the IPPROTO_ICMPV6 raw-socket kernel computes and inserts the mandatory ICMPv6 checksum unconditionally on BOTH POSIX and Windows, and setting IPV6_CHECKSUM on such a socket fails — so the previous setsockopt was a no-op with an inverted-premise comment. Removed it and replaced with an accurate note; _send_icmp_v6 correctly leaves the checksum 0 for the kernel to fill. The Windows v6 send/recv path still requires real-Windows verification (inbound raw ICMPv6 delivery). Also add <cstring> for memset/memcpy. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
LCOV of commit
|
Contributor
Author
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8ae7a15544
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Pull request overview
Adds IPv6 (ICMPv6) ping support to the NetProbe input while aligning ping/TCP DNS resolution behavior and extending TCP probes to support IPv6, with deterministic unit tests covering config validation and request/reply matching.
Changes:
- Implement ICMPv6 send/receive paths in
PingProbe(dual send sockets + dual receive sockets) and add a helper to safely parse raw-socket ICMPv6 replies. - Add per-target
ip_version(4|6) config parsing (with validation/conflict checks) and thread it through DNS resolution andTcpProbe. - Add unit tests for
ip_versionvalidation, ICMPv4/ICMPv6 matching, and ICMPv6 reply packet deep-copy behavior.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/inputs/netprobe/test_netprobe.cpp | Adds deterministic tests for ip_version validation and ICMPv6 reply-carrier deep-copy behavior. |
| src/inputs/netprobe/TcpProbe.h | Extends TcpProbe to accept an optional IPv6/IPv4 force flag. |
| src/inputs/netprobe/TcpProbe.cpp | Applies forced-family DNS resolution behavior for TCP probes. |
| src/inputs/netprobe/PingProbe.h | Adds ICMPv6 parsing helper declaration and dual-socket receiver/sender state. |
| src/inputs/netprobe/PingProbe.cpp | Implements ICMPv6 receive socket + parsing carrier, ICMPv6 send path, and DNS-family selection. |
| src/inputs/netprobe/NetProbeInputStream.h | Refactors target storage and adds per-target forced IP-family metadata for DNS targets. |
| src/inputs/netprobe/NetProbeInputStream.cpp | Parses ip_version, enforces literal conflicts, passes forced family into probes, adjusts info reporting. |
| src/inputs/netprobe/NetProbe.h | Fixes DNS address iteration and adds IPv4-preferred/forced-family selection behavior. |
| src/handlers/netprobe/test_net_probe.cpp | Adds unit tests for ICMPv4 and ICMPv6 request/reply matching behavior. |
| src/handlers/netprobe/NetProbeStreamHandler.h | Declares ICMPv6 processing entrypoint in the metrics manager. |
| src/handlers/netprobe/NetProbeStreamHandler.cpp | Routes ICMPv6 echo layers and implements request/reply accounting for ICMPv6. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Addresses two PR review findings: - IPv6 must not fall back to SOCK_DGRAM. The shared PingReceiver only polls a raw _sock6, and a datagram ICMPv6 socket delivers echo replies to the sending socket (which nothing reads) rather than the receiver — so every v6 reply was silently lost and all probes timed out. Drop the DGRAM fallback on both the receive socket (_setup_receiver) and the send socket (_create_socket, gated on !_is_ipv6); v6 now fails cleanly with a socket error / disabled-IPv6 warning when raw-socket privilege (CAP_NET_RAW/root) is unavailable. ICMP6_FILTER is now unconditional since _sock6 is always RAW. - The v6 poll handler drained the socket in a while-loop; under a reply flood that could starve the v4 poll/timer on the same receiver thread. Read a single datagram per (level-triggered) poll event — uvw re-fires while data remains — matching the v4 path's one-read-per-event behavior. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
Author
|
@codex review |
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
…'t arm Addresses a PR review finding plus an adjacent send/receive consistency gap of the same class (v6 looks active but silently never works): - PingReceiver::_setup_receiver: if the uvw poll handle for _sock6 cannot be created or armed, the previous code left _sock6 open with no handler, yet v6_active() (which only checks _sock6) still reported IPv6 as working — so replies were never read and every v6 probe silently timed out, leaking the fd for the receiver's lifetime. Now gate readiness on poll_handle::start()'s return (resource<>() already runs init(), so the explicit init() was redundant and is dropped) and, on failure, warn + close + invalidate _sock6 so v6_active() returns false and IPv6 ping is cleanly disabled. The destructor already guards both _poll6 and _sock6, so this is double-close-safe. - PingProbe::_create_socket: the v6 send socket is independent of the receiver's raw ICMPv6 socket, and nothing previously checked the two agreed. If the receiver had no v6 socket but a send socket could still open, echoes were sent that could never be matched. Return SocketError when _is_ipv6 && !receiver_v6_active() so the failure surfaces instead of timing out forever. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0a050b75b9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+74
to
+76
| if (first_match || ipv4) { | ||
| for (auto addr = base; addr != nullptr; addr = addr->ai_next) { | ||
| if (addr->ai_family == AF_INET) { |
There was a problem hiding this comment.
When ip_version is unset, TcpProbe still passes first_match=true, but this branch now scans the whole getaddrinfo result for an IPv4 address before considering any IPv6 address. For dual-stack DNS names where the OS resolver policy returns AAAA first, existing TCP probes will silently switch to IPv4 and can report failures even though the resolver-selected IPv6 endpoint is healthy; the unforced path should preserve result order and only do family-specific passes when a version is requested.
Useful? React with 👍 / 👎.
…tion Fixes three pre-existing (v4-era) defects in the shared ping receiver that the IPv6 work exposed. Verified by adversarial review (no deadlock, no use-after- close, v4 path preserved end-to-end): 1. Send-socket fd leak. _sock/_sock6 are static thread_local and are opened in _create_socket(), which runs in the interval-timer callback on the io thread. But _close_socket() ran from stop() on the control thread, where those thread-locals are still INVALID_SOCKET — so the real raw descriptors were never closed (a leak per policy start/stop cycle). Move the actual close to a new PingProbe::close_thread_send_sockets() invoked at the end of the io-thread lambda (after the loop stops, before stop() joins), so it runs on the thread that owns the descriptors. _close_socket() now only adjusts the ping_sockets refcount. Socket sharing across probes is unchanged (via the thread_local socket value). TCP probes use libuv-managed handles and are unaffected. 2. recv_packets data race. The process-wide static recv_packets was reassigned every 100ms on the receiver thread while per-probe async callbacks iterated it on other threads — a use-after-free if a publish reallocated mid-iteration. Guard it with recv_packets_mtx: publish under the lock; each consumer snapshots under the lock then iterates its private copy unlocked. 3. _receiver singleton + _callbacks races. Construct the process-wide receiver via std::call_once (no double-construct / pointer race on concurrent starts), and publish it through an atomic _receiver_view so the cross-thread reader receiver_v6_active() (called from info_json on the HTTP thread) cannot observe a torn pointer. Guard _callbacks with _callbacks_mtx in register/remove and the timer fan-out so the list can't be mutated mid-iteration. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
Author
|
@codex review |
|
Codex Review: Didn't find any major issues. Swish! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
| if (auto lg = spdlog::get("visor")) { | ||
| lg->warn("netprobe: unable to arm ICMPv6 poll handler; IPv6 ping disabled"); | ||
| } | ||
| #ifdef _WIN32 |
Comment on lines
287
to
289
| if (PingProbe::sock_count) { | ||
| j[schema_key()]["ping_sockets"] = PingProbe::sock_count + 1; | ||
| j[schema_key()]["ping_sockets"] = PingProbe::sock_count + 1 + (PingProbe::receiver_v6_active() ? 1 : 0); | ||
| } |
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.
Closes #624 (the open item — ICMPv6 ping; the PcapPlusPlus bump in the issue title is long done).
What
Add IPv6 ICMPv6 echo ping to the NetProbe input, with the same RTT/loss metric model as IPv4. Also fixes two DNS-resolution bugs shared with the TCP probe and gives TCP probes IPv6 support.
ip_version(4|6) option forces a hostname's resolution + ping family (invalid value / literal-vs-version conflict throw at config-parse time).ip_versionis per-target only — the top-level config keys are unchanged._sockv4 /_sock6v6);_send_icmp_v6builds apcpp::ICMPv6EchoLayerecho request. Kernel fills the mandatory ICMPv6 checksum (RFC 3542 §3.1).PingReceiver(adds anIPPROTO_ICMPV6socket + poll handle on the existing receiver thread). A raw ICMPv6 socket strips the IPv6 header, so replies are parsed by synthesizing a minimal IPv6 header (DLT_RAW1) into a self-owningpcpp::Packetthat survives the fan-out deep-copy._id(unique per target), reliable on raw sockets. TheSOCK_DGRAMfallback is documented best-effort.PingProbe::_get_addrandNetProbe::_resolve_dns; DNS targets re-resolve each interval; IPv4-preferred default (ping and TCP now agree);ip_versionthreaded intoTcpProbe.Testing
CI-deterministic (no raw sockets):
ip_versionvalidation + literal-conflict, the unchanged valid-keys regression guard, v4 and v6 handler matching round-trips (single-target success + two-target no-cross-match), and a carrier deep-copy test that asserts the ICMPv6 layer survives the receiver→handler fan-out. Live ICMPv6 round-trip stays manual/privileged (like the existingSKIP'd ping test).Needs verification on this PR's CI / a Windows host
CAP_NET_RAW/ root / Administrator), which pktvisor already needs for capture; DGRAM ping matching is best-effort. Link-local (fe80::) targets are out of scope in v1.Notes
Draft for CI to exercise the Linux/Windows builds + suite. The implementation went through adversarial multi-lens review against
develop+ a re-verification, which caught and fixed real defects (heap-safety of the receive path, reply-drop on the Packet copy, a DNS re-resolution regression) before this PR — see the commit history.🤖 Generated with Claude Code