tcp: ignore segments for closed sockets, plus timer-exhaustion recovery in tcp/dhcp/igmp - #172
Conversation
…he poll loop when the timer heap was full
…od carrying a body, not just POST
There was a problem hiding this comment.
🟡 Changes recommended
Three moderate issues remain in TCP cleanup and IGMP timer recovery.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Improves TCP closed-socket handling, preserves explicitly bound low ports, and adds timer-heap recovery for TCP, DHCP, and IGMP.
Changes:
- Adds TCP, DHCP, and IGMP timer-exhaustion recovery.
- Adds regression tests and updates low-port expectations.
- Clarifies WPA and HTTP documentation.
File summaries
| File | Description |
|---|---|
src/wolfip.c |
Core TCP, DHCP, and IGMP changes. Three moderate findings remain: failed SYN cleanup (3 votes), deadline rebasing (3 votes), and chronological IGMP timer selection (1 vote). |
src/test/unit/unit.c |
Registers new tests. |
src/test/unit/unit_tests_tcp_state.c |
Tests TCP state handling and timer exhaustion. |
src/test/unit/unit_tests_multicast.c |
Tests IGMP timer recovery. |
src/test/unit/unit_tests_dns_dhcp.c |
Updates low-port behavior expectations. |
src/test/unit/unit_tests_dhcp_edges.c |
Tests DHCP timer recovery. |
src/test/unit/unit_tests_api.c |
Tests bound low-port preservation. |
src/port/rp2350_cyw43439/cyw43439_wifi.c |
Corrects WPA documentation. |
src/http/httpd.h |
Clarifies HTTP body documentation. |
Review details
Suppressed comments (1)
src/wolfip.c:4958
- If both a deferred query response and an unsolicited join repeat are pending while only one timer slot becomes available, this always arms the query response first, even when
unsol_atis earlier. A query can have a response window of several seconds while the unsolicited repeat is due within 1 second, so the newly armed later timer can occupy the only slot and delay the repeat; select pending deadlines in chronological order.
if (m->report_at != 0 && m->tmr_report == NO_TIMER)
m->tmr_report = igmp_arm_report(s, m, m->report_at,
igmp_report_timer_cb);
if (m->unsol_at != 0 && m->tmr_unsol == NO_TIMER)
m->tmr_unsol = igmp_arm_report(s, m, m->unsol_at,
- Files reviewed: 9/9 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
🔵 Needs a closer look
Five moderate findings remain in src/wolfip.c, affecting TCP cleanup/state restoration and DHCP retry deadlines.
Review details
Suppressed comments (5)
src/wolfip.c:7061
- Because
newts->callbackwas copied from the listener above, thisclose_socket()call defersCB_EVENT_CLOSEDinstead of releasing the failed child immediately.wolfIP_sock_accept()returns an error and never gives this descriptor to the caller, so the next poll can invoke the listener callback for a phantom child socket. Clear the callback pair before disposing a child whose control-RTO could not be armed (or use a non-notifying cleanup path).
close_socket(newts);
src/wolfip.c:7062
- When this arm fails, the cloned socket is freed but
tsremains inTCP_SYN_RCVD; line 7054 has already cleared its readable event, and its control-RTO was stopped before cloning. The listener therefore retains the peer tuple and port with no timer or accept notification and cannot accept another connection. ReverttstoLISTENbefore returning, as the other timer-arm failure paths do.
if (tcp_ctrl_rto_start(newts, s->last_tick) < 0) {
close_socket(newts);
return -WOLFIP_EAGAIN;
src/wolfip.c:9158
- When
dhcp_send_discover()cannot queue a packet, it requests a one-tick retry (retry_at). If the heap is full for that arm, this recovery branch discards that deadline and creates a normal discover backoff, so a slot freeing immediately can still delay acquisition by several seconds. Preserve the failed arm's requested deadline (or an explicit immediate-retry state) and use it here.
dhcp_schedule_timer_at(s, s->last_tick +
dhcp_discover_retry_delay(s, DHCP_DISCOVER_TIMEOUT));
src/wolfip.c:9162
dhcp_send_request()likewise requestslast_tick + 1when the datagram cannot be queued, but this recovery branch regenerates the normal backoff instead. Under timer-heap exhaustion, the client therefore loses the intended immediate retry and may wait several seconds after the heap slot becomes available; retain and re-arm the original retry deadline.
case DHCP_REQUEST_SENT:
dhcp_schedule_retry_timer(s, 0);
break;
src/wolfip.c:9175
- When a DAD probe cannot be sent,
dhcp_timer_cb()schedules the next attempt forlast_tick + 1, but this recovery path substitutes the fullDHCP_DAD_INTERVAL_MS. A slot becoming available immediately can therefore add an unnecessary one-second delay; preserve the callback's requested retry deadline.
case DHCP_DAD:
dhcp_schedule_timer_at(s, s->last_tick + DHCP_DAD_INTERVAL_MS);
- Files reviewed: 9/9 changed files
- Comments generated: 0 new
- Review effort level: Lite
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #172
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.
…t cannot arm the control rto
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #172
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.
Fenrir's latest completed scan found no issues; clearing the prior automated change request.
Also some minor edits to some docs for WPA and HTTP.
c35eb99 - tcp: ignore inbound segments for any socket in tcp_closed
13780e0 - http: correct the body field comment, which is populated for any method carrying a body, not just POST
7916753 - igmp: re-arm deferred query responses and ojint repeat reports from the poll loop when the timer heap was full
a9d7edc - dhcp: re-arm the client timer from the poll loop when the timer heap was full
fa2378d - tcp: handle timer-heap exhaustion when arming control-rto and teardown timeouts
c341f3c - socket: honour an explicitly bound low source port across tcp connect
46baec7 - minor fix to the comment in the WPA2 wrapper
60e0925 - tcp: cover the control-rto give-up path take when a re-arm finds the timer heap full
8b0197f - igmp: rebase the deferred query response and join repeat deadlines when the tick source rolls back
4628f84 - tcp: drop the queued syn when connect cannot arm the control rto
347a709 - tcp: fully reset a listener reverted to listen by a rst in syn-rcvd