feat(netprobe): IPv6 (ICMPv6) ping support (#624) - #785
Merged
Conversation
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.
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". |
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".
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". |
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: 8c7628bbd9
ℹ️ 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".
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". |
…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>
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>
…'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>
…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>
… and ping_sockets Addresses three PR review comments: - DNS resolver order (Codex P2): NetProbe::_resolve_dns forced IPv4-preference even when no ip_version was set, so unpinned dual-stack TCP targets could silently switch away from the resolver/OS-preferred (e.g. AAAA-first) endpoint and report spurious failures. When unforced (first_match) it now returns the first address in the resolver's own order, of either family; family filtering applies only when ip_version pins a family. PingProbe is unaffected (it uses its own _get_addr, which keeps an intentional IPv4-preferred default so unpinned ping doesn't require a v6 raw socket). - v6 poll-handle cleanup (Copilot): when the ICMPv6 poll handle can't be armed, close() AND reset() _poll6 in the failure branch instead of leaving it allocated for the process-wide receiver's lifetime. uvw's poll_handle self-retains from a successful init(), so dropping our reference is safe — the handle lives until the loop processes uv_close. - ping_sockets metric (Copilot): the field was gated on a thread_local sock_count that info_json (an HTTP worker thread) always read as 0, so it was effectively never emitted. Removed sock_count (and the now-empty _close_socket) entirely and derive the count from _probes.size() — a stable per-stream member — gated behind _type == TestType::Ping. The real send-socket close path (close_thread_send_sockets on the io thread) is unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… is copied Addresses the latest PR review comments: - (Codex P1) Restore the explicit _poll6->init() before _poll6->start(). uvw's resource<poll_handle>() already initializes the handle (loop::init dispatches to poll_handle::init), so this is technically redundant, but the v4 receiver does the same explicit init() and the asymmetry was misleading. Re-adding it is harmless (an un-started poll handle can be re-initialized) and keeps the v4/v6 paths symmetric. - (Copilot) std::move the carrier pcpp::Packet into the receive batch instead of copying it. (pcpp::Packet currently has no move ctor, so this still deep-copies today, but it is correct and benefits automatically if pcpp gains move support.) - (Copilot) Keep the per-consumer deep copy of the received batch rather than switching to a shared snapshot. A shared snapshot would be a data race: the v4 reply path calls pcpp::IcmpLayer::getEchoReplyData(), which mutates the layer's internal m_EchoData on access, and the single receiver fans every packet out to probes across all input streams running on different io threads. The copy isolates each consumer; added a comment explaining this so it isn't "optimized" away. Ping reply volume is low, so the cost is negligible. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
leoparente
force-pushed
the
feature/netprobe-icmpv6-ping
branch
from
June 23, 2026 16:48
677d9b3 to
530fd55
Compare
Contributor
Author
|
@codex review |
|
Codex Review: Didn't find any major issues. Another round soon, 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". |
Copilot review: the v6 poll handler did a single recvfrom() per event but the non-blocking setup (ioctlsocket/fcntl) return value was ignored. If the socket stayed blocking and a READABLE event fired without a datagram ready (spurious wakeup / error condition), that read would block the entire receiver thread, stalling reply processing for every ping probe in the process. Two layers of defense: - Capture whether the socket was actually made non-blocking and fold it into the v6-readiness gate (if (_poll6 && nonblock_ok)). If it can't be set, IPv6 is cleanly disabled via the existing close+invalidate path rather than armed. - On POSIX, pass MSG_DONTWAIT to recvfrom so the read provably never blocks even if the socket somehow ended up blocking. On Windows the socket is verified non-blocking, so a 0 flag is equivalent. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
leoparente
marked this pull request as ready for review
June 23, 2026 18:06
samiura
approved these changes
Jun 23, 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.
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