[LibOS] Add missing put_thread() calls

There were some places where put_thread() calls were missing. This
caused reference counter to never reach 0, so that the shim_thread
struct was never freed, thus leaking memory.

LibOS/shim/include/shim_thread.h

@@ -216,10 +216,11 @@ static inline void thread_setwait (struct shim_thread ** queue,
 {
     if (!thread)
         thread = get_cur_thread();
-    get_thread(thread);
     DkEventClear(thread->scheduler_event);
-    if (queue)
+    if (queue) {
+        get_thread(thread);
         *queue = thread;
+    }
 }
 
 static inline int thread_sleep (uint64_t timeout_us)
@@ -246,8 +247,16 @@ static inline void thread_wakeup (struct shim_thread * thread)
 
 extern struct shim_lock thread_list_lock;
 
-struct shim_thread * __lookup_thread (IDTYPE tid);
-struct shim_thread * lookup_thread (IDTYPE tid);
+/*!
+ * \brief Look up the thread for a given id.
+ *
+ * \param tid Thread id to look for.
+ *
+ * Searches global threads list for a thread with id equal to \p tid.
+ * If no thread was found returns NULL.
+ * Increases refcount of the returned thread.
+ */
+struct shim_thread* lookup_thread(IDTYPE tid);
 struct shim_simple_thread * __lookup_simple_thread (IDTYPE tid);
 struct shim_simple_thread * lookup_simple_thread (IDTYPE tid);
 

LibOS/shim/src/bookkeep/shim_thread.c

@@ -76,9 +76,8 @@ void dump_threads (void)
     unlock(&thread_list_lock);
 }
 
