diff --git a/.release-notes/leak.md b/.release-notes/leak.md new file mode 100644 index 0000000000..67028e199b --- /dev/null +++ b/.release-notes/leak.md @@ -0,0 +1,3 @@ +## Fix memory leak + +In the process of fixing a "in theory could be use after free" that tooling found, we created a real memory leak. We've reverted back to the "in theory bad, in practice not" code from before and fixed the leak. diff --git a/src/libponyrt/sched/mpmcq.c b/src/libponyrt/sched/mpmcq.c index 7f747a389b..461c751a19 100644 --- a/src/libponyrt/sched/mpmcq.c +++ b/src/libponyrt/sched/mpmcq.c @@ -1,7 +1,6 @@ #define PONY_WANT_ATOMIC_DEFS #include "mpmcq.h" -#include "ponyassert.h" #include "../mem/pool.h" #include "../sched/cpu.h" @@ -17,19 +16,9 @@ struct mpmcq_node_t PONY_ATOMIC(void*) data; }; -static __pony_thread_local mpmcq_node_t* nodes_to_recycle; - static mpmcq_node_t* node_alloc(void* data) { - mpmcq_node_t* node = nodes_to_recycle; - - if (NULL != node) - { - nodes_to_recycle = atomic_load_explicit(&node->next, memory_order_relaxed); - } else { - node = POOL_ALLOC(mpmcq_node_t); - } - + mpmcq_node_t* node = POOL_ALLOC(mpmcq_node_t); atomic_store_explicit(&node->next, NULL, memory_order_relaxed); atomic_store_explicit(&node->data, data, memory_order_relaxed); @@ -66,7 +55,6 @@ static size_t mpmcq_size_debug(mpmcq_t* q) void ponyint_mpmcq_init(mpmcq_t* q) { - nodes_to_recycle = NULL; mpmcq_node_t* node = node_alloc(NULL); atomic_store_explicit(&q->head, node, memory_order_relaxed); @@ -78,20 +66,8 @@ void ponyint_mpmcq_init(mpmcq_t* q) #endif } -void ponyint_mpmcq_cleanup() -{ - while(nodes_to_recycle != NULL) - { - mpmcq_node_t* node = nodes_to_recycle; - nodes_to_recycle = atomic_load_explicit(&node->next, memory_order_relaxed); - node_free(node); - } -} - void ponyint_mpmcq_destroy(mpmcq_t* q) { - pony_assert(atomic_load_explicit(&q->head, memory_order_relaxed) == q->tail.object); - atomic_store_explicit(&q->head, NULL, memory_order_relaxed); node_free(q->tail.object); q->tail.object = NULL; @@ -136,6 +112,11 @@ void ponyint_mpmcq_push_single(mpmcq_t* q, void* data) atomic_store_explicit(&prev->next, node, memory_order_release); } +// There's a known issue with "use after free" in the do loop. However, +// we are handling in a way that ASAN doesn't understand with the CAS so +// it is actually ok. If the CAS loop is changed, this no sanitize might +// become problematic. +__attribute__((no_sanitize_address)) void* ponyint_mpmcq_pop(mpmcq_t* q) { PONY_ABA_PROTECTED_PTR(mpmcq_node_t) cmp; @@ -201,14 +182,7 @@ void* ponyint_mpmcq_pop(mpmcq_t* q) ANNOTATE_HAPPENS_AFTER(&tail->data); #endif - // if tail has already been consumed and freed by this thread, the read of - // `tail->next` above in another thread would be a use-after-free error but is - // technically safe in this context since we will end up looping due to the CAS - // failing in the while condition due to the aba counter being out of sync - // we cannot free the tail node if we want to ensure there are no use-after-free - // concerns so we instead add it to the list of nodes to recycle - atomic_store_explicit(&tail->next, nodes_to_recycle, memory_order_relaxed); - nodes_to_recycle = tail; + node_free(tail); return data; } diff --git a/src/libponyrt/sched/mpmcq.h b/src/libponyrt/sched/mpmcq.h index 71e30ad184..b08fcb5ab4 100644 --- a/src/libponyrt/sched/mpmcq.h +++ b/src/libponyrt/sched/mpmcq.h @@ -24,8 +24,6 @@ void ponyint_mpmcq_init(mpmcq_t* q); void ponyint_mpmcq_destroy(mpmcq_t* q); -void ponyint_mpmcq_cleanup(); - void ponyint_mpmcq_push(mpmcq_t* q, void* data); void ponyint_mpmcq_push_single(mpmcq_t* q, void* data); diff --git a/src/libponyrt/sched/scheduler.c b/src/libponyrt/sched/scheduler.c index df4f2cc6e7..ef7d2397c8 100644 --- a/src/libponyrt/sched/scheduler.c +++ b/src/libponyrt/sched/scheduler.c @@ -1305,8 +1305,6 @@ static DECLARE_THREAD_FN(run_thread) #endif run(sched); - - ponyint_mpmcq_cleanup(); ponyint_pool_thread_cleanup(); return 0; @@ -1803,7 +1801,6 @@ bool ponyint_sched_start(bool library) ponyint_sched_shutdown(); } - ponyint_mpmcq_cleanup(); ponyint_pool_thread_cleanup(); while(ponyint_thread_messageq_pop(&this_scheduler->mq