[LibOS] Cleanup of shim_thread::shim_signal_logs

- Make struct shim_signal_log opaque.
- Introduce signal_logs_alloc() and signal_logs_free() to properly
  initialize and free signal_logs.
- Introduce helper signal_logs_pending() to simplify signal_logs check.
- Rename shim_signal_log's {head, tail} to {tail, head}.
- Fix PID leak during get_new_thread().

LibOS/shim/include/shim_signal.h

@@ -98,12 +98,10 @@ struct shim_signal {
     PAL_CONTEXT * pal_context;
 };
 
-#define MAX_SIGNAL_LOG      32
-
-struct shim_signal_log {
-    struct atomic_int head, tail;
-    struct shim_signal * logs[MAX_SIGNAL_LOG];
-};
+struct shim_signal_log;
+struct shim_signal_log* signal_logs_alloc(void);
+void signal_logs_free(struct shim_signal_log* signal_log);
+bool signal_logs_pending(const struct shim_signal_log* signal_log, int sig);
 
 extern const char * const siglist[NUM_KNOWN_SIGS + 1];
 

LibOS/shim/include/shim_thread.h

@@ -192,9 +192,10 @@ void set_cur_thread (struct shim_thread * thread)
         thread->shim_tcb = tcb;
         tid = thread->tid;
 
-        if (!is_internal(thread) && !thread->signal_logs)
-            thread->signal_logs = malloc(sizeof(struct shim_signal_log) *
-                                         NUM_SIGS);
+        if (!is_internal(thread) && !thread->signal_logs) {
+            thread->signal_logs = signal_logs_alloc();
+            assert(thread->signal_logs); /* FIXME on ENOMEM */
+        }
     } else if (tcb->tp) {
         put_thread(tcb->tp);
         tcb->tp = NULL;

LibOS/shim/src/bookkeep/shim_signal.c

@@ -42,6 +42,54 @@ typedef void (*__rt_sighandler_t)(int, siginfo_t*, void*);
 
 static __rt_sighandler_t default_sighandler[NUM_SIGS];
 
+#define MAX_SIGNAL_LOG 32
+
+struct shim_signal_log {
+    /* FIXME: This whole structure needs a rewrite, it can't be implemented correctly lock-free. */
+    /*
+     * ring buffer of pending same-type signals (e.g. all pending SIGINTs).
+     * [tail, head) for used area (with wrap around)
+     * [head, tail) for free area (with wrap around)
+     */
+    struct atomic_int head;
+    struct atomic_int tail;
+    struct shim_signal* logs[MAX_SIGNAL_LOG];
+};
+
+struct shim_signal_log* signal_logs_alloc(void) {
+    struct shim_signal_log* signal_logs = malloc(sizeof(*signal_logs) * NUM_SIGS);
+    if (!signal_logs)
+        return NULL;
+    for (int sig = 0; sig < NUM_SIGS; sig++) {
+        atomic_set(&signal_logs[sig].head, 0);
+        atomic_set(&signal_logs[sig].tail, 0);
+    }
+    return signal_logs;
+}
+
+void signal_logs_free(struct shim_signal_log* signal_logs) {
+    for (int sig = 0; sig < NUM_SIGS; sig++) {
+        struct shim_signal_log* log = &signal_logs[sig];
+        int tail = atomic_read(&log->tail);
+        int head = atomic_read(&log->head);
+        if (head < tail) {
+            for (int i = tail; i < MAX_SIGNAL_LOG; i++) {
+                free(log->logs[i]);
+            }
+            tail = 0;
+        }
+        for (int i = tail; i < head; i++) {
+            free(log->logs[i]);
+        }
+    }
+    free(signal_logs);
+}
+
+bool signal_logs_pending(const struct shim_signal_log* signal_log, int sig) {
+    /* FIXME: race condition between reading two atomic variables */
+    return atomic_read(&signal_log[sig - 1].tail) != atomic_read(&signal_log[sig - 1].head);
+}
+
 static struct shim_signal **
 allocate_signal_log (struct shim_thread * thread, int sig)
 {
@@ -49,24 +97,25 @@ allocate_signal_log (struct shim_thread * thread, int sig)
         return NULL;
 
     struct shim_signal_log * log = &thread->signal_logs[sig - 1];
-    int head, tail, old_tail;
+    int tail, head, old_head;
 
+    /* FIXME: race condition between allocating the slot and populating the slot. */
     do {
-        head = atomic_read(&log->head);
-        old_tail = tail = atomic_read(&log->tail);
+        tail = atomic_read(&log->tail);
+        old_head = head = atomic_read(&log->head);
 
-        if (head == tail + 1 || (!head && tail == (MAX_SIGNAL_LOG - 1)))
+        if (tail == head + 1 || (!tail && head == (MAX_SIGNAL_LOG - 1)))
             return NULL;
 
-        tail = (tail == MAX_SIGNAL_LOG - 1) ? 0 : tail + 1;
-    } while (atomic_cmpxchg(&log->tail, old_tail, tail) == tail);
+        head = (head == MAX_SIGNAL_LOG - 1) ? 0 : head + 1;
+    } while (atomic_cmpxchg(&log->head, old_head, head) == head);
 
-    debug("signal_logs[%d]: head=%d, tail=%d (counter = %ld)\n", sig - 1,
-          head, tail, thread->has_signal.counter + 1);
+    debug("signal_logs[%d]: tail=%d, head=%d (counter = %ld)\n", sig - 1,
+          tail, head, thread->has_signal.counter + 1);
 
     atomic_inc(&thread->has_signal);
 
-    return &log->logs[old_tail];
+    return &log->logs[old_head];
 }
 
 static struct shim_signal *