-struct shim_thread * __lookup_thread (IDTYPE tid)
-{
-    struct shim_thread * tmp;
+static struct shim_thread* __lookup_thread(IDTYPE tid) {
+    struct shim_thread* tmp;
 
     LISTP_FOR_EACH_ENTRY(tmp, &thread_list, list) {
         if (tmp->tid == tid) {
@@ -90,10 +89,9 @@ struct shim_thread * __lookup_thread (IDTYPE tid)
     return NULL;
 }
 
-struct shim_thread * lookup_thread (IDTYPE tid)
-{
+struct shim_thread* lookup_thread(IDTYPE tid) {
     lock(&thread_list_lock);
-    struct shim_thread * thread = __lookup_thread(tid);
+    struct shim_thread* thread = __lookup_thread(tid);
     unlock(&thread_list_lock);
     return thread;
 }
@@ -319,7 +317,7 @@ void put_thread (struct shim_thread * thread)
     int ref_count = REF_DEC(thread->ref_count);
 
 #ifdef DEBUG_REF
-    debug("put thread %p(%d) (ref_count = %d)\n", thread, thread->tid,
+    debug("put_thread %p(%d) (ref_count = %d)\n", thread, thread->tid,
           ref_count);
 #endif
 

LibOS/shim/src/fs/proc/thread.c

@@ -244,8 +244,8 @@ static int parse_thread_fd(const char* name, const char** rest, struct shim_hand
 
     if (fd >= handle_map->fd_top || handle_map->map[fd] == NULL ||
         handle_map->map[fd]->handle == NULL) {
-        unlock(&handle_map->lock);
-        return -ENOENT;
+        ret = -ENOENT;
+        goto out;
     }
 
     if (phdl) {
@@ -253,12 +253,15 @@ static int parse_thread_fd(const char* name, const char** rest, struct shim_hand
         get_handle(*phdl);
     }
 
-    unlock(&handle_map->lock);
-
     if (rest)
         *rest = *p ? p + 1 : NULL;
 
-    return 0;
+    ret = 0;
+
+out:
+    unlock(&handle_map->lock);
+    put_thread(thread);
+    return ret;
 }
 
 static int proc_match_thread_each_fd(const char* name) {
@@ -664,6 +667,8 @@ static int proc_thread_dir_stat(const char* name, struct stat* buf) {
     buf->st_gid = thread->gid;
     unlock(&thread->lock);
     buf->st_size = 4096;
+
+    put_thread(thread);
     return 0;
 }
 
@@ -679,7 +684,12 @@ static int proc_match_thread(const char* name) {
 
     struct shim_thread* thread = lookup_thread(pid);
 
-    return thread ? 1 : 0;
+    if (thread) {
+        put_thread(thread);
+        return 1;
+    }
+
+    return 0;
 }
 
 struct walk_thread_arg {

LibOS/shim/src/ipc/shim_ipc_pid.c

@@ -735,13 +735,12 @@ static LISTP_TYPE(rpcmsg) rpc_msgs;
 static LISTP_TYPE(rpcreq) rpc_reqs;
 static struct shim_lock rpc_queue_lock;
 
-int get_rpc_msg (IDTYPE * sender, void * buf, int len)
-{
+int get_rpc_msg(IDTYPE* sender, void* buf, int len) {
     create_lock_runtime(&rpc_queue_lock);
     lock(&rpc_queue_lock);
 
     if (!LISTP_EMPTY(&rpc_msgs)) {
-        struct rpcmsg * m = LISTP_FIRST_ENTRY(&rpc_msgs, struct rpcmsg, list);
+        struct rpcmsg* m = LISTP_FIRST_ENTRY(&rpc_msgs, struct rpcmsg, list);
         LISTP_DEL(m, &rpc_msgs, list);
         if (m->len < len)
             len = m->len;
@@ -752,7 +751,7 @@ int get_rpc_msg (IDTYPE * sender, void * buf, int len)
         return len;
     }
 
-    struct rpcreq * r = malloc(sizeof(struct rpcreq));
+    struct rpcreq* r = malloc(sizeof(struct rpcreq));
     if (!r) {
         unlock(&rpc_queue_lock);
         return -ENOMEM;
@@ -764,12 +763,17 @@ int get_rpc_msg (IDTYPE * sender, void * buf, int len)
     r->buffer = buf;
     thread_setwait(&r->thread, NULL);
     LISTP_ADD_TAIL(r, &rpc_reqs, list);
+
     unlock(&rpc_queue_lock);
     thread_sleep(NO_TIMEOUT);
+
     put_thread(r->thread);
     if (sender)
         *sender = r->sender;
-    return r->len;
+    int ret = r->len;
+
+    free(r);
+    return ret;
 }
 
 int ipc_pid_sendrpc_callback (IPC_CALLBACK_ARGS)

LibOS/shim/src/sys/shim_alarm.c

@@ -39,6 +39,7 @@ static void signal_alarm(IDTYPE target, void* arg) {
     lock(&thread->lock);
     append_signal(thread, SIGALRM, NULL, true);
     unlock(&thread->lock);
+    put_thread(thread);
 }
 
 int shim_do_alarm(unsigned int seconds) {

LibOS/shim/src/sys/shim_clone.c

@@ -125,9 +125,10 @@ int clone_implementation_wrapper(struct clone_args * arg)
     object_wait_with_retry(arg->create_event);
     DkObjectClose(arg->create_event);
 
-    struct shim_thread * my_thread = arg->thread;
+    /* We acquired ownership of arg->thread from the caller, hence there is
+     * no need to call get_thread. */
+    struct shim_thread* my_thread = arg->thread;
     assert(my_thread);
-    get_thread(my_thread);
 
     if (!my_thread->tcb) {
         stack_allocated = 1;
@@ -172,6 +173,8 @@ int clone_implementation_wrapper(struct clone_args * arg)
     fixup_child_context(tcb->context.regs);
     tcb->context.regs->rsp = (unsigned long)stack;
 
+    put_thread(my_thread);
+
     restore_context(&tcb->context);
     return 0;
 }
@@ -387,6 +390,9 @@ int shim_do_clone (int flags, void * user_stack_addr, int * parent_tidptr,
         goto clone_thread_failed;
     }
 
+    /* Increasing refcount due to copy below. Passing ownership of the new copy
+     * of this pointer to the new thread (receiver of new_args). */
+    get_thread(thread);
     new_args.thread    = thread;
     new_args.parent    = self;
     new_args.stack     = user_stack_addr;
@@ -401,6 +407,7 @@ int shim_do_clone (int flags, void * user_stack_addr, int * parent_tidptr,
                                           &new_args);
     if (!pal_handle) {
         ret = -PAL_ERRNO;
+        put_thread(new_args.thread);
         goto clone_thread_failed;
     }
 

LibOS/shim/src/sys/shim_futex.c

@@ -51,6 +51,28 @@ DEFINE_LISTP(shim_futex_handle);
 static LISTP_TYPE(shim_futex_handle) futex_list = LISTP_INIT;
 static struct shim_lock futex_list_lock;
 
+static void add_futex_waiter(struct futex_waiter* waiter,
+                             struct shim_futex_handle* futex,
+                             uint32_t bitset) {
+    thread_setwait(&waiter->thread, NULL);
+    INIT_LIST_HEAD(waiter, list);
+    waiter->bitset = bitset;
+    LISTP_ADD_TAIL(waiter, &futex->waiters, list);
+}
+
+static void del_futex_waiter(struct futex_waiter* waiter, struct shim_futex_handle* futex) {
+    LISTP_DEL_INIT(waiter, &futex->waiters, list);
+    assert(waiter->thread);
+    put_thread(waiter->thread);
+}
+
+static void del_futex_waiter_wakeup(struct futex_waiter* waiter, struct shim_futex_handle* futex) {
+    LISTP_DEL_INIT(waiter, &futex->waiters, list);
+    assert(waiter->thread);
+    thread_wakeup(waiter->thread);
+    put_thread(waiter->thread);
+}
+
 int shim_do_futex(int* uaddr, int op, int val, void* utime, int* uaddr2, int val3) {
     struct shim_futex_handle* tmp = NULL;
     struct shim_futex_handle* futex = NULL;
@@ -172,24 +194,23 @@ int shim_do_futex(int* uaddr, int op, int val, void* utime, int* uaddr2, int val
                     break;
                 }
 
-                struct futex_waiter waiter;
-                thread_setwait(&waiter.thread, NULL);
-                INIT_LIST_HEAD(&waiter, list);
-                waiter.bitset = bitset;
-                LISTP_ADD_TAIL(&waiter, &futex->waiters, list);
+                struct futex_waiter waiter = { 0 };
+                add_futex_waiter(&waiter, futex, bitset);
 
                 unlock(&hdl->lock);
                 ret = thread_sleep(timeout_us);
                 /* DEP 1/28/17: Should return ETIMEDOUT, not EAGAIN, on timeout. */
                 if (ret == -EAGAIN)
                     ret = -ETIMEDOUT;
-                if (ret == -ETIMEDOUT)
-                    LISTP_DEL(&waiter, &futex->waiters, list);
+                if (ret == -ETIMEDOUT) {
+                    del_futex_waiter(&waiter, futex);
+                }
                 lock(&hdl->lock);
                 /* Chia-Che 10/17/17: FUTEX_WAKE should remove the waiter
                  * from the list; if not, we should remove it now. */
-                if (!LIST_EMPTY(&waiter, list))
-                    LISTP_DEL(&waiter, &futex->waiters, list);
+                if (!LIST_EMPTY(&waiter, list)) {
+                    del_futex_waiter(&waiter, futex);
+                }
                 break;
             }
 
@@ -209,8 +230,7 @@ int shim_do_futex(int* uaddr, int op, int val, void* utime, int* uaddr2, int val
                 debug("FUTEX_WAKE wake thread %d: %p (val = %d)\n", waiter->thread->tid, uaddr,
                       *uaddr);
 
-                LISTP_DEL_INIT(waiter, &futex->waiters, list);
-                thread_wakeup(waiter->thread);
+                del_futex_waiter_wakeup(waiter, futex);
                 nwaken++;
                 if (nwaken >= val)
                     break;
@@ -273,8 +293,7 @@ int shim_do_futex(int* uaddr, int op, int val, void* utime, int* uaddr2, int val
             LISTP_FOR_EACH_ENTRY_SAFE(waiter, wtmp, &futex->waiters, list) {
                 debug("FUTEX_WAKE_OP wake thread %d: %p (val = %d)\n", waiter->thread->tid, uaddr,
                       *uaddr);
-                LISTP_DEL_INIT(waiter, &futex->waiters, list);
-                thread_wakeup(waiter->thread);
+                del_futex_waiter_wakeup(waiter, futex);
                 nwaken++;
             }
 
@@ -287,8 +306,7 @@ int shim_do_futex(int* uaddr, int op, int val, void* utime, int* uaddr2, int val
                 LISTP_FOR_EACH_ENTRY_SAFE(waiter, wtmp, &futex2->waiters, list) {
                     debug("FUTEX_WAKE_OP(2) wake thread %d: %p (val = %d)\n", waiter->thread->tid,
                           uaddr2, *uaddr2);
-                    LISTP_DEL_INIT(waiter, &futex2->waiters, list);
-                    thread_wakeup(waiter->thread);
+                    del_futex_waiter_wakeup(waiter, futex2);
                     nwaken++;
                 }
             }
@@ -308,8 +326,7 @@ int shim_do_futex(int* uaddr, int op, int val, void* utime, int* uaddr2, int val
             struct futex_waiter* wtmp;
             int nwaken = 0;
             LISTP_FOR_EACH_ENTRY_SAFE(waiter, wtmp, &futex->waiters, list) {
-                LISTP_DEL_INIT(waiter, &futex->waiters, list);
-                thread_wakeup(waiter->thread);
+                del_futex_waiter_wakeup(waiter, futex);
                 nwaken++;
                 if (nwaken >= val)
                     break;
@@ -361,10 +378,12 @@ int shim_do_get_robust_list(pid_t pid, struct robust_list_head** head, size_t* l
             return -ESRCH;
     } else {
         thread = get_cur_thread();
+        get_thread(thread);
     }
 
     *head = (struct robust_list_head*)thread->robust_list;
     *len  = sizeof(struct robust_list_head);
+    put_thread(thread);
     return 0;
 }
 
@@ -403,8 +422,7 @@ void release_robust_list(struct robust_list_head* head) {
         debug("release robust list: %p\n", futex_addr);
         *(int*)futex_addr = 0;
         LISTP_FOR_EACH_ENTRY_SAFE(waiter, wtmp, &futex->waiters, list) {
-            LISTP_DEL_INIT(waiter, &futex->waiters, list);
-            thread_wakeup(waiter->thread);
+            del_futex_waiter_wakeup(waiter, futex);
         }
 
         unlock(&hdl->lock);
@@ -443,8 +461,7 @@ void release_clear_child_id(int* clear_child_tid) {
     debug("release futex at %p\n", clear_child_tid);
     *clear_child_tid = 0;
     LISTP_FOR_EACH_ENTRY_SAFE(waiter, wtmp, &futex->waiters, list) {
-        LISTP_DEL_INIT(waiter, &futex->waiters, list);
-        thread_wakeup(waiter->thread);
+        del_futex_waiter_wakeup(waiter, futex);
     }
 
     unlock(&hdl->lock);

LibOS/shim/src/sys/shim_getpid.c

@@ -148,26 +148,38 @@ gid_t shim_do_getegid(void) {
 }
 
 int shim_do_setpgid(pid_t pid, pid_t pgid) {
-    struct shim_thread* thread = pid ? lookup_thread(pid) : get_cur_thread();
+    struct shim_thread* thread;
 
-    if (!pid)
-        assert(thread);
-
-    if (!thread)
-        return -ESRCH;
+    if (pid) {
+        thread = lookup_thread(pid);
+        if (!thread) {
+            return -ESRCH;
+        }
+    } else {
+        thread = get_cur_thread();
+        get_thread(thread);
+    }
 
     thread->pgid = (IDTYPE)pgid ?: thread->tgid;
 
+    put_thread(thread);
     return 0;
 }
 
 int shim_do_getpgid(pid_t pid) {
-    struct shim_thread* thread = pid ? lookup_thread(pid) : get_cur_thread();
+    if (!pid) {
+        return get_cur_thread()->pgid;
+    }
 
-    if (!thread)
+    struct shim_thread* thread = lookup_thread(pid);
+
+    if (!thread) {
         return -ESRCH;
+    }
 
-    return thread->pgid;
+    int ret = thread->pgid;
+    put_thread(thread);
+    return ret;
 }
 
 pid_t shim_do_getpgrp(void) {
@@ -191,10 +203,5 @@ int shim_do_setsid(void) {
 }
 
 int shim_do_getsid(pid_t pid) {
-    struct shim_thread* thread = pid ? lookup_thread(pid) : get_cur_thread();
-
-    if (!thread)
-        return -ESRCH;
-
-    return thread->pgid;
+    return shim_do_getpgid(pid);
 }

LibOS/shim/src/sys/shim_ioctl.c

@@ -300,6 +300,7 @@ void signal_io(IDTYPE target, void* arg) {
     lock(&thread->lock);
     append_signal(thread, SIGIO, NULL, true);
     unlock(&thread->lock);
+    put_thread(thread);
 }
 
 int shim_do_ioctl(int fd, int cmd, unsigned long arg) {

LibOS/shim/src/sys/shim_sigaction.c

@@ -572,34 +572,33 @@ int shim_do_kill (pid_t pid, int sig)
     return ret < 0 ? ret : 0;
 }
 
-int do_kill_thread (IDTYPE sender, IDTYPE tgid, IDTYPE tid, int sig,
-                    bool use_ipc)
-{
+int do_kill_thread(IDTYPE sender, IDTYPE tgid, IDTYPE tid, int sig, bool use_ipc) {
     if (sig < 0 || sig > NUM_SIGS)
         return -EINVAL;
 
-    struct shim_thread * thread = lookup_thread(tid);
-    int ret = 0;
+    struct shim_thread* thread = lookup_thread(tid);
+    int ret = -ESRCH;
 
     if (thread) {
         lock(&thread->lock);
 
         if (thread->in_vm) {
-            if (!tgid || thread->tgid == tgid)
+            if (!tgid || thread->tgid == tgid) {
                 __append_signal(thread, sig, sender);
-            else
-                ret = -ESRCH;
+                ret = 0;
+            }
+            use_ipc = false;
         } else {
-            unlock(&thread->lock);
-            return ipc_pid_kill_send(sender, tid, KILL_THREAD, sig);
+            use_ipc = true;
         }
 
         unlock(&thread->lock);
-        return ret;
+        put_thread(thread);
     }
 
-    if (!use_ipc)
-        return -ESRCH;
+    if (!use_ipc) {
+        return ret;
+    }
 
     return ipc_pid_kill_send(sender, tid, KILL_THREAD, sig);
 }

LibOS/shim/src/sys/shim_wait.c

@@ -71,12 +71,12 @@ pid_t shim_do_wait4(pid_t pid, int* status, int option, struct __kernel_rusage*
             lock(&parent->lock);
             /* DEP 5/15/17: These threads are exited */
             assert(!thread->is_alive);
-            LISTP_DEL_INIT(thread, &thread->parent->exited_children, siblings);
+            LISTP_DEL_INIT(thread, &parent->exited_children, siblings);
             unlock(&parent->lock);
 
             put_thread(parent);
-            put_thread(thread);
             thread->parent = NULL;
+            put_thread(thread);
         }
 
         unlock(&thread->lock);