Fenrir fixes 2026 08 21 - #158
Open
danielinux wants to merge 14 commits into
Open
Conversation
flush_raw_tx and flush_packet_tx discarded the return of wolfIP_ll_send_frame and popped the descriptor unconditionally, so a retryable -WOLFIP_EAGAIN (loopback queue full, driver TX ring full) silently dropped a frame that wolfIP_sock_sendto had already reported as queued. Mirror flush_datagram_tx: break out of the drain loop on a negative send result, leaving the descriptor at the FIFO head for the next poll cycle. Adds regression tests driving the mock link into -WOLFIP_EAGAIN for both a raw socket and an AF_PACKET socket: the frame must not be transmitted on the backpressured poll, the descriptor must survive in the TX FIFO, and the next poll must retransmit it intact.
The raw and AF_PACKET loops in handle_socket_callbacks() cleared r->events / p->events after the callback returned, inverting the order dispatch_events() (UDP/ICMP) and the TCP path use. An event raised on the same slot while the callback is executing (e.g. the callback closes the socket and re-opens a new one in the reused slot) was wiped by the stale post-callback clear, so a consumer waiting on it never woke. Snapshot the events and clear the field before invoking the callback, mirroring dispatch_events(). Adds regression tests for both socket types: a callback that closes and re-opens the socket in place raises CB_EVENT_WRITABLE on the reused slot; it must survive the poll and wake the reopened socket's callback on the next one.
dns_callback validated that the complete RDATA lies inside the DNS message but then passed the full message length to dns_copy_name, whose label bound checks use that length. The inline portion of a PTR answer name could therefore continue past the RDATA into the following record: with rdlength 1 holding a label-length byte of 3 and "foo" in the bytes after, the parser returned "foo" and invoked dns_ptr_cb with a name the RDATA never contained. dns_copy_name now takes the RDATA edge (rdata_end) as a sixth argument: the initial inline portion (labels, terminators and both bytes of a compression pointer) must fit in the RDATA, while pointer targets and the post-jump name portion remain validated against the message length, as RFC 1035 s4.1.4 allows. The PTR arm passes pos + rdlen; direct buffer unit tests pass rdata_end == len (behavior unchanged). Adds the finding's trigger as a regression test (undersized RDATA followed by name-looking bytes must leave the query pending) plus a companion test that a PTR RDATA legitimately ending in a compression pointer still parses.
parse_http_request() splits the request target on '?' and populates
req.query unconditionally, before the method is validated against
GET/POST. A POST target with a query string ("POST /api?id=5") gets
req.query filled exactly like a GET, so the "(for GET requests)"
comment was false and could mislead a handler into skipping
httpd_get_request_arg() on POST.
test_multicast_igmp_query_spoofed_dropped only asserted that no frame was sent synchronously after each spoofed query - but IGMP reports are always deferred to a timer (RFC 3376 s5.2), so the assertion was trivially true whether the query was dropped or accepted. Deleting the TTL guard (ip->ttl != 1) or the destination guard (dst != IGMP_ALL_HOSTS && dst != group) survived the suite: the spoofed case silently armed a report timer, the later compliant case coalesced into it per the s5.2 pending-response rule, and the single final poll emitted exactly one report, satisfying the closing assertion. Each spoofed case now also asserts that no report timer was armed (s.mcast[i].tmr_report == NO_TIMER), then polls past the Max Resp window (10 s) and asserts that still nothing was sent. Cases run at distinct tick marks (t=0, 10001, 20001) so a report armed by a mutated guard cannot be hidden by the compliant case's coalescing. The compliant case now asserts the timer is armed and that exactly one report is emitted. Mutation-checked (make unit-multicast): deleting the TTL guard fails the case-1 NO_TIMER assert, deleting the destination guard fails the case-2 NO_TIMER assert, && -> || fails the compliant case (plus the existing refresh/flood tests), and != 1 -> == 1 fails case 1.
fifo_align_head_pos() wraps an unaligned head in {size-3, size-2, size-1}
to 0, but unlike the explicit end-of-buffer branch it never records the
wrap in h_wrap. When the FIFO is non-empty and h_wrap is 0, the collapsed
head==tail==0 && h_wrap==0 state is indistinguishable from the empty
state: the space test in fifo_push reports the whole buffer as free and
the next push writes a fresh descriptor at offset 0, clobbering every
previously queued descriptor (silent loss of UDP/ICMP/raw datagrams). A
second variant — a wrap write ending exactly on tail — left a non-empty
FIFO that reported empty and orphaned all live descriptors.
Record the wrap (h_wrap = pre-alignment head) in fifo_push when
alignment collapses a non-zero head to 0 on a non-empty, not-yet-wrapped
FIFO, and mirror the same rule in fifo_can_push_len so upstream capacity
checks agree with the fixed empty/full test. A rejected push mutates no
state; a recorded h_wrap is cleared by the existing fifo_pop drain path.
Regression tests: (1) a descriptor filling [0, size-2) with tail 0 must
make the next push fail with -1 and leave the queued descriptor intact;
(2) the wrap-lands-on-tail sequence must leave the FIFO reporting
non-empty with the oldest live descriptor still reachable via
fifo_peek. Both were verified to fail against the unfixed code (the push
returned 0 and clobbered / the FIFO reported empty) and pass with the
fix, across the plain, IP_MULTICAST and VLAN unit builds.
The addr_match expression relaxed all peer and destination validation whenever a socket had no local address (local_ip == 0) while the DHCP state machine was running: ((t->local_ip == 0) && DHCP_IS_RUNNING(s)). That relaxation exists so the DHCP client socket can receive OFFER/ACK before it owns an address, but as written it applied to every socket in s->udpsockets[] - so a connected application socket created before the interface had an address accepted datagrams from any source address and port for as long as local_ip stayed 0 (initial acquisition and every RENEWING/REBINDING cycle), bypassing the connected-peer filter. Compute is_dhcp (the socket's fd equals s->dhcp_udp_sd) and require it in the relaxation clause, keeping peer_match in force for every other socket regardless of local_ip. The DHCP socket is unconnected (peer_match is already 1) and keeps local_ip 0, so OFFER/ACK delivery is unchanged. Adds test_udp_dhcp_relaxation_scoped_to_dhcp_socket: a connected socket with local_ip 0 must not receive a datagram from a non-connected peer while DHCP runs, and the DHCP socket must still receive one from any source. Verified RED (the app socket received the spoofed datagram pre-fix) and GREEN. test_udp_try_recv_dhcp_running_local_zero, which codified the old over-broad relaxation on a non-DHCP socket, now marks its socket as the DHCP socket to keep asserting the intended relaxation. Plain, IP_MULTICAST and VLAN unit builds pass.
…ener In tcp_input the per-socket match gated the 4-tuple comparison on state > TCP_LISTEN, so a TCP_LISTEN socket was matched on local port alone. The SYN handler separately validates bound_local_ip, but a non-SYN segment (data/ACK/FIN) for the same port and a different local address on the same host fell through to the shared bookkeeping writes - t->if_idx, t->last_pkt_ttl, matched = 1, and (for non-RST) t->sock. tcp.peer_rwnd - before any destination-address check. That lets a peer that only needs to know a specifically-bound listener's port corrupt the listener's MTU/TTL bookkeeping, seed the initial window of a later accepted child, and set matched so the RFC 793 unmatched-segment RST is suppressed. Add an else branch so a listener in TCP_LISTEN is skipped when its bound_local_ip is a specific address that does not equal the segment's destination. bound_local_ip (not local_ip) is the discriminator: a 0.0.0.0 bind leaves local_ip set to the interface/primary address as a default source, so local_ip cannot tell a wildcard listener from a specifically-bound one. Wildcard listeners are unaffected (no-op), and the established-socket path (state > TCP_LISTEN) is unchanged. Note on the finding's RST-blackhole claim: a TCP_CLOSED socket with a non-zero src_port never reaches the port-match gate - close_socket() and every RX-path teardown zero proto (first guard) or set CB_EVENT_CLOSED (second guard) and always zero src_port, so a matchable TCP_CLOSED slot only matches invalid port-0 segments. That part of the report does not manifest; the local-address validation gap for listeners is real and is what this commit closes. Adds test_tcp_listen_requires_matching_local_ip: a non-SYN segment for a bound listener's port but a different local address must not change last_pkt_ttl, while one for the bound address still matches. Verified RED (last_pkt_ttl became 64 pre-fix) and GREEN. Plain, IP_MULTICAST and VLAN unit builds pass.
The per-socket match loop recomputed expected_len = ee16(udp->len) + IP_HEADER_LEN + ETH_HEADER_LEN and bailed with if ((int)frame_len < (int)expected_len) return;. That branch is unreachable: the unconditional guard before the socket loop already rejects any datagram where ee16(udp->len) > frame_len - ETH_HEADER_LEN - IP_HEADER_LEN, i.e. it guarantees frame_len >= expected_len. frame_len is pass-by-value and udp->len is not modified between the two, so the inner comparison is always false for every caller (the dispatch path presents an option-stripped IHL=5 header; the loopback multicast caller builds frames where the two quantities are exactly equal). Remove the dead comparison and its now-unused expected_len local. No behavioral change; the plain, IP_MULTICAST and VLAN unit suites pass unchanged.
The header described ssh_server_get_uptime as returning the SSH server uptime in seconds, but the implementation returns the constant 0 until a main-loop tick source is integrated, so the "uptime" SSH command always reports zero. Correct the API-contract comment to state that it is a placeholder returning 0. No behavioral change.
The WOLFTFTP_REQ_BUF_MAX comment totaled the worst-case RRQ/WRQ as 63 + MAX_FILENAME, assigning 12 bytes to the timeout option. But timeout_s is an unclamped uint16_t serialized by wolftftp_append_opt, which includes both key and value terminators: 65535 becomes "timeout\0" (8) + "65535\0" (6) = 14 bytes. The correct total is 65 + MAX_FILENAME. The WOLFTFTP_REQ_BUF_MAX allocation (MAX_FILENAME + 128) already covers this with margin to spare, so no behavioral change - the fix stops a maintainer from using the documented calculation to reduce the margin below a valid maximal request.
dns_callback only parsed an incoming datagram as a DNS response when (flags & DNS_FLAGS_RESPONSE_RD) == DNS_FLAGS_RESPONSE_RD, i.e. when both the QR (query/response) and RD (recursion-desired) bits were set (0x8100). Per RFC 1035 s4.1.1 the QR bit alone distinguishes a response from a query; RD is merely the Recursion-Desired flag a server echoes from the query. A conformant server that does not echo RD (a response otherwise lacking the RD bit) therefore had its reply silently ignored, letting the outstanding query time out and retransmit needlessly. Add a DNS_FLAGS_RESPONSE macro for the QR bit and gate response parsing on it. DNS_FLAGS_RESPONSE_RD is retained: it is still a valid QR|RD response flags value used by the test helpers to build conformant replies. Adds test_dns_callback_qr_without_rd_is_accepted: a response with QR set and RD clear must be parsed and the lookup delivered. Verified RED (lookup not delivered pre-fix) and GREEN. Plain, IP_MULTICAST and VLAN unit builds pass.
After dispatching a TCP socket callback, handle_socket_callbacks() re-read ts->sock.tcp.state and, if TCP_CLOSED, disarmed the callback and called close_socket(), which memsets the whole slot. ts is a fixed slot address, so that check is purely positional. A close callback that closes the socket via wolfIP_sock_close() (freeing the slot) and then allocates a fresh one - tcp_new_socket() scans from index 0 for the first proto == 0 slot, so it can land in the very same slot - leaves the slot holding a brand-new socket whose state is TCP_CLOSED by design. The positional reap then destroyed that fresh socket (zeroing S, proto and the FIFO backing), leaving the application with a permanently dead descriptor. Capture the callback/callback_arg pair before invoking the callback and only reap if both are unchanged afterward: a replaced slot carries a different (or no) callback, so the reap no longer touches it. The normal deferred-close path is unaffected - there the callback does not replace the socket, so the pair is unchanged and the reap proceeds as before. A socket the callback closed without re-creating is already memset (callback NULL), so it is likewise left alone (close_socket would be a no-op). Adds test_handle_socket_callbacks_keeps_recreated_socket: a socket left in the RX-deferred close state (TCP_CLOSED + CB_EVENT_CLOSED) whose callback closes it and re-creates a socket in the same slot must survive the dispatcher's post-callback reap. Verified RED (slot proto zeroed pre-fix) and GREEN. Plain, IP_MULTICAST and VLAN unit builds pass.
In the WOLFIP_RAWSOCKETS branch of wolfIP_sock_sendto(), total_len = ETH_HEADER_LEN + (uint32_t)len narrows the public size_t len to uint32_t before the total_len > LINK_MTU guard, but both payload copies - the ipheader_include memcpy and the non-ipheader memcpy(rip->data, buf, len) - use the original, un-narrowed len. On a 64-bit build a len of UINT32_MAX + 100 narrows to 100 for the MTU check (which passes) while the memcpy still attempts to copy the full, enormous len into the fixed-size frame buffer, overflowing it. Reject len > LINK_MTU as size_t before any narrowing conversion: a payload larger than the frame capacity cannot be sent anyway, and the bound is far below UINT32_MAX so the subsequent (uint32_t)len is exact and the total_len arithmetic cannot wrap. No behavioral change for any valid payload. The unit build enables WOLFIP_RAWSOCKETS unconditionally (unit_shared.c), so the test runs in all unit builds. test_raw_sendto_rejects_oversized_len _before_narrowing pins the bound: the oversized length is refused with -WOLFIP_EINVAL. Verified RED - with the bound check removed the same call segfaults (signal 11) in the payload memcpy - and GREEN. Plain, IP_MULTICAST and VLAN unit builds pass.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR tightens correctness and safety across wolfIP’s socket I/O paths and protocol parsing, addressing multiple edge-case regressions (FIFO wrap accounting, UDP/DHCP matching, TCP LISTEN matching, DNS response/name parsing, and raw/packet socket callback/TX retry semantics) and adds unit coverage to lock in the fixes.
Changes:
- Fix FIFO alignment-induced wrap bookkeeping (and mirror it in capacity checks) to prevent “non-empty looks empty” corruption.
- Harden socket/protocol paths: scope UDP DHCP relaxation to the DHCP socket, enforce LISTEN
bound_local_ipfor non-SYNs, accept DNS responses by QR bit alone, and bound PTR-name parsing to RDATA. - Improve callback/TX robustness: prevent TCP close-callback reaping from destroying a re-created socket, clear raw/packet socket events pre-callback, and keep raw/packet TX descriptors queued on link-layer backpressure.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/wolfip.c | Core fixes across FIFO wrap handling, UDP/TCP receive matching, DNS parsing, callback dispatch safety, and raw/packet TX retry behavior. |
| src/tftp/wolftftp.h | Correct worst-case request-size documentation for timeout option sizing. |
| src/test/unit/unit.c | Registers new unit tests covering the added regressions/edge cases. |
| src/test/unit/unit_tests_tcp_ack.c | Adds regression test ensuring callback-driven close+recreate doesn’t get reaped incorrectly. |
| src/test/unit/unit_tests_proto.c | Adds protocol-level regression tests for DHCP scoping, raw/packet TX EAGAIN retry, and LISTEN local-IP binding. |
| src/test/unit/unit_tests_poll_dispatcher.c | Adds regression tests ensuring events raised during callback re-entrancy aren’t wiped for raw/packet sockets. |
| src/test/unit/unit_tests_multicast.c | Strengthens IGMP anti-disclosure test to assert no deferred report is armed for spoofed queries. |
| src/test/unit/unit_tests_fifo.c | Adds targeted FIFO alignment-wrap regression tests to prevent descriptor clobber/empty misreporting. |
| src/test/unit/unit_tests_dns_edges.c | Adds DNS edge-case tests for QR-only responses and PTR RDATA-bounding behavior; updates dns_copy_name calls for new signature. |
| src/test/unit/unit_tests_dns_dhcp.c | Updates DHCP-running local_ip==0 test to mark the socket as the DHCP socket for the newly-scoped relaxation. |
| src/test/unit/unit_tests_branches.c | Adds regression test for oversized raw sendto length rejection prior to narrowing. |
| src/test/unit/unit_tests_api.c | Updates dns_copy_name calls for the new rdata_end parameter. |
| src/port/stm32h563/ssh_server.h | Documents ssh_server_get_uptime() as a placeholder returning 0 for now. |
| src/http/httpd.h | Clarifies http_request.query comment to apply to any method when the target contains a query. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
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.
c977ef9 F-8525: reject oversized raw-socket payloads before narrowing len
f0e57b2 F-8523: key the handle_socket_callbacks reap on socket identity
2ad91d7 F-6211: detect DNS responses by the QR bit alone
c10690d F-9380: correct TFTP worst-case request-size comment for timeout
49cedb0 F-9374: document ssh_server_get_uptime as a placeholder
ead1ca0 F-6471: remove dead UDP datagram length re-check in udp_try_recv
e275c35 F-10281: validate bound_local_ip for non-SYN segments to a bound listener
c268ae1 F-10280: scope the udp_try_recv DHCP relaxation to the DHCP socket
9ffc674 F-8521: record the alignment-induced head wrap in fifo_push
8ac7717 F-6475: prove the IGMP anti-disclosure guards are actually tested
f21c6da F-9366: fix http_request.query comment to cover non-GET requests
855ad2c F-10260: bound the PTR name walk to the RDATA, not the whole message
5815e8a F-10259: clear raw/packet socket events before dispatching the callback
407178b F-6209: keep raw/packet TX descriptor queued on driver backpressure