@@ -74,28 +123,29 @@ fetch_signal_log (struct shim_thread * thread, int sig)
 {
     struct shim_signal_log * log = &thread->signal_logs[sig - 1];
     struct shim_signal * signal = NULL;
-    int head, tail, old_head;
+    int tail, head, old_tail;
 
+    /* FIXME: race condition between finding the slot and clearing the slot. */
     while (1) {
-        old_head = head = atomic_read(&log->head);
-        tail = atomic_read(&log->tail);
+        old_tail = tail = atomic_read(&log->tail);
+        head = atomic_read(&log->head);
 
-        if (head == tail)
+        if (tail == head)
             return NULL;
 
-        if (!(signal = log->logs[head]))
+        if (!(signal = log->logs[tail]))
             return NULL;
 
-        log->logs[head] = NULL;
-        head = (head == MAX_SIGNAL_LOG - 1) ? 0 : head + 1;
+        log->logs[tail] = NULL;
+        tail = (tail == MAX_SIGNAL_LOG - 1) ? 0 : tail + 1;
 
-        if (atomic_cmpxchg(&log->head, old_head, head) == old_head)
+        if (atomic_cmpxchg(&log->tail, old_tail, tail) == old_tail)
             break;
 
-        log->logs[old_head] = signal;
+        log->logs[old_tail] = signal;
     }
 
-    debug("signal_logs[%d]: head=%d, tail=%d\n", sig -1, head, tail);
+    debug("signal_logs[%d]: tail=%d, head=%d\n", sig -1, tail, head);
 
     atomic_dec(&thread->has_signal);
 

LibOS/shim/src/bookkeep/shim_thread.c

@@ -101,8 +101,7 @@ struct shim_thread* lookup_thread(IDTYPE tid) {
     return thread;
 }
 
-IDTYPE get_pid (void)
-{
+static IDTYPE get_pid(void) {
     IDTYPE idx;
 
     while (1) {
@@ -168,8 +167,18 @@ struct shim_thread * get_new_thread (IDTYPE new_tid)
     }
 
     struct shim_thread * thread = alloc_new_thread();
-    if (!thread)
+    if (!thread) {
+        release_pid(new_tid);
+        return NULL;
+    }
+
+    thread->signal_logs = signal_logs_alloc();
+    if (!thread->signal_logs) {
+        free(thread);
+        release_pid(new_tid);
         return NULL;
+    }
+
 
     struct shim_thread * cur_thread = get_cur_thread();
     thread->tid = new_tid;
@@ -228,22 +237,18 @@ struct shim_thread * get_new_thread (IDTYPE new_tid)
         }
     }
 
-    thread->vmid = cur_process.vmid;
-    thread->signal_logs = malloc(sizeof(struct shim_signal_log) *
-                                 NUM_SIGS);
-    if (!thread->signal_logs) {
-        goto out_error;
-    }
     if (!create_lock(&thread->lock)) {
         goto out_error;
     }
+
+    thread->vmid = cur_process.vmid;
     thread->scheduler_event = DkNotificationEventCreate(PAL_TRUE);
     thread->exit_event = DkNotificationEventCreate(PAL_FALSE);
     thread->child_exit_event = DkNotificationEventCreate(PAL_FALSE);
     return thread;
 
 out_error:
-    free(thread->signal_logs);
+    signal_logs_free(thread->signal_logs);
     if (thread->handle_map) {
         put_handle_map(thread->handle_map);
     }
@@ -370,7 +375,7 @@ void put_thread (struct shim_thread * thread)
             destroy_lock(&thread->lock);
         }
 
