Conversation
Signed-off-by: Gabriel Ganne <gabriel.ganne@gmail.com>
This was found when testing the future option parsing rework, but were unrelated to how the options are actualy parsed. This is mostly about how the error paths are handled. - tcpedit_close(&tcpedit) called on initialization failure paths can lead to use-after-free / NULL dereference. Just remove and errx() exit the program. - tcpprep required both -o AND -i options. use || instead of &&. Signed-off-by: Gabriel Ganne <gabriel.ganne@gmail.com>
autogen's autoopts is getting deprecated in major distributions [1]. Also autoopts and libopts are actually the same package, so this removes libopts dependency entirely. This is a proposal to migrate from autoopts to asciidoctor [2] which is packaged by all major distributions and used by projects like wireshark or git which should guarantee that it will be maintained for a long time. The option parsing themselves are written using standard getopt. Note that asciidoctor comes with a ruby dependency. This should only be needed for maintainers, and since this is already packaged by distributions, I believe this is acceptable. The newly written doc files will be put in a dedicated "docs" folder. Interface changes: * save/load opts removed: - 3 cli options removed - explicit test options instead of loading from file * tcpcapinfo: Remove "--more-help" option which does not display more help. "--help" is enough. * tcpliveplay: Unify all the help options into a single "--help" option that displays the most verbose help output. This is small enough. * migrate roff -> asciidoctor for html documentation generation [1] https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=1076243 [2] https://github.com/asciidoctor/asciidoctor Assisted-by: Claude Sonnet 4.6, Claude Opus 4.8 Signed-off-by: Gabriel Ganne <gabriel.ganne@gmail.com>
l2len has a hardcoded maximum of 50 Bytes in struct fragroute_s. This does not change the maximum value of 50 (12 MPLS labels) and only adds a check to make sure we don't copy over the maximum. Fixes: #992 Signed-off-by: Gabriel Ganne <gabriel.ganne@gmail.com>
Fix an issue where using tcprewrite to add a vlan tag to untagged packets, and omitting either the --enet-vlan-cfi, or the --enet-vlan-pri option, the resulting pcap is truncated by 4 bytes (size of vlan header). This also changes the following so that the vlan tag value is manatory, but the priority and the format are optional: * Both options --enet-vlan-pri/--enet-vlan-cfi now explicitly default to 0 * Add a one-time warning if either option is missing, then proceed and use default * update the doc so that --enet-vlan-tag option is marked as mandatory when adding a vlan tag. Fixes: #990 Assisted-by: Claude Opus 4.8 Signed-off-by: Gabriel Ganne <gabriel.ganne@gmail.com>
check_dst_port() did not check that the dest port could be read for truncated packets. This adds a test that enforces a tiny bit more that strictly necessary: check for the whole l4 header presence instead of just the dport. The whole header is needed overall anyway. This means that tcpprep will fail to classify packets with incomplete l4 headers and fall back to the default classification for those. Fixes: #985 Signed-off-by: Gabriel Ganne <gabriel.ganne@gmail.com>
Documents the build/test workflow, coding standards from docs/HACKING, and the architecture (sendpacket abstraction, AutoOpts CLI generation, tcpedit DLT plugins, tcpprep cache pipeline) so future sessions can get productive without re-deriving it from scratch. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
When Claude has to review a PR or work in a large codebase, it ends up reading half the repo to understand the call graph. This tool parses your repository into an AST with Tree-sitter, stores it as a graph of nodes (functions, classes, imports) and edges (calls, inheritance, test coverage), and at review time computes the minimal set of files Claude actually needs to read. The published benchmarks show 6.8× fewer tokens on code reviews and up to 49× reduction on daily coding tasks in a Next.js monorepo. Average across six real open-source repos was 8.2×. The initial graph build takes around ten seconds for a 500-file project, and it auto-updates on every file edit and git commit. ``` pip install code-review-graph code-review-graph install ```
…loop Merges PR #986 (Yaroslav / d3156) which fixed sendpacket() spinning forever on transient EAGAIN/ENOBUFS under sustained buffer pressure, plus review fixups: - usleep(0) doesn't actually sleep on any target libc; use a real 100us backoff between retries (matches src/common/txring.c's existing usleep(100) convention) so the bound also curbs CPU/kernel load, not just loop count. - Set an explicit error message via sendpacket_seterr() when the retry cap is hit, so callers reading sendpacket_geterr() get an accurate reason instead of a stale/empty errbuf. - Fix EXIT_MAX_RETRIES label indentation to match the existing TRY_SEND_AGAIN label, and drop the trailing-whitespace blank line. Refs #984. Co-Authored-By: Yaroslav <d3156@users.noreply.github.com> Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…UFS_EAGAIN Bug #984 endless retries on enobufs eagain
.claude/ was blanket-ignored, which silently dropped the four code-review-graph skill files (debug-issue, explore-codebase, refactor-safely, review-changes) even though they're portable project workflow docs with no machine-specific content — same category as the already-tracked CLAUDE.md/.cursorrules/etc. Carve out an exception so they're shared with the team; keep .claude/settings*.json and .claude/worktrees/ ignored since those do carry per-machine absolute paths. CLAUDE.md told every session to "ALWAYS use the code-review-graph MCP tools" but never said how to get the MCP server registered in the first place, since .mcp.json/.claude/settings.json are (rightly) gitignored personal config. Add setup steps so a fresh clone isn't stuck with instructions it can't fulfill. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
gitignore: share .claude/skills/, document code-review-graph MCP setup
) The speed_mbpsrate and speed_packetrate branches of calc_sleep_time() each had a tiered guard meant to avoid overflowing COUNTER (64-bit) before dividing, gated on COUNTER_OVERFLOW_RISK (COUNTER_MAX >> 23, ~2.2e12). In both cases the guard's threshold was many orders of magnitude higher than the point where the guarded multiply actually overflows: - speed_mbpsrate: bits_sent * 1e9 overflows once bits_sent exceeds ~1.8e10 (~2.3GB sent) -- ~119x below the guard's threshold. - speed_packetrate: pkts_sent * 1e9 * 3600 overflows once pkts_sent exceeds ~5.1M packets -- ~4.3e5x below the guard's threshold. So for any real long-running -M/-p replay, the "safe" branch was essentially never taken and the multiply silently overflowed, wrapping next_tx_ns to a small/garbage value and causing tcpreplay to stop sleeping between packets near EOF, exactly matching #974's reported symptom (confirmed against the reporter's log: their divergence starts at ~2.3GB sent, matching the computed boundary). Replace both guarded multiplies with a widened-precision multiply (unsigned __int128, available on GCC/Clang for all listed 64-bit targets) divided by the rate, which cannot overflow for any COUNTER-representable input, with a portable non-__int128 fallback that unconditionally uses the previously-underused-but-safe reordering (multiply/divide split around the division) instead of a threshold guard. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Credit @blaa (reported #974) and @AB-Coder96 (diagnosed the bug, validated repro numbers, prototype fix in #989). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
send_packets: fix miscalibrated overflow guards in calc_sleep_time (#974)
migrate autoopts -> getopt + asciidoctor
Replaces duplicated inline flags in prep_config/rewrite_config/replay_config targets with a single test/config.in loaded via --load-opts, so the three tools test against one common config file instead of drifting args. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
configure~ and src/config.h.in~ get regenerated by autoconf/autoheader on every reconfigure and shouldn't be tracked. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Revert "migrate autoopts -> getopt + asciidoctor"
fix fragroute overflow on packets with too many MPLS labels
tcprewrite - fix adding vlan tag with missing options
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Reviewed: fixes real NULL-deref/OOB-read in check_dst_port() for truncated IPv4 packets, plus L4 header length bounds check for TCP/UDP. Docs updated in 353c811.
mod_close() used TAILQ_FOREACH_REVERSE(rule, &rules, next, head), matching bundled lib/queue.h's non-standard (var, head, field, headname) argument order. Standard BSD <sys/queue.h> uses (var, head, headname, field) instead. mod.c includes lib/queue.h first, but common.h pulls in system headers (net/if.h and friends) that transitively include the real <sys/queue.h> on some platforms, redefining the macro with the opposite argument order and breaking the build (macOS 13/14 confirmed via Homebrew CI; macOS 15 and Linux unaffected because their SDK/glibc header graph doesn't pull in sys/queue.h from this include chain). Swapping the call's argument order (as proposed in the issue) just trades one broken platform for another, since it still depends on which queue.h wins the macro collision. Fix instead avoids TAILQ_FOREACH_REVERSE and iterates with TAILQ_LAST/TAILQ_PREV/TAILQ_END directly, whose signatures are identical across every BSD queue.h implementation. Fixes: #981
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ackets The original fix (cherry-picked from Sup3rGeo/tcpreplay#915) only updated send_packets(), leaving send_dual_packets() (-I/dual-interface replay) with the old cumulative pkt_ts_delta accumulation while calc_sleep_time() had its timesdiv_float(multiplier) call removed - so --multiplier would have silently stopped working at all for dual-interface replay had this merged as-is. Ports the same fix: anchor on the first packet's capture timestamp and wall-clock time, then compute pkt_ts_delta/time_delta as offsets from that anchor every packet, instead of accumulating deltas from the previous packet. Matches the pattern already proven in send_packets() and in speed_mbpsrate/speed_packetrate's existing calc_sleep_time() cases.
Verified against both #724 (100k-packet drift: 3.7% error -> 0.1% error with --multiplier=10) and #674 (--multiplier<1.0 + --loop linearity, no regression) with a baseline-vs-fixed comparison build. Also closes a gap in the original patch (#915): send_dual_packets() now gets the same anchor-based fix, since --multiplier would have silently done nothing for -I dual-interface replay otherwise. Full local test suite: 69/69 OK. CI green.
send_packets()/send_dual_packets() spin on netmap_tx_queues_empty() at the end of a --loop replay, waiting for the netmap TX ring to report drained. Some netmap/driver combinations never flip the ring's empty flag (external bug, not something we control - see #560, where the reporter traced it into netmap's own nm_tx_pending() always returning true on their patched ixgbe driver). Two things in our own code made this worse than it needed to be: neither ctx->abort nor any timeout were checked in the wait loop, so a stuck ring meant an unkillable, unresponsive-to-SIGINT, 100% CPU spin forever - "not being able to abort with CTRL+C" was the reporter's main complaint, and is fixable independent of whatever's wrong in netmap/the driver. Factor the four (two functions x two interfaces) duplicate wait loops into netmap_wait_tx_drain(): checks ctx->abort every iteration so SIGINT breaks out immediately, and gives up with a warning after NETMAP_TX_DRAIN_TIMEOUT_SEC (2s) if the ring still hasn't drained, instead of spinning indefinitely. Doesn't fix the underlying non-draining ring (external to this repo), but turns an unkillable hang into a bounded, diagnosable exit. Fixes: #560 Note: could not compile-test the HAVE_NETMAP path locally - this dev machine has no netmap headers/library, and CI doesn't build with netmap either (no --with-netmap in the CI workflow). Verified by manual review that all macros/functions used (TIMESPEC_SET, timessub, get_current_time, netmap_tx_queues_empty) match their existing call signatures elsewhere in this same file. Non-netmap build compiles clean and full local test suite passes (69/69), but the netmap-gated code itself needs a real netmap build to verify.
Merging: makes the netmap TX-ring drain wait abortable (ctx->abort checked every iteration, so SIGINT/Ctrl+C works) and bounded (2s timeout with a warning instead of an infinite 100%-CPU spin). Doesn't fix the underlying non-draining ring itself, which is external to this repo (netmap/driver-side per the original reporter's own trace into nm_tx_pending()) - this just stops it from hanging unkillably. CI (compile + full test suite) green. Note: could not compile-test the HAVE_NETMAP code path itself - no netmap headers on the dev machine, and CI doesn't build with netmap either - verified only by manual signature review against matching usage elsewhere in the file. Real netmap hardware/build verification is still outstanding.
nm_do_ioctl()'s post-ioctl result-parsing switch(what) only has cases for GET-style requests (SIOCGIFFLAGS, ETHTOOL_Gxxx) that need their result extracted from ifr/eval. SET-style requests (SIOCSIFFLAGS, ETHTOOL_Sxxx) have nothing to parse and were always meant to fall through to `done:`, returning whatever `error` the real ioctl() call produced. Commit 8f3761d ("Feature #773: Code cleanup based on clang-tidy and clang-format", between 4.4.2 and 4.4.4) added "default: return -1;" to both this switch and its nested ETHTOOL subcmd switch. Neither existed in the original code. The outer one means every SIOCSIFFLAGS call - used to set IFF_UP/IFF_PROMISC when opening a real (non-VALE) netmap interface - now reports failure even when the ioctl succeeded, and skips `close(fd)` on the way out (fd leak) since the return happens before the `done:` label. sendpacket_open_netmap() treats that false failure as fatal (`if (nm_do_ioctl(sp, SIOCSIFFLAGS, 0) < 0) goto NM_DO_IOCTL_FAILED;`), which formats its error message with strerror(errno) - but errno was never set on this path, so the reported reason is stale/unrelated. This matches #810's debug log exactly: the SIOCGIFFLAGS call right before succeeds ("SIOCGIFFLAGS flags are 0x..." - it has a real case in the switch, so it's unaffected), then the very next call is SIOCSIFFLAGS, which hits the bogus default and fails - matching the "failed!" printed immediately after, and the garbage "nm_do_ioctl: No such file or directory" that follows. The inner ETHTOOL-subcmd default has the same problem for the ETHTOOL_S* (set) variants called right after, which would have failed too once the SIOCSIFFLAGS call ahead of them stopped short-circuiting first. Fixes: #810 Note: could not compile-test this - netmap.c only builds under the COMPILE_NETMAP automake conditional, and neither this dev machine nor this repo's CI has netmap available. Verified by diffing against 8f3761d~1 (pre-cleanup): this restores that exact, previously-working structure. Brace-balance checked manually.
Merging: root-caused via bisection to commit 8f3761d (clang-tidy cleanup between 4.4.2 and 4.4.4), which added two erroneous switch default clauses to nm_do_ioctl() that made SIOCSIFFLAGS/ethtool SET requests report failure - and leak the ioctl fd - even when the underlying ioctl succeeded. This diff restores the exact pre-regression structure (diffed byte-for-byte against 8f3761d~1). CI (compile + full test suite) green. Note: could not compile-test the netmap-gated code itself - COMPILE_NETMAP isn't available on this dev machine or in CI - confidence comes from the direct diff against the known-working prior revision, not a live build or hardware repro.
sendpacket_open() always prefers PF_PACKET over libpcap on Linux,
regardless of what libpcap was compiled against. For "zc:<ifname>"-style
device names - PF_RING ZC's own virtual device addressing, resolved by
PF_RING's patched libpcap, not by the kernel - the PF_PACKET path's
get_iface_index() does a plain SIOCGIFINDEX ioctl on the literal device
string. The kernel has no interface actually named "zc:ens160", so this
always fails with ENODEV ("ioctl: No such device"), even though the
interface itself works fine - confirmed in #913 by the reporter's own
test with PF_RING's pfsend utility succeeding on the same device string.
tcpreplay already knows about "zc:" devices (interface.c formats them
that way for --listnics under HAVE_PF_RING_PCAP), it just never routed
sendpacket_open() to the one backend (libpcap, PF_RING-aware when built
against PF_RING's patched libpcap) that understands the naming.
Route "zc:"-prefixed devices to sendpacket_open_pcap() instead. That
function (and its libpcap-based send path, already used by SP_TYPE_LIBPCAP
in the main send switch) was previously only compiled when no native
method (PF_PACKET/BPF/libdnet) was available at all - widened its build
guard to also compile when HAVE_PF_RING_PCAP is set, since it's now
needed alongside PF_PACKET on a typical PF_RING-enabled Linux build.
Also fixed sendpacket_close()'s SP_TYPE_LIBPCAP case while touching this:
it was guarded by "#ifdef HAVE_LIBPCAP", a macro configure.ac never
actually defines (confirmed via config.h) - pcap_close() was dead code on
every build. Newly reachable now that SP_TYPE_LIBPCAP is compiled in
alongside PF_PACKET; removed the bogus guard so the handle actually gets
closed.
Not fixed (out of scope): tcpbridge's sendpacket_get_hwaddr() dispatches
by compile-time #elif on which native method is available, not by the
sendpacket_t's actual runtime handle_type - so a zc: interface opened via
libpcap would still hit sendpacket_get_hwaddr_pf() there and read the
wrong union member. Pre-existing architectural issue, not part of #913's
report (plain tcpreplay, not tcpbridge), and riskier to change blind.
Fixes: #913
Verified by force-compiling sendpacket.c with -DHAVE_PF_RING_PCAP=1 added
to this build's real flags (BPF+libpcap already present here) - compiles
clean with -Wall -Wextra, and nm confirms pcap_open_live/pcap_inject are
referenced (sendpacket_open_pcap() actually got compiled in, not
dead-code-eliminated). Full local test suite still 69/69 with the normal
(no PF_RING) build. Could not test actual PF_RING ZC behavior - no PF_RING
install available.
Merging: routes "zc:" PF_RING ZC device names through libpcap (PF_RING-aware when built against PF_RING's patched libpcap) instead of always defaulting to PF_PACKET, which did a plain SIOCGIFINDEX kernel lookup that can never resolve PF_RING's virtual device naming. Also fixed an adjacent dead-code bug: sendpacket_close()'s SP_TYPE_LIBPCAP case was guarded by a macro (HAVE_LIBPCAP) that configure.ac never actually defines, so pcap_close() never ran on any build - newly reachable now that this path compiles in alongside PF_PACKET. Force-compiled with -DHAVE_PF_RING_PCAP=1 on top of this build's real flags, clean under -Wall -Wextra, confirmed via nm the new code path is actually compiled in (not eliminated). CI (compile + full test suite) green. Note: could not test actual PF_RING ZC runtime behavior - no PF_RING install available (patched libpcap + numa/pthread prereqs).
RFC4443 section 3 has Destination Unreachable, Packet Too Big, Time Exceeded, and Parameter Problem messages embed "as much of the invoking packet as possible" - including its IPv6 header. rewrite_ipv6l3() only ever ran once per packet, on the outer header, so tcprewrite --pnat/ --seed/--endpoints left the embedded original addresses untouched. Two real problems from this: rewritten captures aren't RFC4443-compliant, and using tcprewrite for capture sanitization/anonymization silently leaked the original addresses through every ICMPv6 error message in the capture. Fix: after rewriting the outer header's addresses, check if the L4 protocol is ICMPv6 and the type is one of the four error types (the other ICMPv6 types - echo, neighbor discovery, MLD, etc - don't embed a packet and are left alone). If so, locate the embedded IPv6 header (fixed 8-byte ICMPv6 header, then the embedded packet) and recurse rewrite_ipv6l3() on it. Reuses the exact same address-rewrite logic as the outer header, including whatever further recursion the embedded payload itself needs (e.g. a TCP/UDP checksum inside the embedded packet, if it's fully present in the capture rather than truncated). Bounds-checked against the actual captured length throughout, and naturally terminates since each recursion level consumes real bytes from a fixed-size input - no unbounded loop risk. The recursive call was placed carefully: the existing function had an early `return (0)` right after the ipmap (--srcipmap/--dstipmap) loops whenever no cidrmap (--pnat) was configured, which would have skipped the new embedded-header logic entirely for ipmap-only usage. Restructured that into a conditional block instead so the ICMPv6 check always runs regardless of which rewrite mode was used. Adjacent, pre-existing bug also fixed while touching this code: ipv6_addr_csum_replace()'s protocol switch (used to patch the L4 checksum when an *outer* IPv6 address changes) only handled TCP/UDP - ICMPv6 was missing, even though ICMPv6's checksum covers a pseudo-header (src/dst IP) exactly like TCP/UDP per RFC4443 2.3. Every ICMPv6 packet's checksum went stale after any IPv6 address rewrite unless --fixcsum was separately passed. Added the missing case. Full checksum correctness for the *embedded* header's changes (the outer ICMPv6 checksum, which covers the whole message including the now-edited embedded bytes) relies on --fixcsum doing a full recompute, same as this issue's own reproduction steps - didn't attempt to hand-roll incremental multi-address checksum delta math for that case; do_checksum() already does a correct full recompute over the current byte content regardless of what's inside, so --fixcsum "just works" here already. Fixes: #818 Verified against the exact pcap and reproduction command from the issue (downloaded the reporter's attached ttl.pcap): built both this fix and an unmodified v4.5.3-beta1 baseline, ran the reporter's literal command --pnat=[2001:0DB8:85A3:08D3::/64]:[2001:0db8:FFFF:FFFF::/64] --fixcsum against both. Baseline: embedded packet inside the Time Exceeded message still shows the original 2001:0db8:85a3:08d3::666 - confirmed leak, matches the bug report exactly. Fixed: embedded packet correctly shows the rewritten 2001:0db8:ffff:ffff::666. tcpdump -v independently validates both ICMPv6 checksums as "[icmp6 sum ok]" on the fixed output. Full local test suite: 69/69, no regressions.
Merging: rewrites IPv6 addresses embedded in ICMPv6 error messages (RFC4443 has these embed the invoking packet's IP header), fixing both an RFC-compliance gap and a real address-leak in capture sanitization/anonymization. Also fixes an adjacent pre-existing bug: ICMPv6's checksum was never fixed up when an outer IPv6 address changed, unless --fixcsum was separately passed. Verified end-to-end against the reporter's own attached pcap and exact reproduction command - byte-level diff confirms the leak in baseline and the fix in this PR, tcpdump -v confirms both ICMPv6 checksums are valid on the fixed output. CI (compile + full test suite) green.
README's "Simple directions for Unix users" only showed ./configure && make && sudo make install, which only works from a release tarball (configure is pre-generated there). A fresh git clone has no configure script until autogen.sh generates it - missing that step is the first thing anyone hits building from source. Fixes: #568
Merging: doc-only fix, adds the missing autogen.sh step to README's build instructions for git checkouts (release tarballs already ship a pre-generated configure, so that path is unchanged). Confirmed the gap still existed on current v4.5.3-beta1 before fixing. CI green.
catch_alarm() only set keep_going = 0 - it never called pcap_breakloop(live_handle). When the alarm fires while pcap_dispatch() is blocked waiting for the remote host's response, that block wasn't interrupted: pcap_dispatch() just ran out its own buffer timeout (TIMEOUT_ms, 10s) before returning, regardless of the alarm. Worse, on libpcap implementations where the buffer timeout doesn't start counting until the first packet arrives, a remote host that never responds at all could leave pcap_dispatch() blocked indefinitely, with no way for the SIGALRM to reach it. Root-caused by @tflament in the issue thread (comment from 2024-04-29): traced it to this exact function via manual TIMEOUT_ms reduction and observed the correlation with the alarm's ~10s firing time. This is the fix they diagnosed but couldn't submit themselves (self-described non-C-developer). pcap_breakloop() is documented as safe to call from a signal handler specifically for this purpose. With this, an alarm firing mid-wait causes pcap_dispatch() to return immediately (0 packets), and the existing outer while loop's `if (!keep_going) break;` check reports the timeout right away instead of after an extra ~10s delay - matching the "remote host is not responding" false-timeout every reporter in this thread hit despite the remote SYN-ACK having already arrived. Fixes: #540 (partially - see below) Scope note: this thread also reports a second, separate bug once packets do start flowing - an incorrect ACK value sent after the handshake (per tflament's same comment, not fully root-caused even by them). That's in tcpliveplay's relative-to-absolute SEQ/ACK remapping state machine (fix_ack()/relative_sched() and friends) - a materially riskier area to touch blind, and not something I can verify without a live remote TCP endpoint to replay against. Left alone; flagging as still open. Verification: tcpliveplay only builds under COMPILE_TCPLIVEPLAY (Linux-only per configure.ac, "requires linux OS to function properly") - this dev machine is macOS, can't compile it directly. Got as far as a full-file -fsyntax-only pass allows: resolved all includes (libdnet, libopts/autoopts), and clang parsed cleanly through this function with zero errors near it - the only errors reported are for genuinely Linux-only code much later in the file (SIOCGIFHWADDR), unrelated to this change. Also compiled the exact modified function standalone against real pcap.h/signal.h - clean, zero warnings under -Wall -Wextra. This repo's CI does build tcpliveplay for real (Linux runner), so this PR should get actual compile coverage there, unlike the netmap/PF_RING fixes earlier in this branch's history. No live functional test was possible.
Merging: catch_alarm() now calls pcap_breakloop(live_handle), fixing the false 'remote host is not responding' timeout every reporter in this thread hit despite the response having already arrived - credit to @tflament's diagnosis in the issue thread. CI confirms this actually compiles on Linux (tcpliveplay is Linux-only, no coverage on this dev machine). Note: this fixes the hang/timeout specifically - the thread's separate 'wrong ACK value' bug is NOT fixed by this PR, flagged as out of scope in the PR description.
rewrite_ipv4_tcp_sequence()/rewrite_ipv6_tcp_sequence()
(rewrite_sequence.c) and rewrite_ipv4_ports()/rewrite_ipv6_ports()
(portmap.c) all take an ipv4_hdr_t**/ipv6_hdr_t** parameter and call
get_layer4_v4()/get_layer4_v6() to locate the L4 header, passing an
"end of buffer" bounds pointer computed as:
(u_char *)ip_hdr + l3len
ip_hdr here is the ipv4_hdr_t** parameter itself - a pointer to the
caller's local variable. Casting it straight to u_char* and adding
l3len computes an address relative to wherever the compiler happened
to place that parameter (typically the stack), which has no
relationship whatsoever to the actual packet buffer (typically heap).
Should have been:
(u_char *)(*ip_hdr) + l3len
i.e. dereference first to get the real packet buffer address, matching
every other caller of get_layer4_v4()/v6() in this codebase (all of
which already do this correctly - only these four call sites, across
two files, had the bug).
get_layer4_v4()/v6() bounds-check the computed L4 pointer against this
end_ptr (`if (ptr > end_ptr) return NULL;`) and return NULL - a benign,
handled "not enough data" outcome - if it's ever exceeded. Since the
garbage end_ptr here bears no relation to the real buffer, whether
that check spuriously passes (rewrite works, by luck) or spuriously
fails (rewrite silently no-ops for every packet) depends entirely on
the relative addresses of stack vs. heap at runtime - which depends on
stack layout, allocator behavior, and ASLR, all of which are
platform/ABI/compiler dependent. That's exactly the reported symptom:
--tcp-sequence and --portmap/-r have zero effect on macOS arm64 while
working correctly on macOS Intel and Linux arm64, for byte-identical
compiled logic and configuration.
Root-caused with a lot of help from real hardware I don't have access
to: this diagnosis went through two wrong turns first (a real but
practically-inert signed-shift UB in tcpr_random(), and a
disproven "AutoOpts isn't parsing the options" theory) before a set of
temporary diagnostic prints - run on the actual affected arm64 machine
- showed tcp_sequence_adjust and the parsed portmap chain were both
already 100% correct at configure-time, which pointed at the rewrite
*application* path instead and led here.
Fixes: #1011
Verified: full local test suite 69/69 on macOS Intel (no regression -
consistent with this platform's stack layout having made the old,
buggy bounds check pass by chance). Cannot verify this actually fixes
the arm64 divergence directly - no such hardware in this environment -
but the mechanism is now fully explained rather than inferred, and
directly addresses the exact "some platforms it's zero effect, others
it works" pattern confirmed live in #1011.
Confirmed fixed on the actual affected hardware: reporter ran the full test suite on macOS arm64 with this branch and all 69 tests pass, including the three that were failing (Portmap, Portmap range, TCP sequence). CI (compile + full test suite) green on Linux. Root cause and fix as described: rewrite_ipv4_tcp_sequence()/rewrite_ipv6_tcp_sequence() and rewrite_ipv4_ports()/rewrite_ipv6_ports() computed the L4 end-of-buffer bounds pointer from the address of their own double-pointer parameter instead of dereferencing it first - a stack-vs-heap address bug whose bounds-check outcome depended on platform-specific memory layout.
tcpr_random() (a custom rand_r()-alike, explicitly documented as
"consistent across all platforms" to avoid libc rand()/rand_r()
portability differences) declared its accumulator as signed int:
int result;
...
result <<= 10; // x2
After the first two mixing stages, result can carry up to ~21
significant bits. The second `result <<= 10` shifts that up to ~31
bits - close enough to the sign bit that a set bit can be shifted into
or past it. Left-shifting a signed int such that the result isn't
representable in the type is undefined behavior in C (C11 6.5.7p4).
Different compiler backends are free to resolve that UB differently -
which is exactly what happened: confirmed via #1011 that this function
produces different output for the same seed on macOS arm64 than on
macOS Intel and Linux arm64, defeating the entire point of the function
existing (avoiding platform-dependent randomness).
Fix: declare `result` as `unsigned int` instead. Unsigned left-shift
overflow is well-defined (mod 2^32 wraparound) in C, which removes the
UB entirely - this is what actually makes the function's
platform-consistency claim true. All five call sites already treat the
return value as unsigned (uint32_t rand_num/r, or an explicit (int)
cast by the one caller that wants a signed reinterpretation), so this
doesn't change any calling code.
Verified via #1011's diagnosis: --tcp-sequence's seed-to-adjustment
derivation calls this 5 times and was the reproducer - traced the
platform divergence to an exact constant offset (2165354059) across
every differing packet in that report, meaning it was one seed
computation producing a different result, not per-packet drift.
Fixes: #1011 (partially - see issue for the other two affected tests,
not yet root-caused)
Testing: confirmed this doesn't change output on this dev machine
(macOS Intel, where the UB apparently already resolved to what the
well-defined unsigned computation produces) - full local test suite
still 69/69, including every other tcpr_random() consumer (TCP
sequence, seeded MAC, seeded MAC keep-bytes, L7 fuzzing). Cannot verify
this actually fixes the divergence on macOS arm64 itself - no such
hardware available here.
The have_netmap detection block unconditionally appended
"-DNETMAP_WITH_LIBS -DND -I$NETMAPINCDIR" to the global CFLAGS as soon
as a netmap checkout was found, with no restore anywhere afterward -
unlike the equivalent CPPFLAGS addition a few lines later, which is
correctly scoped (saved to OLDCPPFLAGS, restored right after the
netmap-version-specific checks that need it). A dead OLDCFLAGS
assignment right next to the CFLAGS mutation suggests a save/restore
was intended here too, but never actually implemented - OLDCFLAGS was
computed but never read again anywhere in the script.
Since CFLAGS stayed permanently polluted with the netmap include path
for the rest of configure.ac, every later AC_CHECK_HEADERS/
AC_COMPILE_IFELSE test (autoconf test macros compile using CPPFLAGS
and CFLAGS together) could also pick up headers from inside the
netmap checkout. A netmap source tree mirrors a BSD kernel source
layout under sys/ - including its own sys/net/bpf.h - so the later
"checking for net/bpf.h" feature check (meant to detect a genuine BSD
system header, used for the macOS/BSD BPF injection backend) found
netmap's vendored copy instead and set have_bpf=yes even on Linux,
where no such header should exist. That wrongly defines HAVE_BPF,
which activates a defines.h.in code path meant only for real BSD
systems (#include <net/bpf.h>) ahead of the system libpcap headers -
and on this reporter's system, the anti-double-include guard
(PCAP_DONT_INCLUDE_PCAP_BPF_H) didn't stop the system libpcap from
also defining its own copy of struct bpf_insn, producing:
error: redefinition of 'struct bpf_insn'
Fix: don't touch the shared CFLAGS during netmap detection. Save the
netmap include flags into a dedicated NETMAP_CFLAGS variable instead,
and apply it to CFLAGS exactly once, right before AC_CONFIG_FILES/
AC_OUTPUT - after every other AC_CHECK_*/AC_COMPILE_IFELSE test in the
script has already run against a clean CFLAGS. The real build (where
netmap.c etc. need to find netmap's headers) still gets the same
CFLAGS addition, just deferred to the point where it can no longer
shadow unrelated system headers during configure's own detection.
Verified: this dev machine has no netmap checkout to test the
--with-netmap path directly, so I cannot reproduce or fully verify the
fix end-to-end myself. Confirmed the non-netmap path (the common
case - configure without --with-netmap) is completely unaffected:
NETMAP_CFLAGS stays empty, the new "if" block at the end is a no-op,
full rebuild and local test suite (69/69) unchanged. Regenerated
configure via autogen.sh cleanly (valid autoconf/m4 syntax, only the
usual pre-existing AC_HEADER_STDC-obsolete warning, unrelated to this
change).
netmap_user.h defines its own function-like ND(...) debug-print macro guarded by #ifndef ND. A bare -DND (no value) makes the compiler define ND as the literal 1, so every real ND(...) call in netmap's headers expands to 1(...), a hard compile error. Discovered while verifying the CFLAGS-leak fix on real netmap+asan Linux hardware. Fixes #1015
configure: stop --with-netmap from leaking -I<DIR>/sys into CFLAGS
update Copyright to 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
4 participants
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
Merges v4.5.3 into master. Fixes from CHANGELOG:
--with-netmap=DIRpermanently leaking-I<DIR>/sysonto CFLAGS, causing bogusHAVE_BPF=1on Linux via netmap's vendoredsys/net/bpf.hshadowing the system header-DNDfrom--with-netmapCFLAGS/CPPFLAGS breaking netmap'sND(...)debug macro ([Bug] --with-netmap build fails with 'redefinition of struct bpf_insn' (--enable-asan on Linux) #1015)--tcp-sequenceand--portmap/-rsilently having zero effect on some platforms (confirmed macOS arm64) — wrong end-of-buffer pointer in rewrite_sequence.c/portmap.c ([Bug] macOS arm64 (Apple Silicon): 3 tcprewrite tests produce different output than macOS Intel / Linux arm64 - 2 real cross-platform bugs found #1011)catch_alarm()not callingpcap_breakloop(), causing spurious "remote host is not responding" timeouts (Simple tcpliveplay test #540)--fixcsum([Bug] Tcprewrite does not rewrite the contents of ICMPv6 error messages #818)zc:<ifname>PF_RING ZC device names failing with "ioctl: No such device" ([Bug]Tcpreplay with PF_Ring ZC - Failed to open device zc：ens160 #913)--netmapfailing to switch driver to bypass mode on real NICs, stray switch default leaking ioctl socket fd ([Bug] Failed to use netmap,nm_do_ioctl: No such file or directory#810)--loopreplays (Tcreplay hangs using --netmap and --loop options (ixgbe) #560)--multiplierin single and dual-interface (-I) replay ([Bug] Packets send date drift #724, Sleep based on fixed point in time to avoid time drift when replaying packets #915)--xdpAF_XDP zero-copy TX EINVAL on i40e/ixgbe requiring native XDP program ([Bug] Cannot run with--xdp: Packet sending exited with error #956, sendpacket: stop inhibiting libbpf's default XDP program load for AF_XDP TX #1002)--enet-vlan=addsilently truncating output by 4 bytes ([Bug] tcprewrite adding vlan tag with missing options results in broken output #990, tcprewrite - fix adding vlan tag with missing options #994)Test plan
sudo make testpassed during development of individual fixes