From c40b3337280cb59ba511120dc8887ca501e1b77c Mon Sep 17 00:00:00 2001
From: Isaku Yamahata <isaku.yamahata@gmail.com>
Date: Tue, 26 Nov 2019 23:23:33 -0800
Subject: [PATCH] [LibOS] Trigger callbacks only after list traversal in Async
 Helper

In Async Helper thread, async_list is protected by async_list_lock.
Previously, this thread unlocked async_list_lock on each detected
triggered entry to perform a callback during traversal of async_list
(unlocking is needed because it is unknown how much time the callback
takes to execute). However, once we unlock, the list may become
unstable (install_async_event may delete/append an entry). Then, next
FOR_EACH_LIST_SAFE list entry may be freed while trigering callback,
and the next iteration may try to dereference this invalid pointer.
This commit alleviates this scenario by creating a temporal list of
triggered entries and performing all callbacks after list traversal.
---
 LibOS/shim/src/shim_async.c | 28 ++++++++++++++++++++++++----
 1 file changed, 24 insertions(+), 4 deletions(-)

diff --git a/LibOS/shim/src/shim_async.c b/LibOS/shim/src/shim_async.c
index eeebd64d..c58b74c9 100644
--- a/LibOS/shim/src/shim_async.c
+++ b/LibOS/shim/src/shim_async.c
@@ -193,6 +193,10 @@ static void shim_async_helper(void * arg) {
             break;
         }
 
+        LISTP_TYPE(async_event) triggered;
+        INIT_LISTP(&triggered);
+
+    again:;
         /* Iterate through all async IO events and alarm/timer events to:
          *   - call callbacks for all triggered events, and
          *   - repopulate object_list with async IO events (if any), and
@@ -207,16 +211,22 @@ static void shim_async_helper(void * arg) {
             if (polled && tmp->object == polled) {
                 debug("Async IO event triggered at %lu\n", now);
                 unlock(&async_helper_lock);
+                /* FIXME: potential race condition when
+                 * ioctl(FIOASYNC, off) and cleanup on fd-close are
+                 * correctly implemented. tmp can be freed at the same
+                 * time. */
                 tmp->callback(tmp->caller, tmp->arg);
+
+                /* async_list may be changed because async_helper_lock is
+                 * released; list traverse cannot be continued. */
+                polled = NULL;
                 lock(&async_helper_lock);
+                goto again;
             } else if (tmp->expire_time && tmp->expire_time <= now) {
                 debug("Async alarm/timer triggered at %lu (expired at %lu)\n",
                         now, tmp->expire_time);
                 LISTP_DEL(tmp, &async_list, list);
-                unlock(&async_helper_lock);
-                tmp->callback(tmp->caller, tmp->arg);
-                free(tmp);
-                lock(&async_helper_lock);
+                LISTP_ADD_TAIL(tmp, &triggered, list);
                 continue;
             }
 
@@ -242,6 +252,16 @@ static void shim_async_helper(void * arg) {
             }
         }
 
+        if (!LISTP_EMPTY(&triggered)) {
+            unlock(&async_helper_lock);
+            LISTP_FOR_EACH_ENTRY_SAFE(tmp, n, &triggered, list) {
+                LISTP_DEL(tmp, &triggered, list);
+                tmp->callback(tmp->caller, tmp->arg);
+                free(tmp);
+            }
+            lock(&async_helper_lock);
+        }
+
         uint64_t sleep_time;
         if (next_expire_time) {
             sleep_time = next_expire_time - now;