From 7647ea7c83627c02f5ccb88295cee5f795156b1a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Barnab=C3=A1s=20P=C5=91cze?= Date: Sat, 19 Feb 2022 13:31:01 +0100 Subject: [PATCH] spa: support: loop: fix use-after-free when loop is reentered The core of the issue is the following: what happens if an active source is destroyed before it could be dispatched? For loop-managed sources (`struct source_impl`) this was addressed by storing all destroyed sources in a list, and only freeing them after dispatching has been finished. (0eb73f0f06a2e40362990080007db295141f6f1f) This approach works for both strictly single-threaded and `pw_thread_loop` loops assuming the loop is not reentered. However, if the loop is reentered, there can still be issues. Assume that in one iteration sources A and B are active, and returned from the system call, and source B is destroyed before the loop starts dispatching. Consider what happens when "A" is dispatched first, and it reenters the loop with timeout 0. Imagine there are no new events, so `loop_iterate()` will immediately return, but it will first destroy everything in the destroy list (this is done at the end of `loop_iterate()`). And herein lies the problem. In the previous iteration, there exists a `spa_poll_event` object which points to source "B", but that has just been destroyed at the end of the recursive iteration. This will trigger a use-after-free once the previous iteration inspects it. Fix that by processing the destroy list right after first processing the returned `spa_poll_event` objects, and "detach" the source from the loop and its iterations in `process_destroy()` before the source is destroyed. See #2114 #2147 --- spa/plugins/support/loop.c | 53 ++++++++++++++++++++++++++++---------- 1 file changed, 39 insertions(+), 14 deletions(-) diff --git a/spa/plugins/support/loop.c b/spa/plugins/support/loop.c index 693495cc6..02ad8e359 100644 --- a/spa/plugins/support/loop.c +++ b/spa/plugins/support/loop.c @@ -132,23 +132,35 @@ static int loop_update_source(void *object, struct spa_source *source) return spa_system_pollfd_mod(impl->system, impl->poll_fd, source->fd, source->mask, source); } -static int loop_remove_source(void *object, struct spa_source *source) +static void detach_source(struct spa_source *source) { - struct impl *impl = object; struct spa_poll_event *e; - spa_assert(source->loop == &impl->loop); + source->loop = NULL; + source->rmask = 0; if ((e = source->priv)) { /* active in an iteration of the loop, remove it from there */ e->data = NULL; source->priv = NULL; } - source->loop = NULL; - source->rmask = 0; +} + +static int remove_from_poll(struct impl *impl, struct spa_source *source) +{ + spa_assert(source->loop == &impl->loop); + return spa_system_pollfd_del(impl->system, impl->poll_fd, source->fd); } +static int loop_remove_source(void *object, struct spa_source *source) +{ + int res = remove_from_poll(object, source); + detach_source(source); + + return res; +} + static void flush_items(struct impl *impl) { uint32_t index; @@ -337,11 +349,19 @@ static void loop_leave(void *object) impl->thread = 0; } +static inline void free_source(struct source_impl *s) +{ + detach_source(&s->source); + free(s); +} + static inline void process_destroy(struct impl *impl) { struct source_impl *source, *tmp; + spa_list_for_each_safe(source, tmp, &impl->destroy_list, link) - free(source); + free_source(source); + spa_list_init(&impl->destroy_list); } @@ -371,11 +391,16 @@ static int loop_iterate(void *object, int timeout) e->data = NULL; s->priv = &ep[i]; } + + if (SPA_UNLIKELY(!spa_list_is_empty(&impl->destroy_list))) + process_destroy(impl); + for (i = 0; i < nfds; i++) { struct spa_source *s = ep[i].data; - if (SPA_LIKELY(s && s->rmask && s->loop)) + if (SPA_LIKELY(s && s->rmask)) s->func(s); } + for (i = 0; i < nfds; i++) { struct spa_source *s = ep[i].data; if (SPA_LIKELY(s)) { @@ -383,8 +408,6 @@ static int loop_iterate(void *object, int timeout) s->priv = NULL; } } - if (SPA_UNLIKELY(!spa_list_is_empty(&impl->destroy_list))) - process_destroy(impl); return nfds; } @@ -748,14 +771,18 @@ static void loop_destroy_source(void *object, struct spa_source *source) if (s->fallback) loop_destroy_source(s->impl, s->fallback); - else if (source->loop) - loop_remove_source(s->impl, source); + else + remove_from_poll(s->impl, source); if (source->fd != -1 && s->close) { spa_system_close(s->impl->system, source->fd); source->fd = -1; } - spa_list_insert(&s->impl->destroy_list, &s->link); + + if (!s->impl->polling) + free_source(s); + else + spa_list_insert(&s->impl->destroy_list, &s->link); } static const struct spa_loop_methods impl_loop = { @@ -826,8 +853,6 @@ static int impl_clear(struct spa_handle *handle) spa_list_consume(source, &impl->source_list, link) loop_destroy_source(impl, &source->source); - process_destroy(impl); - spa_system_close(impl->system, impl->ack_fd); spa_system_close(impl->system, impl->poll_fd);