-        free(thread->signal_logs);
+        signal_logs_free(thread->signal_logs);
         free(thread);
     }
 }
@@ -846,8 +851,9 @@ BEGIN_RS_FUNC(running_thread)
         thread->set_child_tid = NULL;
     }
 
-    thread->signal_logs = malloc(sizeof(struct shim_signal_log) *
-                                 NUM_SIGS);
+    thread->signal_logs = signal_logs_alloc();
+    if (!thread->signal_logs)
+        return -ENOMEM;
 
     if (cur_thread) {
         PAL_HANDLE handle = DkThreadCreate(resume_wrapper, thread);

LibOS/shim/src/sys/shim_sigaction.c

@@ -202,8 +202,7 @@ int shim_do_sigsuspend(const __sigset_t* mask) {
 
     /* return immediately on some pending unblocked signal */
     for (int sig = 1; sig <= NUM_SIGS; sig++) {
-        if (atomic_read(&cur->signal_logs[sig - 1].head) !=
-            atomic_read(&cur->signal_logs[sig - 1].tail)) {
+        if (signal_logs_pending(cur->signal_logs, sig)) {
             /* at least one signal of type sig... */
             if (!__sigismember(mask, sig)) {
                 /* ...and this type is not blocked in supplied mask */
@@ -245,8 +244,7 @@ int shim_do_sigpending(__sigset_t* set, size_t sigsetsize) {
         return 0;
 
     for (int sig = 1; sig <= NUM_SIGS; sig++) {
-        if (atomic_read(&cur->signal_logs[sig - 1].head) !=
-            atomic_read(&cur->signal_logs[sig - 1].tail))
+        if (signal_logs_pending(cur->signal_logs, sig))
             __sigaddset(set, sig);
     }
 

LibOS/shim/src/sys/shim_sleep.c

@@ -44,8 +44,7 @@ static bool signal_pending(void) {
     }
 
     for (int sig = 1; sig <= NUM_SIGS; sig++) {
-        if (atomic_read(&cur->signal_logs[sig - 1].head) !=
-            atomic_read(&cur->signal_logs[sig - 1].tail)) {
+        if (signal_logs_pending(cur->signal_logs, sig)) {
             /* at least one signal of type sig... */
             if (!__sigismember(&cur->signal_mask, sig)) {
                 /* ...and this type is not blocked  */