Fenrir fixes 2026 09 21 - #175
Open
danielinux wants to merge 17 commits into
Open
danielinux wants to merge 17 commits into
danielinux wants to merge 17 commits into
Conversation
ip_recv dropped every fragment before the local/forward decision, so a router build relayed none of them. RFC 1812 5.2.6 forbids reassembling transit fragments; the forwarding path (TTL, route, ARP, transmit) needs no reassembly. Move the drop to the local-dispatch entry: only a locally addressed fragment can reach it (reassembly still unimplemented, F-1326 scope), transit fragments fall through to the relay unchanged.
Retiring a payload-less segment popped the descriptor at the tail even when the cursor had advanced past it, discarding an unacked data descriptor that could never be retransmitted. Only pop when the cursor is at the tail; otherwise leave the descriptor in place with PKT_FLAG_SENT set and let tcp_ack() reclaim it from the tail.
udp_try_recv() delivered datagrams to wildcard (INADDR_ANY) bound sockets whenever the destination matched no constraint, so in non-forwarding builds (where ip_recv() compiles out its is_local check) an L2-adjacent sender could inject application traffic addressed to a third party. Gate delivery on the destination being local, a broadcast, a multicast, or the pre-address DHCP exchange (RFC 2131 OFFER/ACK), mirroring the ip_recv() forwarding-build policy and the checks tcp_input()/icmp_input() already apply.
tcp_persist_start() cancelled and re-inserted the zero-window persist timer on every call, and flush_tcp_tx() calls it on every poll while the peer window is zero. With a poll cadence shorter than TCP_PERSIST_MIN_MS the deadline was pushed forward before it could expire, so the probe never fired and a lost window-reopening ACK stalled the connection forever (RFC 9293 3.8.6.1). Make the start idempotent: return when the timer is already armed. Only tcp_persist_cb() re-arms, after a probe has been sent; it drops the stale handle first since handle_timers() does not clear it.
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Moderate issues remain in DHCP-port filtering and TCP FIFO processing, with a minor fragment-test coverage gap.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 1
What changed in this PR
This PR addresses UDP destination filtering, TCP persist timers/FIFO handling, IPv4 fragment forwarding, and related regression tests.
Changes:
- Updates networking behavior for UDP, TCP, and IPv4 fragments.
- Adds and revises regression coverage across unit tests.
- Remaining issues affect DHCP exception scoping and TCP FIFO processing.
| File | Summary |
|---|---|
src/wolfip.c |
Implements networking fixes; DHCP filtering and FIFO tail handling need changes. |
src/test/unit/unit.c |
Registers regression tests. |
src/test/unit/unit_tests_tcp_flow.c |
Tests TCP FIFO retention. |
src/test/unit/unit_tests_proto.c |
Tests fragment forwarding and dropping; local-delivery coverage is incomplete. |
src/test/unit/unit_tests_poll_dispatcher.c |
Tests persist probes. |
src/test/unit/unit_tests_dns_dhcp.c |
Updates UDP destination expectations. |
src/test/unit/unit_tests_api.c |
Tests non-local UDP rejection. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
The 67->68 exception in the udp_try_recv() gate was unconditional, so a datagram with those ports addressed to a third-party IP passed even with DHCP off and reached any wildcard bind on port 68. Now the gate requires DHCP_IS_RUNNING (excludes OFF and BOUND), and a third-party addressed 67->68 datagram may only match the DHCP client's own socket in the delivery loop. The F-11438 L2-group test now sets dhcp_state = DHCP_DISCOVER_SENT to match the OFFER/ACK scenario it models.
The tcp_ack() zero-length drain called fifo_pop() unconditionally, which removes the OLDEST descriptor. With a pure-ACK descriptor parked behind an unacked data descriptor (the state the F-14169 flush fix now leaves), an ACK covering the data marked it ACKED, then the drain popped it out from under the cursor: fresh_desc ended up NULL and the RTT sample plus cwnd growth for that ACK were skipped. Pop only when the cursor is on the oldest descriptor; otherwise leave the parked descriptor in place and advance the cursor - it is reclaimed when it becomes the oldest. New unit test pins it: pre-fix the test fails on rto_initialized == 0 (sample lost).
The fragment-relay fix moved the local-dispatch fragment drop below the forwarding block, so transit fragments again reach the router's ICMP generators and the SENDING filter hooks: - RFC 1812 4.3.2.7: a router must not generate an ICMP error for a non-first fragment (it cannot validate what the fragment does not carry). Guard the Parameter Problem, Time Exceeded and Fragmentation Needed sites in the forwarding path; the drops stay silent. - The SENDING filter hooks read the transport header at the IHL offset, which for a non-first fragment is payload: a port-based policy matched on garbage (RFC 1858 evasion). Skip the L4 notify for non-first fragments; the IP-level policy still applies. The first fragment (offset 0) carries a valid L4 header and is notified as usual. - Fix the local-dispatch drop comment: raw sockets and the IP-level filter do observe locally addressed fragments; what is never delivered is L4 data.
New src/test/unit/unit_tests_forwarding.c, self-contained (WOLFIP_ENABLE_FORWARDING=1), covers the branches the general ip_recv tests leave open: RFC 1812 4.3.2.7 silent drops for non-first fragments (TTL=1, DF oversize, malformed option) paired with the first-fragment reply cases, RFC 1812 4.3.2.4 multicast Parameter Problem exemption, and RFC 1858 L4 filter notify suppression for non-first fragments.
The two loopback-address drop tests injected via the demux helper, which bypasses the ip_recv source/destination gate they claim to verify. Build real frames and call ip_recv instead; the wildcard socket stays as the must-not-receive witness.
A socket bound before any interface had an address latched local_ip == 0 and was skipped by the ingress match for its whole life, while the stack answered port-unreachable for every datagram. Match on src_port != 0 (a bound slot) instead; liveness no longer rides on the egress snapshot. Tests cover the embedded start-up order for both protocols: UDP bind and TCP bind+listen before ipconfig_set, then delivery/accept after.
amd_eth_init returns -1 on alloc failure while the TRM port contract says TRM errors. The header now names the contract so the two are not read as a bug; no behavior change.
… failure With the timer heap full the persist probe would never fire while the flag claims it is active, stalling the sender on a zero-window peer. Only arm the flag when the insert succeeds. Test fills the heap and asserts the flag stays clear.
RFC 6298 5.7: if SYN retransmits used a sub-3s base RTO, reset the base to 3s when the control sequence completes. Test covers the reset and the no-timeout no-op case.
The single increment site caps dns_retry_count at DNS_QUERY_RETRIES, so the shift (max 3) can never overflow and the guard was dead code. The test now pins the reachable maximum shift instead of an unreachable state.
The quoted original is capped at ihl+8 without checking the declared datagram length, so short datagrams quote bytes past the packet (the sibling ICMP senders clamp via orig->len). Same clamp here. Test verifies a 24-byte datagram produces a 24-byte quote.
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
A moderate TCP TX FIFO issue can leave acknowledged data queued indefinitely.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 1
Resolved since last review (1)
The marking scan only walks SENT descriptors and the drain only ran when the segment marked new ones, so with [data1, pure-ACK, data2] all acked, data2 was marked ACKED behind the parked zero-length descriptor, never drained, and sat at the FIFO head forever: the scan stopped at it for every later segment, so new data was never marked acked and RTOs retransmitted already-acked segments until the connection died. The drain now runs on every ACK and reclaims zero-length SENT descriptors alongside ACKED ones. Regression test covers the parked case; the RTT-sample test now expects the parked descriptor reclaimed.
wolfSSL-Fenrir-bot
previously requested changes
Sep 21, 2026
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #175
Scan targets checked: wolfip-src, wolfip-bugs
Findings: 1
1 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Reported findings require changes before merge.
Fenrir review of 416eecd: the reclaim drain overwrote fresh_desc with the zero-length pure-ACK descriptor popped after the data, so the RTT sample came from the later pure-ACK time_sent and underestimated the RTT. Only ACKED descriptors now become the sample source. Pins the exact sample in test_tcp_ack_parked_zero_desc_keeps_rtt_sample (rtt == 10 from the data desc; red at 5 without the guard).
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #175
Scan targets checked: wolfip-src, wolfip-bugs
Fenrir result: Approved ✅
No new issues found in the changed files.
Advisory only — this automated result does not count as a GitHub approval.
wolfSSL-Fenrir-bot
dismissed
their stale review
September 21, 2026 11:09
Fenrir's latest completed scan found no issues; clearing the prior automated change request.
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.
32875e7 F-14188: wolfIP_send_port_unreachable: clamp quote to the datagram
a967115 F-14172: dns_schedule_timer: drop the dead overflow guard
87a74b6 F-8566: tcp_ctrl_rto_stop: reinitialize base RTO after a control timeout
4a049f0 F-14171: tcp_persist_start: do not set persist_active on timer insert failure
dc6e906 F-14174: gem.h: state the -1 vs TRM error code contract
313896b F-14187: deliver wildcard-bound UDP after the address arrives
e138a32 F-14179: no ICMP errors or L4 filter matches for non-first frags
299980c F-14169: keep RTT sample when a zero-length desc is parked
0e7308f F-14186: scope UDP DHCP exception to an active client
0c7345c F-14170: keep armed persist timer deadline across polls
3790531 F-14186: drop UDP datagrams addressed to non-local destinations
2f48f0d F-14169: guard flush_tcp_tx fifo_pop to the FIFO tail
45df1a3 F-14179: relay transit IP fragments in the forwarding path