pkg/lwip: fix race in async sock API with event

maribu · maribu · commit 8ac3a430e723 · 2024-12-16T19:50:39.000+01:00
In TCP server mode, the sock_tcp_t sockets are managed by the network
stack and can be reused if a previous connection is no longer in used.
However, an event may still be posted in the event queue when the socket
is reused. Wiping it will result in the `next` pointer in that event to
be NULL, which will cause the event handler fetching that event to
crash.

This adds an `event_cancel()` at two places:
1. Just before reusing the socket
2. During sock_tcp_disconnect()

The former catches issues in server mode e.g. when a connect has been
closed (e.g. due to timeout) and is reused before a pending event (e.g.
a timeout event) has been processed.

The letter may be an issue on client side. E.g. when `sock_tcp_t` was
allocated on stack and goes out of scope after `sock_tcp_disconnect`
but before the event handler was run.
diff --git a/pkg/lwip/contrib/sock/tcp/lwip_sock_tcp.c b/pkg/lwip/contrib/sock/tcp/lwip_sock_tcp.c
@@ -85,6 +85,9 @@ int sock_tcp_listen(sock_tcp_queue_t *queue, const sock_tcp_ep_t *local,
     queue->array = queue_array;
     queue->len = queue_len;
     queue->used = 0;
+    /* This wipe is important: We cancel pending events in async event API when
+     * reusing the socket. If the memory would be uninitialized, bad things
+     * would happen */
     memset(queue->array, 0, sizeof(sock_tcp_t) * queue_len);
     mutex_unlock(&queue->mutex);
 
@@ -125,6 +128,14 @@ void sock_tcp_disconnect(sock_tcp_t *sock)
         }
     }
 
+#ifdef SOCK_HAS_ASYNC_CTX
+    /* Cancel any pending event in the event queue */
+    if (sock->base.async_ctx.queue) {
+        event_cancel(sock->base.async_ctx.queue,
+                     &sock->base.async_ctx.event.super);
+    }
+#endif
+
     mutex_unlock(&sock->mutex);
     memset(&sock->mutex, 0, sizeof(mutex_t));
 }
@@ -240,6 +251,16 @@ int sock_tcp_accept(sock_tcp_queue_t *queue, sock_tcp_t **sock,
             for (unsigned short i = 0; i < queue->len; i++) {
                 sock_tcp_t *s = &queue->array[i];
                 if (s->base.conn == NULL) {
+#ifdef SOCK_HAS_ASYNC_CTX
+                    /* If there still is an event pending, we cannot just wipe
+                     * its memory but have to remove the event from the list
+                     * first. We rely here sock_tcp_listen to zero-initialize
+                     * the sockets. */
+                    if (s->base.async_ctx.queue) {
+                        event_cancel(s->base.async_ctx.queue,
+                                     &s->base.async_ctx.event.super);
+                    }
+#endif
                     _tcp_sock_init(s, tmp, queue);
                     queue->used++;
                     assert(queue->used > 0);
