Fix socket lifecycle in the core: accept(), close() and descriptor reuse - #184
Merged
Merged
Conversation
Member
Author
|
@wolfSSL-Fenrir-bot review balanced |
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Descriptor reuse can break POSIX callback mapping, and established accepts still violate the documented full-table behavior.
Review effort: Balanced
Findings: 1
Open (2)
What changed in this PR
Fixes socket lifecycle handling across the core, FreeRTOS wrapper, examples, and tests.
Changes:
- Adds generation-tagged descriptors, stale-descriptor rejection, and TCP slot reclamation.
- Revises TCP
accept()/close()semantics and FreeRTOS blocking timeouts. - Expands lifecycle, timeout, callback, and polling regression coverage.
| File | Description |
|---|---|
wolfip.h |
Adds timeout options, errors, and timeval type. |
src/wolfip.c |
Implements generations, parked accepts, reclamation, and close changes. |
src/test/unit/unit.c |
Registers new unit tests. |
src/test/unit/unit_tests_tcp_state.c |
Updates handshake and close-state tests. |
src/test/unit/unit_tests_tcp_flow.c |
Adds lifecycle and parked-connection tests. |
src/test/unit/unit_tests_socket_api_arms.c |
Tests descriptor generations and reuse. |
src/test/unit/unit_tests_proto.c |
Updates protocol-level accept and close tests. |
src/test/unit/unit_tests_poll_dispatcher.c |
Tests generated callbacks and rebased deadlines. |
src/test/unit/unit_tests_dns_dhcp.c |
Updates accept and close expectations. |
src/test/unit/unit_tests_api.c |
Updates socket API tests. |
src/test/unit/unit_shared.c |
Adds shared parked-handshake helpers. |
src/test/test_wolfssl_forwarding.c |
Preserves the client sentinel on failed accept. |
src/test/test_native_wolfssl.c |
Preserves the client sentinel on failed accept. |
src/test/test_freertos_close_last_ack.c |
Updates FreeRTOS close regression behavior. |
src/test/test_freertos_bsd_semantics.c |
Adds FreeRTOS blocking-semantics coverage. |
src/test/test_eventloop.c |
Handles negative accept results. |
src/test/test_eventloop_tun.c |
Handles negative accept results. |
src/test/ipfilter_logger.c |
Handles negative accept results. |
src/test/freertos_mocks/wolfip.h |
Extends FreeRTOS socket mocks. |
src/test/freertos_mocks/FreeRTOS.h |
Supports variable tick rates. |
src/test/esp/test_esp.c |
Handles negative accept results. |
src/port/va416xx/main.c |
Handles negative accept results. |
src/port/stm32n6/main.c |
Handles negative accept results. |
src/port/stm32h753/main.c |
Handles negative accept results. |
src/port/stm32h563/main.c |
Handles negative accept results. |
src/port/stm32f439/main.c |
Handles negative accept results. |
src/port/stm32c5a3/main.c |
Handles negative accept results. |
src/port/raspberry-pico-usb-server/src/main.c |
Handles negative accept results. |
src/port/pic32mz/main.c |
Handles negative accept results. |
src/port/lpc54s018/main.c |
Handles negative accept results. |
src/port/freeRTOS/README.md |
Documents timeout and close semantics. |
src/port/freeRTOS/bsd_socket.c |
Implements bounded blocking and safe close handling. |
Makefile |
Adds FreeRTOS semantics test targets. |
docs/API.md |
Documents lifecycle and descriptor changes. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #184
Scan targets checked: wolfip-src, wolfip-bugs
Coverage: 3 of 11 in-scope changed file(s) opened by the reviewer; not opened: src/port/lpc54s018/main.c, src/port/pic32mz/main.c, src/port/stm32c5a3/main.c, src/port/stm32f439/main.c, src/port/stm32h563/main.c, src/port/stm32h753/main.c, src/port/stm32n6/main.c, src/port/va416xx/main.c
Fenrir result: Approved ✅
No new issues found in the changed files.
Advisory only — this automated result does not count as a GitHub approval.
Review tier: Balanced
Frauschi
force-pushed
the
freertos-bsd-semantics
branch
from
October 1, 2026 14:22
ddd1adb to
5599ac9
Compare
danielinux
previously approved these changes
Oct 2, 2026
Frauschi
force-pushed
the
freertos-bsd-semantics
branch
from
October 2, 2026 10:58
5599ac9 to
4acd7d0
Compare
The zynq7000 OCM layout has .bss end just below 0xFFFF0000, followed by the 16 KB-aligned MMU page tables. Once .bss crosses that boundary the page tables and the DMA buffers move up a full 16 KB and the image no longer fits in the 256 KB OCM. Master had 1320 bytes left before the boundary, so any core change larger than that broke the zynq7000 default build with "region 'OCM' overflowed by 4352 bytes". The default application opens two UDP sockets on every AMD board, one for DHCP and one for the echo service, so three still leaves a spare. Dropping the fourth socket frees its receive and transmit buffers and leaves about 18 KB before the zynq7000 boundary. The profile is shared by all three boards, so zcu102 and versal get the same socket count. The SPEED_TEST profile already uses two.
A wolfIP descriptor is a slot index with a type mark, so once the stack frees a socket the next wolfIP_sock_socket() or wolfIP_sock_accept() returns the very same number. Anything still holding the old one then acts on the new socket: a second close() closes it, a late abort() resets someone else's connection. The stack frees TCP sockets on its own (final ACK, peer RST, retransmission give-up), so a caller cannot avoid this by being careful, and every blocking wrapper had to build its own bookkeeping to detect it. API.md documented the hazard instead of removing it. Each slot now carries a 15-bit generation, kept in bits 16-30 of its descriptors, so a descriptor only matches again after 32768 reuses of its slot. It changes when the application closes the socket, and when the stack hands out a slot it freed while the application still held the descriptor; for TCP that happens in tcp_new_socket(), so a slot the stack takes for a socket of its own retires the old descriptor too. Every call that takes a descriptor checks it and returns -WOLFIP_EBADF for a stale one, and callbacks pass the current one. A slot's first generation is 0, so descriptors keep their old values until a slot is reused, and so do the literal descriptors in the tests. SOCKET_UNMARK() and the IS_SOCKET_*() marks are unchanged. Closing a descriptor whose slot the stack freed but has not reused still returns 0, as before. To keep that the common case, tcp_new_socket() takes such a slot only when no other one is free, so a peer reset is reported as a closed connection rather than -WOLFIP_EBADF unless the table is under pressure. Because such a descriptor still passes the check, a callback registered through it lands in the free slot, so tcp_new_socket() now clears the callback and pending events of every slot it claims. The POSIX layer mapped each core slot to its wrapper entry by slot index alone, so once a held slot was reused, closing the old descriptor cleared the new socket's mapping and its callbacks were lost. Its lookup now matches the whole descriptor, generation included, and a detach only clears a mapping that is still its own.
wolfIP_sock_close() returned -WOLFIP_EAGAIN in two different cases: when it had queued the FIN and the stack was finishing the close on its own, and when the TX FIFO had no room for the FIN, so nothing had happened and the caller had to retry. A caller cannot tell them apart, and retrying is dangerous in the first case: in LAST_ACK a second close() fell through to the teardown branch and released the socket at once, dropping the FIN retransmission before the peer's final ACK. The POSIX layer passed the -WOLFIP_EAGAIN on, so its close() returned -1 with EAGAIN for every normal TCP close. close() now returns 0 once the FIN is queued, and -WOLFIP_EAGAIN only when it could not be queued. On a socket that is already closing (FIN_WAIT_1, FIN_WAIT_2, CLOSING, LAST_ACK, TIME_WAIT) a repeated close() returns 0 and changes nothing; RFC 9293 3.10.4 leaves those connections alone too, and a repeated close() in TIME_WAIT used to cut it short. The descriptor stays valid until the stack releases the socket, so wolfIP_sock_abort() still works on it as API.md documents; after that the generation check from the previous commit answers -WOLFIP_EBADF once the slot is reused.
A socket the application has closed keeps its slot until the FIN exchange finishes: up to TCP_FIN_WAIT_2_TIMEOUT_MS (60 s) in FIN_WAIT_2, and the full control retransmission budget in FIN_WAIT_1 or LAST_ACK when the peer has gone. On a small socket table that is an outage. With MAX_TCPSOCKETS at 2 and one listener, a single TLS client that aborts its handshake leaves an orphan behind, and every following connection is refused with an RST until it times out. When tcp_new_socket() finds no free slot it now resets one the application no longer holds, preferring TIME_WAIT, which has nothing left to deliver, then FIN_WAIT_2, whose peer has acknowledged all our data, then the remaining closing states. Sockets the application still owns are never taken. Linux does the same under orphan pressure. The reset is the abortive close wolfIP_sock_abort() already performs, moved into tcp_abort() so both paths share it.
The example servers store wolfIP_sock_accept()'s result straight into
their client descriptor and only call it again while that descriptor is
-1:
if (... && (client_fd == -1)) {
client_fd = wolfIP_sock_accept(s, listen_fd, NULL, NULL);
if (client_fd > 0) ...
Any other negative result, -WOLFIP_EAGAIN included, leaves the server
waiting for a connection it can never accept again. accept() is about to
return -WOLFIP_EAGAIN after CB_EVENT_READABLE while a handshake is still
in progress, which POSIX allows anyway, so put the -1 back on any
negative result. rtl8735b already did this.
wolfIP_sock_accept() on a listener in SYN_RCVD cloned the half-open connection and returned it at once, before the peer's final ACK. The application got a socket that was not connected: send() returned -WOLFIP_EAGAIN, recv() had nothing, and a TLS server that went straight for the ClientHello failed a connection that was merely young. Every other stack (Linux, the BSDs, Winsock, lwIP, FreeRTOS+TCP, Zephyr) hands out only completed connections, and the FreeRTOS wrapper had grown a parking scheme to fake it; the POSIX layer had the same bug. accept() in SYN_RCVD now clones the connection as before, so the listener is free for the next SYN at once, but keeps the child back and returns -WOLFIP_EAGAIN. The child has no callback and remembers its listener by slot and generation. When its handshake completes and its pre-accept timer is armed, the listener raises CB_EVENT_READABLE again, wolfIP_sock_can_read() reports it, and the next accept() returns it with the listener's callback, ESTABLISHED or CLOSE_WAIT. The child records the listener's full 15-bit slot generation. CB_EVENT_READABLE is an edge, so when accept() hands out a child while another is ready, or while the listener holds a handshake of its own, it raises the event again and wakes the poller; otherwise two connections completing in one poll would reach a callback-driven application as one. A held-back child that is reset, or whose SYN-ACK goes unanswered TCP_SYNACK_MAXRTX times (default 3, about 15 s instead of the 8 retries and 3 minutes a connection gets), is freed without an event, since nobody holds its descriptor. One that completes but is not accepted within TCP_PREACCEPT_TIMEOUT_MS is reset and freed, as the listener's own pre-accept timeout does for a connection it completed itself; otherwise an application that misses the second CB_EVENT_READABLE would leave it holding a slot for good. Closing or aborting the listener resets the children still held. When the table is full they are reclaimed after FIN_WAIT_2 orphans and before the other closing states, and with no slot at all the listener keeps the handshake itself and accept() returns -WOLFIP_EAGAIN instead of -1. A connection that completes before the application calls accept() still reaches it through the listener's own ESTABLISHED path, unchanged. If that path finds no slot for the child, accept() now returns -WOLFIP_EAGAIN and leaves the connection on the listener, as the SYN_RCVD path does, instead of resetting it and returning -1; the listener's pre-accept timeout still resets it if no slot frees up in time. The tests that called accept() during the handshake and inspected the returned socket now find the held-back child instead, or complete the handshake first.
The BSD wrapper waited portMAX_DELAY on every blocking operation, so a peer that connected and then said nothing blocked a single-threaded server for good. There was no way to bound it: neither the stack nor the wrapper had any notion of a timeout. Each descriptor now carries a receive and a send timeout, both portMAX_DELAY by default so existing callers are unaffected. setsockopt() handles the two options in the wrapper rather than forwarding them to wolfIP, because blocking belongs to the wrapper; the stack never blocks. An expired wait already reported -1 with EAGAIN, which is the POSIX behaviour for these options, so the call sites pass on what is left of the timeout, counted from the start of the call: a wake that does not let the call complete must not restart it. struct wolfIP_timeval carries the argument, since wolfIP must not depend on sys/time.h. The values behave as on Linux and in the POSIX layer: all-zero, or a timeout too long for TickType_t, waits without bound, a negative tv_sec does not wait, a tv_usec outside [0, 999999] fails with WOLFIP_EDOM, and a non-zero value shorter than a tick rounds up to one tick rather than becoming "no timeout". A connect() that runs out of time fails with WOLFIP_EINPROGRESS, new in wolfip.h, while the handshake goes on. The conversion to ticks goes through configTICK_RATE_HZ, as pdMS_TO_TICKS() does, not portTICK_PERIOD_MS, which is 0 above 1000 Hz. optlen must be exactly sizeof(struct wolfIP_timeval): a platform struct timeval can be larger (64-bit time_t on a 32-bit target) and would be misread, not rejected. getsockopt() reads both options back. Both option calls recheck the descriptor under the lock, and the blocking calls read the timeout while they still hold it. Adds test-freertos-bsd-semantics, a scripted-core harness for the wrapper's blocking behaviour, starting with the timeouts, and test-freertos-bsd-semantics-2khz, the same harness at 2000 Hz. The FreeRTOS mocks gain the option names and struct it needs, and derive their tick macros from configTICK_RATE_HZ.
With the core changes before this one, the wrapper no longer has to second-guess what the stack returns. accept() gets only established connections from wolfIP_sock_accept(), so it waits on -WOLFIP_EAGAIN until the listener becomes readable or SO_RCVTIMEO runs out. Each pass first checks that its descriptor still refers to the listener it started with: another task may have closed it while this one waited, and the wrapper slot and the core slot can both be reused. The one-shot retry on a bare -1 from accept(), send() and sendto(), which covered a socket handed out before its handshake had finished, goes away with that case. close() returns as soon as wolfIP_sock_close() has queued the FIN, and the stack finishes the exchange on its own, reclaiming the slot under pressure. It waits only while the transmit buffer has no room for the FIN, woken by CB_EVENT_WRITABLE, for at most WOLFIP_BSD_CLOSE_LINGER_MS (10 s), and then resets the connection with wolfIP_sock_abort(), so a peer that stops reading cannot hold the calling task. -WOLFIP_EBADF means the stack has already released the socket and is a successful close. The special case for a -1 after CB_EVENT_CLOSED is gone: the generation check makes a released descriptor answer -WOLFIP_EBADF. The harness gains tests for both calls, and test-freertos-close-last-ack now scripts the new core: -WOLFIP_EAGAIN while the FIFO is full, then -WOLFIP_EBADF once the stack has released the socket.
close() released the wrapper slot at once, and releasing it deletes the slot's ready semaphore. A task blocked on that descriptor in recv(), send(), accept() or connect() was still inside xSemaphoreTake() on it, so it woke on a deleted semaphore, and the next socket() or accept() could hand the slot, with a new semaphore, to an unrelated socket while the old caller still held a pointer into it. Closing a socket from one task while another reads it is how a server stops a stuck connection, so this is not a contrived case. Each slot now counts the tasks blocked on it, in wolfip_bsd_wait(), which every blocking call uses to drop the lock and wait. close() on a slot with waiters marks it closing and gives its semaphore instead of releasing it. A woken waiter that finds its slot closing passes the wake on to the next one, the last one releases the slot, and each returns -1 with WOLFIP_EBADF. A closing slot is not valid for new calls and is not reissued until it is released. A task that was woken normally drops out of the count before it relocks, and every call validates its descriptor before taking the lock, so in either window another task can still close the descriptor and a third can be handed the slot. Every call therefore remembers the internal descriptor it started with and takes the lock through wolfip_bsd_lock(), which fails with WOLFIP_EBADF when the slot is closing, released or holds another socket. A call preempted before it reads its descriptor at all can still land on a reissued one; as in POSIX, closing a descriptor while another task is about to use it is the application's race, and the README says so. The unlocked validity check now sets WOLFIP_EBADF as well, so a call on a closed descriptor never leaves an earlier EAGAIN behind for a retry loop to spin on. The README also states two limits of the wrapper that stay as they are: one socket_last_error() value for all tasks, and one wake-up per descriptor shared by every task blocked on it. The harness keeps deleted semaphores and counts any use of one, and tests a close() from another task under one and under two waiters, and a close and reissue that lands just before a woken recv(), a setsockopt() or a listen() takes the lock. The accept() test that reused the listener's slot while accept() waited now checks that the slot is not reused.
wolfIP_poll_by() kept the earliest deadline with a raw 64-bit compare, while tick_expired() and the wait wolfIP_poll() returns use the signed difference of the low 32 bits. The two disagree once pending timers hold 32-bit values in a 64-bit tick domain: timers_heap_rebase() truncates them after the clock steps back (a POSIX epoch-ms now), and timers armed before the first poll start from last_tick 0. Such a timer is small as a raw value, so it replaced the deadline set by wolfIP_poll_by(s, now) for pending events, loopback frames or TX backpressure, and wolfIP_poll() returned the timer's whole remaining time instead of 0. That can exceed WOLFIP_POLL_MAX_WAIT_MS, so the POSIX stack thread slept with work pending. Compare in the same wrapped domain as tick_expired(). Every deadline then orders the way the timers fire, and none can lie beyond the now + WOLFIP_POLL_MAX_WAIT_MS that each poll starts from. Reported in the review of wolfSSL#180. The new tests step a 64-bit clock back under a pending timer and check that the poll still returns at most WOLFIP_POLL_MAX_WAIT_MS, and that a rebased timer does not override a deadline at now.
Frauschi
force-pushed
the
freertos-bsd-semantics
branch
from
October 2, 2026 12:04
4acd7d0 to
6e9e40d
Compare
danielinux
self-requested a review
October 2, 2026 12:11
danielinux
approved these changes
Oct 2, 2026
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.
Summary
Fixes accept(), close() and descriptor reuse in the core, where both the FreeRTOS and POSIX layers see them, and leaves the FreeRTOS wrapper with timeouts and a plain wait loop.
socket()oraccept()returned the same number and a lateclose()orwolfIP_sock_abort()hit the new owner. Each slot now carries a 15-bit generation in bits 16-30 of its descriptors. It advances when the application closes the socket and when the stack reuses a slot whose descriptor is still held, and every call on an old descriptor returns-WOLFIP_EBADF. A slot's first generation is 0, so descriptor values are unchanged until a slot is reused, and a still-held slot is reused only when no other slot is free.close(): returns 0 once the FIN is queued;-WOLFIP_EAGAINnow means only that the TX FIFO had no room for the FIN and the caller should retry. A repeatedclose()inFIN_WAIT_1,FIN_WAIT_2,CLOSING,LAST_ACKorTIME_WAITchanges nothing (a retry inLAST_ACKused to tear the socket down before the peer's final ACK). The descriptor stays valid forwolfIP_sock_abort()until the stack releases the socket. The POSIXclose()no longer returns -1 withEAGAINon every TCP close.socket()andaccept()reset a socket the application no longer holds, preferringTIME_WAIT, thenFIN_WAIT_2, then a held-back handshake, thenFIN_WAIT_1,CLOSINGorLAST_ACK. On a small table one aborted client no longer blocks every later connection until itsFIN_WAIT_2timeout (60 s).accept(): returns only established connections (ESTABLISHED, orCLOSE_WAITwhen the peer already sent its FIN), as Linux, the BSDs, lwIP and FreeRTOS+TCP do. A handshake in progress is cloned as before, so the listener is free for the next SYN at once, but the child is held back andaccept()returns-WOLFIP_EAGAIN. When it completes, the listener reportsCB_EVENT_READABLEagain and the nextaccept()returns it with the listener's callback; the event is raised again after each handout while more are ready. Held-back children are released without an event when reset, afterTCP_SYNACK_MAXRTXunanswered SYN-ACKs (default 3, about 15 s), or when not accepted withinTCP_PREACCEPT_TIMEOUT_MS, and closing the listener resets them.accept()'s result in a client descriptor guarded by== -1, so any negative result wedged them; they now keep the -1 sentinel.SO_RCVTIMEOandSO_SNDTIMEObound every blocking call, with Linux value semantics: all-zero or too large waits without bound, a negativetv_secdoes not wait, an out-of-rangetv_usecfails withWOLFIP_EDOM, and aconnect()that runs out of time reportsWOLFIP_EINPROGRESS.accept()waits on-WOLFIP_EAGAIN.close()returns once the FIN is queued and waits only for room for it, at mostWOLFIP_BSD_CLOSE_LINGER_MS, then aborts with an RST. Closing a descriptor other tasks are blocked on no longer deletes the semaphore under them: they return -1 withWOLFIP_EBADF, the last one releases the slot, and every call rechecks under the lock that its descriptor was not closed or reissued.wolfIP_poll_by(): compares deadlines in the wrapped 32-bit tick domain, liketick_expired(). After a 64-bit clock stepped back, a rebased timer could override the deadline for pending work andwolfIP_poll()returned more thanWOLFIP_POLL_MAX_WAIT_MS(fenrir's post-merge comment on FreeRTOS: wake the poll task when there is work instead of waiting for the timer #180).Behaviour and API changes
-WOLFIP_EBADF,WOLFIP_EDOM,WOLFIP_EINPROGRESS,WOLFIP_SO_RCVTIMEO,WOLFIP_SO_SNDTIMEOandstruct wolfIP_timevalinwolfip.h;TCP_SYNACK_MAXRTX(default 3);WOLFIP_BSD_CLOSE_LINGER_MS(default 10000) in the FreeRTOS wrapper.wolfIP_sock_close(): returns 0 instead of-WOLFIP_EAGAINafter queueing the FIN, so a caller that looped until 0 now stops at once; the stack still finishes the FIN exchange on its own.wolfIP_sock_accept(): returns-WOLFIP_EAGAINduring the handshake (previously the half-open socket) and when no slot is free for the child (previously -1). A caller must not treat-WOLFIP_EAGAINafterCB_EVENT_READABLEas a connection; POSIX allows the same.-WOLFIP_EBADF. Code that indexes withSOCKET_UNMARK()or tests the type withIS_SOCKET_*()is unaffected.close()no longer waits for the FIN exchange to finish.socket_last_error()is one value for all tasks, and the blocking calls on one descriptor share one wake-up, soclose()waiting for room for its FIN should not race another task blocked on the same descriptor (both documented in the README).Testing
close()inLAST_ACKandTIME_WAIT, abort after close, reclaim order on a full table, held-back connections (handout, readiness re-announcement, reset, SYN-ACK retry cap, pre-accept timeout, listener close, reused listener slot, wide listener generation) and thewolfIP_poll_by()deadline after a clock step. Tests that calledaccept()during the handshake now inspect the held-back child.test-freertos-bsd-semantics(and a 2 kHz build) covering the timeouts and their value semantics,accept(),close(), closing a descriptor under one and two blocked tasks, and a close and reissue racing a woken call;test-freertos-close-last-ackupdated for the newclose()results.Docs
docs/API.md(accept, abort, close, stale descriptors, reclaim order) andsrc/port/freeRTOS/README.md(timeouts, accept and close behaviour, wrapper limits).