From 127b5711d97ade7453a887ad9e5ca3a004d41f84 Mon Sep 17 00:00:00 2001
From: Robin Jarry <robin@jarry.cc>
Date: Fri, 6 Dec 2024 14:25:27 +0100
Subject: [PATCH] api: fix deadlock on shutdown when clients are connected

The docstring of event_base_foreach_event() states explicitly that
modifying events in the callback function is unsafe and will cause
crashes.

event_free_finalize tries to acquire a lock that is already held when
the callback is called. Leading to a deadlock:

 (gdb) bt
 ...
 #3  ___pthread_mutex_lock (mutex=0x504000001550) at pthread_mutex_lock.c:93
 #4  0x00007f3f211b8485 in event_finalize_impl_ (flags=65536, ev=0x50c000005200, cb=0x4031ba <finalize_fd>)
 #5  0x00000000004042ed in close_connections (ev=0x50c000005200) at main/api.c:174
 ...
 #10 0x00007f3f211b263a in event_base_foreach_event (base=0x517000006d00, fn=0x40429f <close_connections>, arg=0x0)
 #11 0x0000000000404a3a in api_socket_stop () at main/api.c:253
 #12 0x00000000004072aa in main (argc=4, argv=0x7ffcc575d898) at main/main.c:210

Only use event_base_foreach_event() for iterating over the events that
we actually want to free (namely, ones that have read_cb and write_cb as
callbacks).

Only *after* returning from event_base_foreach_event(), call
event_free_finalize on all these events.

Fixes: 865332077351 ("main: close active connections on shutdown")
Signed-off-by: Robin Jarry <robin@jarry.cc>
---
 main/api.c | 24 ++++++++++++++++--------
 1 file changed, 16 insertions(+), 8 deletions(-)

diff --git a/main/api.c b/main/api.c
index a8607259..cc30f01f 100644
--- a/main/api.c
+++ b/main/api.c
@@ -8,6 +8,7 @@
 #include <gr_api.h>
 #include <gr_log.h>
 #include <gr_macro.h>
+#include <gr_vec.h>
 
 #include <event2/event.h>
 
@@ -168,13 +169,6 @@ static void read_cb(evutil_socket_t sock, short what, void * /*priv*/) {
 		event_free_finalize(0, ev, finalize_fd);
 }
 
-static int close_connections(const struct event_base *, const struct event *ev, void * /*priv*/) {
-	event_callback_fn cb = event_get_callback(ev);
-	if (cb == read_cb || cb == write_cb)
-		event_free_finalize(0, (struct event *)ev, finalize_fd);
-	return 0;
-}
-
 static void listen_cb(evutil_socket_t sock, short what, void * /*priv*/) {
 	struct event *ev;
 	int fd;
@@ -246,9 +240,23 @@ int api_socket_start(struct event_base *base) {
 	return 0;
 }
 
+static int collect_clients(const struct event_base *, const struct event *ev, void *priv) {
+	struct event ***events = priv;
+	event_callback_fn cb = event_get_callback(ev);
+	if (cb == read_cb || cb == write_cb)
+		gr_vec_add(*events, (struct event *)ev);
+	return 0;
+}
+
 void api_socket_stop(struct event_base *) {
+	struct event **events = NULL;
+	struct event *ev;
+
 	if (ev_listen != NULL)
 		event_free_finalize(0, ev_listen, finalize_fd);
 
-	event_base_foreach_event(ev_base, close_connections, NULL);
+	event_base_foreach_event(ev_base, collect_clients, &events);
+	gr_vec_foreach (ev, events)
+		event_free_finalize(0, ev, finalize_fd);
+	gr_vec_free(events);
 }