Fenrir 2026 09 16 - #174
Conversation
The SYN_SENT handler sent the handshake ACK and fell through into the synchronized-state branch, which saw the consumed SYN-ACK as out-of-window and queued a second, redundant ACK on every active open. Continue past it; add a unit test asserting exactly one ACK is queued for the handshake.
A forward ACK can advance snd_una while the marking loop counts zero descriptors (retransmit-marked or partially covered head), so the ack_count gate miscounted it as a duplicate, triggering fast retransmit one ACK early. Reject forward ACKs in the dup arm, keeping the stale-ACK rejection; add a unit test.
The field was written on two transmit paths (tcp_send_empty_immediate, flush_tcp_tx) and never read anywhere; one unit test asserted its refresh. Remove the field, both assignments, and the test references. No behaviour change.
The heap compares deadlines in the 32-bit tick domain, so a renewal past INT32_MAX ticks (lease > 24.8 days) reads as already due and the BOUND callback drops it, leaving the lease timers disarmed and the lease to expire silently. Long deadlines are now driven by bounded checkpoints (now + INT32_MAX, re-armed by the callback until representable); RENEWING/REBINDING retry schedules are bounded the same way. Also lands F-12388: dhcp_lease_ip_sane rejects host-bit-set and network-broadcast addresses. Verification: make build/wolfip.o && make unit -> 1559/1559. Pre-fix, test_dhcp_long_lease_renewal_checkpoint_rearm fails on s.dhcp_timer == 0 after DAD (fix stashed); with the fix the checkpoint re-arms through T1 and the renewal starts exactly at T1.
dhcp_parse_offer wrote s->dhcp_server_ip inside the option loop, before the end-of-stream validation, so a rejected OFFER (bad msg type, missing END, unsane lease IP) committed the attacker's server identifier into stack state. No reader is reachable from DISCOVER_SENT today - every consumer (ACK cross-check, REQUEST, DECLINE) only runs after a successful parse re-wrote the field - but the write breaks the function's own collect-then-validate-then-commit invariant. Collect into a local and commit with the rest. Verification: make build/wolfip.o && touch src/test/unit/unit.c && sleep 1 && make unit && ./build/test/unit -> 1560/1560. Pre-fix, test_dhcp_parse_offer_reject_does_not_commit_server_id fails with s.dhcp_server_ip == 0x0A000001 (fix stashed).
RFC 2131 4.4.2: after declining a conflicted address the client waits 10 s before a new DISCOVER so the server can stop leasing the address. The DAD-conflict path sent DECLINE and re-DISCOVER in the same tick, so the client re-acquired the same address in a conflict loop. The wait is armed as the DHCP timer and a new DHCP_OFF case in the timer callback drives the re-DISCOVER; the conflict wait is the only path that arms a timer while OFF (NAK and lease-expiry restarts call dhcp_send_discover directly).
The GCM constructor is the only one with a fallible step (pre-iv RNG) after the slot is filled; its state restore ran even after the failure wipe, querying the read callback with the wiped zero SPI. Return -1 right after the wipe so restore runs on success only. Adds test_esp_state_restore_gcm pinning the GCM success-path restore (persisted window applied, read callback called once with the SPI).
flush_datagram_tx serves both UDP and ICMP sockets, but the post-drain writable event was gated on is_udp. An ICMP sender that got -WOLFIP_EAGAIN from a full txbuf stayed blocked after the drain until some unrelated event reached the socket. Raise the event for both protocols; the loopback path already handles itself via wolfIP_notify_loopback_space_available().
The ICMP bind arm had no final fallback: re-binding IPADDR_ANY while no interface holds a configured address retained the previous local_ip, so the socket kept sending with an address the stack no longer owned. Reset to IPADDR_ANY like the TCP and UDP arms.
The option walker in ip_recv silently dropped transit datagrams whose IP options failed length validation. RFC 1122 3.2.2.4 requires a Parameter Problem (type 12) with the pointer at the offending option byte. Model the reply on the existing ttl-exceeded constructor, including the error-in-response-to-error suppression; multicast destinations are exempt per RFC 1812 4.3.2.4. Locally addressed packets are still dropped silently.
The router portion of ICMP Router Discovery (solicited/periodic advertisements, solicitation handling, RFC 1256 configuration variables) is intentionally not implemented: wolfIP hosts are configured via DHCP or static config, routing is connected plus static only, and there is no host-side consumer in the ecosystem. Record the deviation in the forwarding how-to next to the fragmentation deviation, mark the icmp_input fall-through, and correct the stale Fragmentation Needed sentence the code superseded.
Accept clones carry the listener's callback, so closing one on an error path (SYN-ACK send failure, accepting-filter rejection, and the pre-accept struct-copy path) deferred a CB_EVENT_CLOSED for a descriptor accept() never returned. Route the abort through abort_accept_clone(), which clears the callback first.
UDP connect stores dst_port/remote_ip but getpeername only handled TCP (and raw), so a connected UDP socket returned an error instead of its peer. Add the UDP branch: connected sockets report the peer, unconnected ones return -1 like the raw no-remote-ip case.
A raw-socket header declaring more header bytes than were supplied made the dst-override checksum recompute iterate past the initialized frame, folding stale stack bytes into the checksum. Require IPv4 and IHL*4 in [20, len] before trusting the header; the FIFO push already bounds the wire bytes to what was supplied.
…socket tables wolfIP_vlan_delete() refuses to free a slot while interface-indexed state still references it, because wolfIP_vlan_create reuses freed slots. The scan covered routes, multicast, TCP, UDP and raw sockets but missed icmpsockets[] and packetsockets[], which bind an if_idx the same way: a socket left on the deleted index would silently transmit and receive through the next VLAN created in that slot. Add both scans using each table's own liveness field (proto != 0 for the tsocket ICMP table, used for packetsockets, under the same WOLFIP_RAWSOCKETS/WOLFIP_PACKET_SOCKETS nesting as the struct definitions). Test test_vlan_delete_rejected_with_icmp_socket: an ICMP socket on the VLAN sub-iface makes delete return -WOLFIP_EBUSY, the socket survives, close releases the dependency and delete then succeeds. Verified in the unit-vlan build (make unit-vlan): pre-fix the test fails with ret == 0 (delete succeeds), post-fix 1615/1615; plain make unit 1570/1570.
wolfIP_print_ip labeled the columns (ipv, hdr_len) but printed a hard-coded 0x04 and the packed ver_ihl byte, misreporting packets with IP options or malformed versions. Print ver_ihl >> 4 for the version and (ver_ihl & 0x0f) * 4 for the header length in bytes.
wolfIP_print_udp early-returned before the closing rows when the datagram had no payload; guard only the payload preview on len > UDP_HEADER_LEN so the dump always completes. Same pass: hoist the preview locals out of the anonymous block and use UDP_HEADER_LEN instead of the magic 8.
The doc promised the cached PMK skips SAE/EAP authentication entirely, but pmksa_reconnect rejects any cache without a PMKID (supplicant.c:1762) and the PMKID is only stored for SAE auth (supplicant.c:944-947). State the PMKID precondition and that EAP/PEAP sessions never satisfy it.
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved moderate findings affect IPv4 option handling, DHCP timing, TCP ACK cleanup, and packet-socket VLAN coverage.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
This PR bundles Fenrir networking fixes across IPv4, TCP, DHCP, socket APIs, ESP, diagnostics, tests, and documentation.
Changes:
- Improves IPv4 validation, ICMP behavior, VLAN checks, UDP peer reporting, and debug output.
- Fixes TCP ACK/teardown, DHCP timing, PMKSA reconnect, and ESP GCM handling.
- Adds regression tests and updates networking documentation.
File summaries
| File | Reviewed changes and final notes |
|---|---|
src/wolfip.c |
Core protocol changes. Moderate findings: validate malformed source-route options before dropping them (2 votes); preserve the DHCP decline wait state (1); bound initial lease/DAD timers (1); reconcile acknowledged retransmission descriptors (1); guard packet-socket scans independently of raw sockets (1). Nit: add packet-socket-only VLAN coverage (1). |
src/wolfip_debug.c |
Corrected IP and UDP debug output. |
src/wolfesp.c |
GCM security-association restoration handling. |
src/test/unit/unit.c |
Registered new unit tests. |
src/test/unit/unit_tests_vlan.c |
VLAN socket-dependency tests. |
src/test/unit/unit_tests_tcp_flow.c |
TCP handshake regression tests. |
src/test/unit/unit_tests_tcp_ack.c |
TCP ACK behavior tests. |
src/test/unit/unit_tests_socket_api_arms.c |
IP header validation and UDP peer tests. |
src/test/unit/unit_tests_proto.c |
Forwarding regression expectations. |
src/test/unit/unit_tests_poll_dispatcher.c |
ICMP writable-event tests. |
src/test/unit/unit_tests_ip_arp_recv.c |
Malformed-option tests. Moderate finding: correct the checksum assertion’s ICMP payload pointer (1 vote). |
src/test/unit/unit_tests_dns_dhcp.c |
DHCP and protocol regression tests. |
src/test/unit/unit_tests_dhcp_edges.c |
DHCP edge-case tests. |
src/test/unit/unit_tests_api.c |
ICMP bind-state regression tests. |
src/test/unit/unit_esp.c |
GCM state-restoration tests. |
src/supplicant/supplicant.h |
PMKSA reconnect contract documentation. |
docs/advanced_ipv4_howto.md |
Forwarding and router-discovery documentation. |
Review details
Suppressed comments (6)
src/test/unit/unit_tests_ip_arp_recv.c:1543
- This test casts the ICMP payload start to
struct wolfIP_icmp_packet, but that struct includes the Ethernet and IP prefix, soic->csumreads 36 bytes past the actual checksum. The expected value is correctly computed from the frame start; pointicatlast_frame_sent(or read the checksum at the payload offset) so the regression assertion checks the transmitted checksum rather than unrelated buffer data.
ic = (struct wolfIP_icmp_packet *)(last_frame_sent +
ETH_HEADER_LEN + IP_HEADER_LEN);
ck_assert_uint_eq(ic->csum, ee16(icmp_checksum(
(struct wolfIP_icmp_packet *)last_frame_sent,
(uint16_t)(8 + 24 + 8))));
src/wolfip.c:10710
- The new AF_PACKET dependency branch is not covered by the added regression tests—the VLAN test only exercises ICMP. Add a
WOLFIP_PACKET_SOCKETS-guarded test that binds an AF_PACKET socket to the VLAN, verifies deletion returns-WOLFIP_EBUSY, and verifies deletion succeeds after closing it; this is especially important because the packet table is conditionally compiled.
#if WOLFIP_PACKET_SOCKETS
for (i = 0; i < WOLFIP_MAX_PACKETSOCKETS; i++) {
if (s->packetsockets[i].used &&
s->packetsockets[i].if_idx == (uint8_t)if_idx)
return -WOLFIP_EBUSY;
src/wolfip.c:10103
- This leaves the state as
DHCP_OFFwhile the 10-second timer is pending, butdhcp_client_init()treats anyDHCP_OFFstate as immediately restartable. If the public API is called during this window, it sends a DISCOVER immediately and overwritesdhcp_timerwhile the original wait timer remains in the heap; when that timer fires, the newDHCP_OFFcase sends another DISCOVER. Keep the decline-wait distinguishable from an idle OFF state (or reject init while this timer is active) so the delay and single-timer invariant hold.
dhcp_schedule_timer_at(s, s->last_tick + DHCP_DECLINE_WAIT_MS);
src/wolfip.c:9179
- This checkpoint rearm is only reached after an unbounded lease timer has already fired.
dhcp_schedule_lease_timer()and the DAD-completion branch still insertdhcp_renew_atdirectly, so deadlines beyond the 32-bit signed horizon can be interpreted as early or late bytick_expired()before this recovery runs. Bound the initial lease/DAD timer insertion as well as this callback rearm.
dhcp_schedule_timer_at(s,
dhcp_bounded_at(s, s->dhcp_renew_at));
src/wolfip.c:5702
- When a fast-recovery marker clears
PKT_FLAG_SENT, a cumulative ACK can advancesnd_unawhile the ACK walk counts zero descriptors. This guard avoids inflatingdup_acks, but the coveredPKT_FLAG_RETRANSdescriptor remains queued; if the retransmission has not been sent yet,flush_tcp_tx()will transmit data that was already acknowledged. Reconcile or remove descriptors covered by a forward ACK before returning.
if (ack_advanced || ack != t->sock.tcp.snd_una)
src/wolfip.c:10706
- The new AF_PACKET dependency scan is nested inside
#if WOLFIP_RAWSOCKETS, butconfig.hand the migration documentation exposeWOLFIP_PACKET_SOCKETSindependently. A packet-socket-only configuration therefore does not compile or run this scan, so the VLAN delete guard does not cover the documented configuration; keep the packet-socket storage and scan under their own feature guard.
#if WOLFIP_PACKET_SOCKETS
- Files reviewed: 17/17 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
A malformed LSRR/SSRR option hit the source-route drop before the length check ran, so it was silently dropped instead of getting a Parameter Problem. Validate the option length first; the source-route drop now only applies to well-formed source routes. Adds a forwarding regression test for the malformed-source-route Parameter Problem.
There was a problem hiding this comment.
🟡 Changes recommended
Four unresolved findings remain, including one critical VLAN/ESP crash risk.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (3)
Previously missed (1) — in code that hasn't changed since the last review.
src/supplicant/supplicant.h:420
- This contract is not enforced by
wolfip_supplicant_pmksa_reconnect(): it only checks the cached PMKID/SSID/BSSID, notauth_mode. If a context is reinitialized from SAE to PSK or EAP/PEAP and this API is called, it returns success with an SAE PMKID/PMK for the wrong AKM; EAP still starts EAPOL authentication while PSK uses the SAE PMK. Reject reconnects unless the current mode is SAE, or persist and validate the cache's source authentication mode.
src/wolfip.c:10710
- This AF_PACKET dependency scan is nested inside the
#if WOLFIP_RAWSOCKETSblock, butconfig.hexposesWOLFIP_PACKET_SOCKETSas a separate feature flag and the other packet-socket paths use its own guard. In a packet-socket configuration without raw sockets, this new scan is omitted, so a packet socket bound to the VLAN will not keepwolfIP_vlan_delete()busy. Guard the packet table independently and keep its storage consistently available under the packet-socket feature.
#if WOLFIP_PACKET_SOCKETS
for (i = 0; i < WOLFIP_MAX_PACKETSOCKETS; i++) {
if (s->packetsockets[i].used &&
s->packetsockets[i].if_idx == (uint8_t)if_idx)
return -WOLFIP_EBUSY;
src/wolfip.c:11082
- This response is only reached inside
if (out_if >= 0). A malformed-option datagram that passes RPF but has no connected or static route skips this block and returns at the no-route path, so it is still silently dropped instead of receiving the new Parameter Problem. Handle the malformed-option case before theout_if-dependent forwarding decision (while retaining the multicast/RPF suppression rules).
if (bad_opt_off != 0) {
/* RFC 1122 3.2.2.4: a transit datagram with a malformed
* IP option gets a Parameter Problem pointing at the
* offending option byte, not a silent drop. Multicast
- Files reviewed: 17/17 changed files
- Comments generated: 1
- Review effort level: Lite
esp_send() was passed the VLAN child ll, whose send function is intentionally NULL (tx delegates to vlan_parent), crashing whenever ESP is enabled and a malformed transit packet arrives on a VLAN sub-interface. Resolve the VLAN parent for the ESP call, matching the existing Fragmentation Needed path. Found by Copilot review on PR 174 (follow-up to F-13189).
There was a problem hiding this comment.
🔵 Needs a closer look
The VLAN packet-socket guard issue remains, and the GCM RNG-failure path lacks regression coverage.
Review details
Suppressed comments (2)
Previously missed (1) — in code that hasn't changed since the last review.
src/wolfip.c:10719
- This new AF_PACKET dependency scan is nested inside the surrounding
#if WOLFIP_RAWSOCKETS, so it is compiled only when raw sockets are enabled too.WOLFIP_PACKET_SOCKETSis an independent configuration option (config.h:46-53,docs/migrating_from_lwIP.md:95); in that configuration a VLAN deletion can skip a bound packet socket and invalidate its interface index. Guard the packet-socket loop independently (and make the packet table definition follow the same independent guard).
src/test/unit/unit_esp.c:2420
- This regression test exercises only the successful GCM construction path, but the production change at this line fixes the RNG-failure path by preventing
esp_state_restore()from running after the slot is wiped. Without a test that forceswc_RNG_GenerateBlock()to fail and verifies the read callback is not invoked and the slot remains reusable, the behavior being fixed is still unprotected.
/* F-11436: the GCM constructor is the only SA constructor with a
* fallible step (pre-iv RNG) after the slot is filled. Its restore must
* run on the success path only; on the RNG failure path the SA is wiped
* and -1 returned, without querying the read callback with the wiped
* zero SPI. This test pins the success path: a GCM SA recreated with a
* known SPI picks up its persisted window. */
- Files reviewed: 17/17 changed files
- Comments generated: 0 new
- Review effort level: Lite
6fde1df F-12394: correct the PMKSA fast reconnect contract
0164138 F-12393: fix the UDP debug dump for empty payloads
81a04d2 F-12389: print real version and header length in the IP debug dump
0af8f24 F-13766: extend the VLAN delete EBUSY scan to the ICMP and AF_PACKET socket tables
1bfb480 F-13207: validate caller IHL in the IP_HDRINCL sendto path
9c6ff95 F-13167: getpeername for connected UDP sockets
46a18a2 F-12390: plain teardown for aborted accept clones
285d6f2 F-13190: document the RFC 1256 router discovery deviation
2daf3f1 F-13189: send ICMP Parameter Problem for malformed forwarded IP options
f871593 F-12391: clear stale local_ip on ICMP bind to ANY
3bd8d7a F-11435: raise CB_EVENT_WRITABLE when the ICMP txbuf drains
2481b00 F-11436: restore GCM SA state only on successful construction
151c6f5 DHCP: wait 10 s after DECLINE before re-DISCOVER (F-13776)
60fe112 F-13764: collect the DHCP server ID like the other offer fields
a3e502f F-13206: bound DHCP lease timer deadlines to the heap horizon
ff18e66 F-13767: remove dead tcpsocket field last_ack
4d0c3cf F-13765: gate tcp_ack dup-ACK arm on forward progress
36ff9bb F-11434: consume SYN-ACK after handshake completion in tcp_input