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);