From f3c30f8b70f58734524097c4f1ae4b70294f7056 Mon Sep 17 00:00:00 2001 From: Tinic Uro Date: Sat, 26 Sep 2026 01:37:21 -0700 Subject: [PATCH] bsdsocket: drain parked closes past transient holds (#53) bsd_close_all() drained only at sb_StackRefs <= 1, so an async AddressAllocation's transient hold kept the last opener from draining, and the worker's release tore the stack down with closing sockets still created. The gate now discounts transient refs, shared with the handoff flush as bsd_stack_last_opener(). ami_ns_destroy() discarded nx_ip_delete()'s NX_SOCKETS_BOUND and freed the stack block under a live IP thread; it now retains it. Tests: expunge_refusal(_cork) t_transient_last_opener_drains, expunge_joint t_bound_socket_keeps_ip. Co-Authored-By: Claude Opus 5.5 --- src/bsdsocket/bsdsocket_internal.h | 9 ++++ src/bsdsocket/library.c | 3 +- src/bsdsocket/socket.c | 2 +- src/netstack/netstack.c | 14 +++++- tests/bsdsocket/host/test_expunge_host.c | 44 +++++++++++++++++ tests/netstack/host/netstack_host_env.c | 12 ++++- tests/netstack/host/netstack_host_env.h | 4 ++ tests/netstack/host/test_expunge_joint_host.c | 48 +++++++++++++++++++ 8 files changed, 130 insertions(+), 6 deletions(-) diff --git a/src/bsdsocket/bsdsocket_internal.h b/src/bsdsocket/bsdsocket_internal.h index 4233e8819..fcdc6f017 100644 --- a/src/bsdsocket/bsdsocket_internal.h +++ b/src/bsdsocket/bsdsocket_internal.h @@ -417,6 +417,15 @@ static inline NX_PACKET_POOL *bsd_stack_pool(const struct AmiSocketBase *base) return master->sb_StackPool; } +/* The closing opener is the last one that is not an async worker's transient + hold: parked closing sockets are drained now, since nothing else will + before the worker's release tears the stack down (#53). */ +static inline BOOL bsd_stack_last_opener(const struct AmiSocketBase *master) +{ + return master->sb_StackRefs >= master->sb_TransientStackRefs && + master->sb_StackRefs - master->sb_TransientStackRefs <= 1; +} + #define ASF_TCP (1UL << 0) #define ASF_UDP (1UL << 1) #define ASF_NONBLOCK (1UL << 2) diff --git a/src/bsdsocket/library.c b/src/bsdsocket/library.c index 2b018bc1d..338c0c4ea 100644 --- a/src/bsdsocket/library.c +++ b/src/bsdsocket/library.c @@ -1059,8 +1059,7 @@ APTR bsd_lib_close(register struct AmiSocketBase *SocketBase __asm("a6")) bracketed = (bsd_nx_enter(base) == 0); ObtainSemaphore(&master->sb_Lock); - if (master->sb_StackRefs >= master->sb_TransientStackRefs && - master->sb_StackRefs - master->sb_TransientStackRefs <= 1) + if (bsd_stack_last_opener(master)) bsd_handoff_flush(base, bracketed); ReleaseSemaphore(&master->sb_Lock); diff --git a/src/bsdsocket/socket.c b/src/bsdsocket/socket.c index 4087943e1..6e0852071 100644 --- a/src/bsdsocket/socket.c +++ b/src/bsdsocket/socket.c @@ -971,7 +971,7 @@ VOID bsd_close_all(struct AmiSocketBase *base) bsd_closing_sweep(); - if (base->sb_Master != NULL && base->sb_Master->sb_StackRefs <= 1) + if (base->sb_Master != NULL && bsd_stack_last_opener(base->sb_Master)) bsd_closing_drain(); bsd_nx_leave(base); diff --git a/src/netstack/netstack.c b/src/netstack/netstack.c index 73da5d5a7..4d4220e5f 100644 --- a/src/netstack/netstack.c +++ b/src/netstack/netstack.c @@ -218,9 +218,21 @@ static VOID ami_ns_destroy(AmiNetStack *ns) ami_netstack_dhcpv6_destroy(ns); #endif + /* + * A socket still created refuses the delete and leaves the IP thread and + * its timers running on ns_Ip, so nothing they can reach is closed or + * freed: a bounded leak, as below (#53). + */ if (ns->ns_IpCreated) { - AMI_NX_CLEANUP(nx_ip_delete(&ns->ns_Ip)); + UINT status = nx_ip_delete(&ns->ns_Ip); + + if (status != NX_SUCCESS) + { + AMI_ERROR("netstack: IP instance not deleted (0x%lx); retaining " + "stack memory", (unsigned long)status); + return; + } ns->ns_IpCreated = FALSE; } diff --git a/tests/bsdsocket/host/test_expunge_host.c b/tests/bsdsocket/host/test_expunge_host.c index 86e4b6fc2..500a5d24f 100644 --- a/tests/bsdsocket/host/test_expunge_host.c +++ b/tests/bsdsocket/host/test_expunge_host.c @@ -661,6 +661,49 @@ static VOID t_transient_stack_reference(VOID) h_report("transient", 0, 0, 0, h_teardown_ran()); } +/* + * Issue #53: tool T launches an async DHCP job (a transient hold) and closes; + * app A parks a closing socket and closes; T's job then releases the last + * reference. A's bsd_close_all() is the last chance to drain the parked + * socket before netstack_shutdown(), so its gate must say so. + */ +static VOID t_transient_last_opener_drains(VOID) +{ + BOOL t_drains; + BOOL a_drains; + + printf("the last opener's close with a transient worker outstanding\n"); + + h_machine_reset(TRUE); + h.stack_running = TRUE; + h_base->sb_StackRefs = 1; /* T opens */ + + CHECK(bsd_stack_transient_hold(h_base) == 0, "T's DHCP job holds the stack"); + h_base->sb_StackRefs++; /* A opens */ + + t_drains = bsd_stack_last_opener(h_base); /* T's close_all gate */ + h_base->sb_StackRefs--; /* T closes */ + + a_drains = bsd_stack_last_opener(h_base); /* A's close_all gate */ + h_base->sb_StackRefs--; /* A closes */ + + CHECK(h.shutdown_calls == 0, "the worker still holds the stack"); + bsd_stack_transient_release(h_base); /* the job completes */ + + printf("transient_last_opener t_drains=%ld a_drains=%ld shutdowns=%ld\n", + (long)t_drains, (long)a_drains, (long)h.shutdown_calls); + CHECK(!t_drains, "T's close leaves A's sockets alone"); + CHECK(a_drains, "A's close drains the parked sockets"); + CHECK(h.shutdown_calls == 1, "the worker's release tears the stack down"); + + /* A hold is an opener, not a worker: the network stays up. */ + h_machine_reset(TRUE); + h_base->sb_StackRefs = 2; + CHECK(!bsd_stack_last_opener(h_base), "a held stack is not drained"); + h_base->sb_StackRefs = 1; + CHECK(bsd_stack_last_opener(h_base), "the last plain opener drains"); +} + #ifdef AMINETXDUO_TCP_CORK /* One open/close cycle of the stack: a bring-up takes a netstack reference (bsd_lib_open(), netstack.c:1864), and the last library reference going @@ -802,6 +845,7 @@ int main(void) t_other_refusals(); t_last_close_retries(); t_transient_stack_reference(); + t_transient_last_opener_drains(); t_loopback_startup_failure_ownership(); #ifdef AMINETXDUO_TCP_CORK t_cork_pass_keeps_stack(); diff --git a/tests/netstack/host/netstack_host_env.c b/tests/netstack/host/netstack_host_env.c index e28ade562..e9a43c3a0 100644 --- a/tests/netstack/host/netstack_host_env.c +++ b/tests/netstack/host/netstack_host_env.c @@ -105,6 +105,8 @@ VOID FreeMem(APTR memoryBlock, ULONG byteSize) if (memoryBlock != NULL) { + if (memoryBlock == nsh.watch_block) + nsh.watch_freed = TRUE; nsh.frees++; free(memoryBlock); } @@ -1298,11 +1300,17 @@ UINT _nxe_ip_address_get(NX_IP *ip_ptr, ULONG *ip_address, ULONG *network_mask) return TX_SUCCESS; } +/* nx_ip_delete.c's refusal: a created socket leaves the IP thread and its + timers running and answers NX_SOCKETS_BOUND. */ UINT _nxe_ip_delete(NX_IP *ip_ptr) { - (VOID)ip_ptr; + nsh.ip_deletes++; + nsh.ip_delete_status = NX_SUCCESS; + if (ip_ptr->nx_ip_udp_created_sockets_count != 0 || + ip_ptr->nx_ip_tcp_created_sockets_count != 0) + nsh.ip_delete_status = NX_SOCKETS_BOUND; - return TX_SUCCESS; + return nsh.ip_delete_status; } UINT _nxe_ip_driver_interface_direct_command(NX_IP *ip_ptr, UINT command, UINT interface_index, ULONG *return_value_ptr) diff --git a/tests/netstack/host/netstack_host_env.h b/tests/netstack/host/netstack_host_env.h index 029bfd4a4..d1a7fee4c 100644 --- a/tests/netstack/host/netstack_host_env.h +++ b/tests/netstack/host/netstack_host_env.h @@ -128,6 +128,8 @@ typedef struct NetStackHostEnv /* ---- NetX Duo ------------------------------------------------------ */ UINT ip_create_status; + ULONG ip_deletes; + UINT ip_delete_status; /* what the last nx_ip_delete() answered */ ULONG iface_attaches; ULONG iface_detaches; ULONG iface_address; /* what nx_ip_interface_address_get() has */ @@ -145,6 +147,8 @@ typedef struct NetStackHostEnv ULONG forbids; /* Forbid() minus Permit(), must end at 0 */ LONG forbid_depth; BOOL attempt_semaphore_fails; /* the contended-lock arm of can_unload */ + APTR watch_block; /* FreeMem() of this block is recorded */ + BOOL watch_freed; } NetStackHostEnv; extern NetStackHostEnv nsh; diff --git a/tests/netstack/host/test_expunge_joint_host.c b/tests/netstack/host/test_expunge_joint_host.c index 797cc99b6..851571933 100644 --- a/tests/netstack/host/test_expunge_joint_host.c +++ b/tests/netstack/host/test_expunge_joint_host.c @@ -261,6 +261,53 @@ static void t_refcount(void) CHECK(netstack_can_unload() == TRUE, "and the expunge is allowed"); } +/* + * Issue #53, link 3. A TCP socket still created on the IP instance at + * teardown: nx_ip_delete() answers NX_SOCKETS_BOUND and leaves the IP thread + * and its timers running on ns_Ip. The stack block holding them must not be + * freed under that thread. + */ +static void t_bound_socket_keeps_ip(void) +{ + AmiNetStack *ns; + NX_IP *ip; + + printf("expunge joint: a socket still bound at teardown\n"); + + nsh_reset(); + + CHECK(netstack_startup() == AMI_NET_OK, "up"); + ns = netstack_get(); + ip = netstack_ip(); + CHECK(ns != NULL && ip != NULL, "a stack and its IP instance"); + if (ns == NULL || ip == NULL) + return; + + ip->nx_ip_tcp_created_sockets_count = 1; /* a parked closing socket */ + nsh.watch_block = ns; + + netstack_shutdown(); + + printf("teardown ip_delete_status=%lu ns_freed=%ld\n", + (unsigned long)nsh.ip_delete_status, (long)nsh.watch_freed); + CHECK(nsh.ip_deletes == 1, "nx_ip_delete() was called"); + CHECK(nsh.ip_delete_status == NX_SOCKETS_BOUND, + "and refused: the IP thread is still running"); + CHECK(!nsh.watch_freed, + "the stack block holding that IP thread is not freed"); + CHECK(netstack_get() == NULL, "the singleton is gone all the same"); + + h_teardown(); + + /* A clean teardown still frees it. */ + nsh_reset(); + CHECK(netstack_startup() == AMI_NET_OK, "up again"); + nsh.watch_block = netstack_get(); + netstack_shutdown(); + CHECK(nsh.ip_delete_status == NX_SUCCESS, "no socket: the delete succeeds"); + CHECK(nsh.watch_freed, "and the stack block is freed"); +} + int main(void) { printf("netstack expunge joint host checks\n\n"); @@ -273,6 +320,7 @@ int main(void) t_startup_refuses_over_a_failed_stop(); t_contended_lock_refuses(); t_refcount(); + t_bound_socket_keeps_ip(); printf("\n%lu checks, %lu failures\n", h_checks, h_failures);