bsdsocket: drain parked closes past transient holds (#53)

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)

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

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

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

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

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)

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;

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