Fix use-after-free race in socket driver close

https://ampcode.com/threads/T-019cb8b8-9e4c-7316-9566-c7e3f5f2b6db

Fix a use-after-free race condition in the generic_unix socket driver's close
handler, detected by Valgrind during CI gen_tcp tests.

The close handler in socket_consume_mailbox used a two-phase locking pattern:
it acquired the glb->listeners lock to NULL-out the socket_data listener
pointers, released it, then called sys_unregister_listener (which re-acquires
the lock) to remove the listener from the linked list. Between the unlock and
re-lock, the event loop thread could also unlink the same listener node via
process_listener_handler after the callback returned NULL. The subsequent
list_remove in sys_unregister_listener then operated on stale prev/next
pointers, corrupting the list or writing to freed memory.

The fix makes the pointer detach and list unlink atomic under a single lock hold
by introducing sys_unregister_listener_nolock — a variant that assumes the
caller already holds the glb->listeners write lock. The close handler now
NULLs the pointers, unlinks the listeners, and releases the lock before freeing
the memory.

This pattern is specific to generic_unix; ESP32 and RP2 use a single global
listener for the socket driver subsystem and are not affected.

Signed-off-by: Peter M <petermm@gmail.com>

Author: petermm

src/platforms/generic_unix/lib/socket_driver.c

@@ -47,6 +47,8 @@
 #include <sys/types.h>
 #include <unistd.h>
 
+void sys_unregister_listener_nolock(GlobalContext *global, struct EventListener *listener);
+
 // #define ENABLE_TRACE
 #include "trace.h"
 
@@ -1194,31 +1196,26 @@ static NativeHandlerResult socket_consume_mailbox(Context *ctx)
         TRACE("close\n");
         port_send_reply(ctx, pid, ref, OK_ATOM);
         SocketDriverData *socket_data = (SocketDriverData *) ctx->platform_data;
-        // Callbacks (active_recv_callback, passive_recv_callback) are called
-        // while glb->listeners lock is held. They may want to free the
-        // listener, causing a potential double free here.
-        // We acquire the lock on listeners here and set the listeners
-        // to NULL in the socket_data structure to prevent them from freeing
-        // the listeners.
+        // Callbacks (active_recv_callback, passive_recv_callback, accept_callback)
+        // are called while glb->listeners lock is held. They may free the
+        // listener and set the socket_data pointer to NULL.
+        // We must atomically detach the pointers AND unlink from the listeners
+        // list under the same lock hold, to prevent a race where the callback
+        // also unlinks the same listener node.
         synclist_wrlock(&glb->listeners);
         ActiveRecvListener *active_listener = socket_data->active_listener;
         PassiveRecvListener *passive_listener = socket_data->passive_listener;
         socket_data->active_listener = NULL;
         socket_data->passive_listener = NULL;
-        synclist_unlock(&glb->listeners);
         if (active_listener) {
-            // Then we unregister, which also acquires the lock. The callbacks
-            // may have returned NULL which means the listener would no longer
-            // be registered, but this will work.
-            sys_unregister_listener(glb, &active_listener->base);
-            // After the listener is unregistered, the callbacks can no longer
-            // be called, so we can eventually free the listener
-            free(active_listener);
+            sys_unregister_listener_nolock(glb, &active_listener->base);
         }
         if (passive_listener) {
-            sys_unregister_listener(glb, &passive_listener->base);
-            free(passive_listener);
+            sys_unregister_listener_nolock(glb, &passive_listener->base);
         }
+        synclist_unlock(&glb->listeners);
+        free(active_listener);
+        free(passive_listener);
         socket_driver_do_close(ctx);
         // We don't need to remove message.
         return NativeTerminate;

src/platforms/generic_unix/lib/sys.c

@@ -698,6 +698,12 @@ void sys_unregister_listener(GlobalContext *global, struct EventListener *listen
     synclist_remove(&global->listeners, &listener->listeners_list_head);
 }
 
+void sys_unregister_listener_nolock(GlobalContext *global, struct EventListener *listener)
+{
+    listener_event_remove_from_polling_set(listener->fd, global);
+    list_remove(&listener->listeners_list_head);
+}
+
 void sys_register_select_event(GlobalContext *global, ErlNifEvent event, bool is_write)
 {
     struct GenericUnixPlatformData *platform = global->platform_data;