bisect-a: first 7 fenrir fixes (temporary, do not merge) - #160
Closed
danielinux wants to merge 7 commits into
Closed
bisect-a: first 7 fenrir fixes (temporary, do not merge)#160danielinux wants to merge 7 commits into
danielinux wants to merge 7 commits into
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.
Contributor
There was a problem hiding this comment.
Pull request overview
Temporary bisect branch that bundles several targeted correctness fixes in the wolfIP core (FIFO state tracking, UDP receive matching, DNS name parsing bounds, event dispatch reentrancy, and TX retry semantics) along with new/updated unit tests to prevent regressions.
Changes:
- Fix FIFO “alignment-induced wrap” state tracking so non-empty queues can’t be misdetected as empty (and so capacity checks match push behavior).
- Tighten UDP receive address matching during DHCP so the
local_ip==0relaxation only applies to the DHCP socket, and harden DNS PTR parsing to ensure names don’t read past RDATA (while still allowing compression pointers). - Make callback dispatch resilient to reentrancy (clear events before invoking callbacks) and ensure raw/packet TX does not drop frames on link-layer
-EAGAIN/errors; add regression tests covering these behaviors.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/wolfip.c | Core fixes in FIFO push/capacity logic, UDP DHCP scoping, DNS PTR/RDATA bounds, callback event clearing order, and raw/packet TX retry behavior. |
| src/test/unit/unit.c | Registers new regression tests covering the added/changed behaviors. |
| src/test/unit/unit_tests_proto.c | Adds regression tests for DHCP scoping and raw/packet TX retry-on-EAGAIN behavior. |
| src/test/unit/unit_tests_poll_dispatcher.c | Adds regression tests ensuring events raised during reentrant callbacks are not lost. |
| src/test/unit/unit_tests_multicast.c | Strengthens IGMP spoofing-drop test to assert no deferred report timer is armed and no later send occurs. |
| src/test/unit/unit_tests_fifo.c | Adds regression tests for FIFO alignment wrap handling and non-empty-state preservation. |
| src/test/unit/unit_tests_dns_edges.c | Updates DNS name-copy tests for the new dns_copy_name(..., rdata_end) signature and adds PTR/RDATA edge-case tests. |
| src/test/unit/unit_tests_dns_dhcp.c | Updates DHCP-running UDP receive test to set dhcp_udp_sd so the scoped relaxation applies as intended. |
| src/test/unit/unit_tests_api.c | Updates internal DNS copy-name unit test calls for the new signature. |
| src/http/httpd.h | Clarifies http_request.query comment to reflect “query string if present in the target”. |
💡 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.
Temp bisect branch: F-6209, F-10259, F-10260, F-9366, F-6475, F-8521, F-10280 on master. macOS bisect for the test-wolfssl hang. Will be deleted.