diff --git a/src/platforms/generic_unix/lib/socket_driver.c b/src/platforms/generic_unix/lib/socket_driver.c index 7e9439622d..dd9228c0d2 100644 --- a/src/platforms/generic_unix/lib/socket_driver.c +++ b/src/platforms/generic_unix/lib/socket_driver.c @@ -47,6 +47,9 @@ #include #include +void sys_register_listener_nolock(GlobalContext *global, struct EventListener *listener); +void sys_unregister_listener_nolock(GlobalContext *global, struct EventListener *listener); + // #define ENABLE_TRACE #include "trace.h" @@ -83,6 +86,22 @@ typedef struct SocketDriverData PassiveRecvListener *passive_listener; } SocketDriverData; +static void register_active_listener(GlobalContext *glb, SocketDriverData *socket_data, ActiveRecvListener *listener) +{ + synclist_wrlock(&glb->listeners); + socket_data->active_listener = listener; + sys_register_listener_nolock(glb, &listener->base); + synclist_unlock(&glb->listeners); +} + +static void register_passive_listener(GlobalContext *glb, SocketDriverData *socket_data, PassiveRecvListener *listener) +{ + synclist_wrlock(&glb->listeners); + socket_data->passive_listener = listener; + sys_register_listener_nolock(glb, &listener->base); + synclist_unlock(&glb->listeners); +} + // clang-format off // TODO define in defaultatoms const char *const send_a = "\x4" "send"; @@ -253,8 +272,7 @@ static term init_udp_socket(Context *ctx, SocketDriverData *socket_data, term pa listener->base.handler = active_recvfrom_callback; listener->buf_size = socket_data->buffer; listener->process_id = ctx->process_id; - sys_register_listener(glb, &listener->base); - socket_data->active_listener = listener; + register_active_listener(glb, socket_data, listener); } } return ret; @@ -338,8 +356,7 @@ static term init_client_tcp_socket(Context *ctx, SocketDriverData *socket_data, listener->base.handler = active_recv_callback; listener->buf_size = socket_data->buffer; listener->process_id = ctx->process_id; - sys_register_listener(glb, &listener->base); - socket_data->active_listener = listener; + register_active_listener(glb, socket_data, listener); } } return ret; @@ -731,10 +748,10 @@ static EventListener *active_recv_callback(GlobalContext *glb, EventListener *ba port_send_message_nolock(glb, pid, msg); mailbox_send(ctx, globalcontext_make_atom(glb, close_internal)); // See socket_consume_mailbox close path below - if (socket_data->active_listener) { + if (socket_data->active_listener == listener) { socket_data->active_listener = NULL; - free(listener); } + free(listener); result = NULL; END_WITH_STACK_HEAP(heap, glb); } else { @@ -835,12 +852,12 @@ static EventListener *passive_recv_callback(GlobalContext *glb, EventListener *b port_send_message_nolock(glb, pid, reply); memory_destroy_heap(&heap, glb); } - globalcontext_get_process_unlock(glb, ctx); // See socket_consume_mailbox close path below - if (socket_data->passive_listener) { + if (socket_data->passive_listener == listener) { socket_data->passive_listener = NULL; - free(listener); } + globalcontext_get_process_unlock(glb, ctx); + free(listener); free(buf); // remove the EventListener from the global list return NULL; @@ -975,12 +992,12 @@ static EventListener *passive_recvfrom_callback(GlobalContext *glb, EventListene port_send_message_nolock(glb, pid, reply); memory_destroy_heap(&heap, glb); } - globalcontext_get_process_unlock(glb, ctx); // See socket_consume_mailbox close path below - if (socket_data->passive_listener) { + if (socket_data->passive_listener == listener) { socket_data->passive_listener = NULL; - free(listener); } + globalcontext_get_process_unlock(glb, ctx); + free(listener); free(buf); // remove the EventListener from the global list and clean up return NULL; @@ -1015,8 +1032,7 @@ static void do_recv(Context *ctx, term pid, term ref, term length, term timeout, listener->length = term_to_int(length); listener->buffer = socket_data->buffer; listener->ref_ticks = term_to_ref_ticks(ref); - sys_register_listener(glb, &listener->base); - socket_data->passive_listener = listener; + register_passive_listener(glb, socket_data, listener); } void socket_driver_do_recvfrom(Context *ctx, term pid, term ref, term length, term timeout) @@ -1044,6 +1060,13 @@ static EventListener *accept_callback(GlobalContext *glb, EventListener *base_li socklen_t clientlen = sizeof(clientaddr); int fd = accept(listener->base.fd, (struct sockaddr *) &clientaddr, &clientlen); Context *ctx = globalcontext_get_process_lock(glb, listener->process_id); + if (UNLIKELY(ctx == NULL)) { + if (fd != -1) { + close(fd); + } + free(listener); + return NULL; + } SocketDriverData *socket_data = (SocketDriverData *) ctx->platform_data; EventListener *result = NULL; if (fd == -1) { @@ -1058,6 +1081,21 @@ static EventListener *accept_callback(GlobalContext *glb, EventListener *base_li TRACE("socket_driver|accept_callback: accepted connection. fd: %i\n", fd); term pid = listener->pid; + if (UNLIKELY(fcntl(fd, F_SETFL, O_NONBLOCK) == -1)) { + int err = errno; + close(fd); + BEGIN_WITH_STACK_HEAP(12, heap); + term ref = term_from_ref_ticks(listener->ref_ticks, &heap); + term reply = port_heap_create_reply(&heap, ref, port_heap_create_sys_error_tuple(&heap, FCNTL_ATOM, err)); + port_send_message_nolock(glb, pid, reply); + END_WITH_STACK_HEAP(heap, glb); + if (socket_data->passive_listener == listener) { + socket_data->passive_listener = NULL; + } + globalcontext_get_process_unlock(glb, ctx); + free(listener); + return NULL; + } SocketDriverData *new_socket_data = socket_driver_create_data(); new_socket_data->sockfd = fd; new_socket_data->proto = socket_data->proto; @@ -1070,9 +1108,12 @@ static EventListener *accept_callback(GlobalContext *glb, EventListener *base_li Context *new_ctx = create_accepting_socket(glb, new_socket_data); ctx = globalcontext_get_process_lock(glb, listener->process_id); if (UNLIKELY(ctx == NULL)) { + socket_driver_do_close(new_ctx); + scheduler_terminate(new_ctx); free(listener); return NULL; } + socket_data = (SocketDriverData *) ctx->platform_data; if (new_socket_data->active) { result = &create_accepting_socket_listener(new_ctx, new_socket_data)->base; } @@ -1086,12 +1127,12 @@ static EventListener *accept_callback(GlobalContext *glb, EventListener *base_li port_send_message_nolock(glb, pid, reply); END_WITH_STACK_HEAP(heap, glb); } - globalcontext_get_process_unlock(glb, ctx); // See socket_consume_mailbox close path below - if (socket_data->passive_listener) { + if (socket_data->passive_listener == listener) { socket_data->passive_listener = NULL; - free(listener); } + globalcontext_get_process_unlock(glb, ctx); + free(listener); // remove the EventListener from the global list and replace it if needed return result; } @@ -1117,8 +1158,7 @@ void socket_driver_do_accept(Context *ctx, term pid, term ref, term timeout) listener->length = 0; listener->buffer = 0; listener->ref_ticks = term_to_ref_ticks(ref); - sys_register_listener(glb, &listener->base); - socket_data->passive_listener = listener; + register_passive_listener(glb, socket_data, listener); } static NativeHandlerResult socket_consume_mailbox(Context *ctx) @@ -1194,31 +1234,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; diff --git a/src/platforms/generic_unix/lib/sys.c b/src/platforms/generic_unix/lib/sys.c index ce6a032fc9..64ecf4f643 100644 --- a/src/platforms/generic_unix/lib/sys.c +++ b/src/platforms/generic_unix/lib/sys.c @@ -660,9 +660,8 @@ void event_listener_add_to_polling_set(struct EventListener *listener, GlobalCon #endif } -void sys_register_listener(GlobalContext *global, struct EventListener *listener) +void sys_register_listener_nolock(GlobalContext *global, struct EventListener *listener) { - struct ListHead *listeners = synclist_wrlock(&global->listeners); event_listener_add_to_polling_set(listener, global); #ifndef AVM_NO_SMP #ifndef HAVE_KQUEUE @@ -670,7 +669,13 @@ void sys_register_listener(GlobalContext *global, struct EventListener *listener sys_signal(global); #endif #endif - list_append(listeners, &listener->listeners_list_head); + list_append(synclist_nolock(&global->listeners), &listener->listeners_list_head); +} + +void sys_register_listener(GlobalContext *global, struct EventListener *listener) +{ + synclist_wrlock(&global->listeners); + sys_register_listener_nolock(global, listener); synclist_unlock(&global->listeners); } @@ -692,10 +697,17 @@ static void listener_event_remove_from_polling_set(listener_event_t listener_fd, #endif } -void sys_unregister_listener(GlobalContext *global, struct EventListener *listener) +void sys_unregister_listener_nolock(GlobalContext *global, struct EventListener *listener) { listener_event_remove_from_polling_set(listener->fd, global); - synclist_remove(&global->listeners, &listener->listeners_list_head); + list_remove(&listener->listeners_list_head); +} + +void sys_unregister_listener(GlobalContext *global, struct EventListener *listener) +{ + synclist_wrlock(&global->listeners); + sys_unregister_listener_nolock(global, listener); + synclist_unlock(&global->listeners); } void sys_register_select_event(GlobalContext *global, ErlNifEvent event, bool is_write)