Fix memory leak

.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.

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;
 }

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);

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