Skip to content

Commit

Permalink
pkg/lwip: fix race in async sock API with event
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
maribu committed Dec 16, 2024
1 parent 4044c85 commit 8ac3a43
Showing 1 changed file with 21 additions and 0 deletions.
21 changes: 21 additions & 0 deletions pkg/lwip/contrib/sock/tcp/lwip_sock_tcp.c
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand Down Expand Up @@ -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));
}
Expand Down Expand Up @@ -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);
Expand Down

0 comments on commit 8ac3a43

Please sign in to comment.