From 6342344ed988652de5cf1b5f14aa037e4042853c Mon Sep 17 00:00:00 2001 From: YAMAMOTO Takashi Date: Mon, 12 Jun 2023 14:55:27 +0900 Subject: [PATCH 1/2] Fix a use-after-free bug for detached threads the issue was pointed out by @alexcrichton in https://github.com/WebAssembly/wasi-libc/issues/405 --- .../musl/src/thread/pthread_create.c | 34 ++++++++++++++++++- 1 file changed, 33 insertions(+), 1 deletion(-) diff --git a/libc-top-half/musl/src/thread/pthread_create.c b/libc-top-half/musl/src/thread/pthread_create.c index 12094d634..0ac58b9f5 100644 --- a/libc-top-half/musl/src/thread/pthread_create.c +++ b/libc-top-half/musl/src/thread/pthread_create.c @@ -60,6 +60,18 @@ void __tl_sync(pthread_t td) if (tl_lock_waiters) __wake(&__thread_list_lock, 1, 0); } +#ifndef __wasilibc_unmodified_upstream +/* this queue has at most one entry */ +static void *map_base_lazy_free_queue; + +static void process_map_base_lazy_free_queue() +{ + /* called with __tl_lock held */ + free(map_base_lazy_free_queue); + map_base_lazy_free_queue = NULL; +} +#endif + #ifdef __wasilibc_unmodified_upstream _Noreturn void __pthread_exit(void *result) #else @@ -182,7 +194,17 @@ static void __pthread_exit(void *result) } #else if (state==DT_DETACHED && self->map_base) { - free(self->map_base); + // As we use malloc/free which is considerably more complex + // than mmap/munmap to call and can even require a valid + // thread context, it's difficult to implement __unmapself. + // + // Here we take an alternative approach which simply defers + // the deallocation. An obvious downside of this approach is + // that it keeps the stack longer. (possibly forever.) + // To avoid wasting too much memory, we keep the free queue + // length at most one. + process_map_base_lazy_free_queue(); + map_base_lazy_free_queue = self->map_base; // Can't use `exit()` here, because it is too high level return; } @@ -424,6 +446,16 @@ int __pthread_create(pthread_t *restrict res, const pthread_attr_t *restrict att if (map == MAP_FAILED) goto fail; } #else + /* + * Process the free queue before allocationg a new one. + * Hopefully it enables a reuse of the memory. + * + * Note: We can't perform a simple "handoff" becasue allocation + * sizes might be different. (eg. the stack size might differ) + */ + __tl_lock(); + process_map_base_lazy_free_queue(); + __tl_unlock(); map = malloc(size); if (!map) goto fail; #endif From 6df86ea9dff61ed31391ca5041437126309cedee Mon Sep 17 00:00:00 2001 From: YAMAMOTO Takashi Date: Fri, 16 Jun 2023 19:51:27 +0900 Subject: [PATCH 2/2] Rename map_base_lazy_free_queue as it only keeps a single item Also, align the comment style with musl. --- .../musl/src/thread/pthread_create.c | 41 +++++++++---------- 1 file changed, 19 insertions(+), 22 deletions(-) diff --git a/libc-top-half/musl/src/thread/pthread_create.c b/libc-top-half/musl/src/thread/pthread_create.c index 0ac58b9f5..5de9f5a0c 100644 --- a/libc-top-half/musl/src/thread/pthread_create.c +++ b/libc-top-half/musl/src/thread/pthread_create.c @@ -61,14 +61,13 @@ void __tl_sync(pthread_t td) } #ifndef __wasilibc_unmodified_upstream -/* this queue has at most one entry */ -static void *map_base_lazy_free_queue; +static void *map_base_deferred_free; -static void process_map_base_lazy_free_queue() +static void process_map_base_deferred_free() { /* called with __tl_lock held */ - free(map_base_lazy_free_queue); - map_base_lazy_free_queue = NULL; + free(map_base_deferred_free); + map_base_deferred_free = NULL; } #endif @@ -194,17 +193,17 @@ static void __pthread_exit(void *result) } #else if (state==DT_DETACHED && self->map_base) { - // As we use malloc/free which is considerably more complex - // than mmap/munmap to call and can even require a valid - // thread context, it's difficult to implement __unmapself. - // - // Here we take an alternative approach which simply defers - // the deallocation. An obvious downside of this approach is - // that it keeps the stack longer. (possibly forever.) - // To avoid wasting too much memory, we keep the free queue - // length at most one. - process_map_base_lazy_free_queue(); - map_base_lazy_free_queue = self->map_base; + /* As we use malloc/free which is considerably more complex + * than mmap/munmap to call and can even require a valid + * thread context, it's difficult to implement __unmapself. + * + * Here we take an alternative approach which simply defers + * the deallocation. An obvious downside of this approach is + * that it keeps the stack longer. (possibly forever.) + * To avoid wasting too much memory, we only defer a single + * item at most. */ + process_map_base_deferred_free(); + map_base_deferred_free = self->map_base; // Can't use `exit()` here, because it is too high level return; } @@ -446,15 +445,13 @@ int __pthread_create(pthread_t *restrict res, const pthread_attr_t *restrict att if (map == MAP_FAILED) goto fail; } #else - /* - * Process the free queue before allocationg a new one. - * Hopefully it enables a reuse of the memory. + /* Process the deferred free request if any before + * allocationg a new one. Hopefully it enables a reuse of the memory. * * Note: We can't perform a simple "handoff" becasue allocation - * sizes might be different. (eg. the stack size might differ) - */ + * sizes might be different. (eg. the stack size might differ) */ __tl_lock(); - process_map_base_lazy_free_queue(); + process_map_base_deferred_free(); __tl_unlock(); map = malloc(size); if (!map) goto fail;