Non-SFI mode: Add Linux asynchronous signal support

tests/nonsfi/user_async_signal_test.cc

Index: tests/nonsfi/user_async_signal_test.cc
diff --git a/tests/nonsfi/user_async_signal_test.cc b/tests/nonsfi/user_async_signal_test.cc
new file mode 100644
index 0000000000000000000000000000000000000000..8f5fa893588946d70d06ef988a35722aa2e867fb
--- /dev/null
+++ b/tests/nonsfi/user_async_signal_test.cc
@@ -0,0 +1,292 @@
+/*
+ * Copyright (c) 2015 The Native Client Authors. All rights reserved.
+ * Use of this source code is governed by a BSD-style license that can be
+ * found in the LICENSE file.
+ */
+
+#include <pthread.h>
+#include <semaphore.h>
+
+#include "native_client/src/include/nacl_assert.h"
+#include "native_client/src/nonsfi/linux/irt_signal_handling.h"

[Mark Seaborn, 2015/08/12 01:43:08]

Shouldn't be needed now -- this is an IRT-internal header.

[Luis Héctor Chávez, 2015/08/12 22:14:27]

Done.

+#include "native_client/src/public/nonsfi/irt_signal_handling.h"

[Mark Seaborn, 2015/08/12 01:43:08]

Also shouldn't be needed now -- this is an IRT-internal header.

[Luis Héctor Chávez, 2015/08/12 22:14:27]

Done.

+#include "native_client/src/untrusted/irt/irt.h"
+#include "native_client/src/untrusted/nacl/nacl_irt.h"

[Mark Seaborn, 2015/08/12 01:43:08]

Not needed now?

[Luis Héctor Chávez, 2015/08/12 22:14:27]

This is used for __libnacl_irt_thread. We cannot use a local copy since we are
using the thread_create wrapper.

+#include "native_client/src/untrusted/nacl/nacl_thread.h"
+#include "native_client/src/untrusted/pthread/pthread_types.h"

[Mark Seaborn, 2015/08/12 01:43:08]

Not needed now?

[Luis Héctor Chávez, 2015/08/12 22:14:27]

Done.

+
+#define CHECK_OK(expr) ASSERT_EQ(expr, 0)
+
+struct nacl_irt_thread_v0_2 __libnacl_irt_thread_v0_2;

[Mark Seaborn, 2015/08/12 01:43:08]

You shouldn't need the "__" prefix here (or in the next decl).

Can you also put all these definitions in a top-level anon namespace (which
would replace uses of "static")?  Then we get warnings about unused decls.

[Luis Héctor Chávez, 2015/08/12 22:14:27]

Done.

+struct nacl_irt_async_signal_handling __libnacl_irt_async_signal_handling;
+
+volatile int g_signal_count;
+volatile int g_signal_arrived;
+volatile int g_test_running;
+static nacl_irt_tid_t g_child_tid;
+void *g_expected_tls;
+sem_t g_sem;
+
+int thread_create_wrapper(void (*start_func)(void), void *stack,
+                          void *thread_ptr) {
+  return __libnacl_irt_thread_v0_2.thread_create(start_func, stack, thread_ptr,
+                                                 &g_child_tid);
+}
+
+int set_async_signal_handler(NaClIrtSignalHandler handler) {
+  return __libnacl_irt_async_signal_handling.set_async_signal_handler(handler);
+}
+
+int send_async_signal(nacl_irt_tid_t tid) {
+  return __libnacl_irt_async_signal_handling.send_async_signal(tid);
+}
+
+/*
+ * Check that sending a signal before initializing signal support will result in
+ * an error.
+ */
+void test_send_signal_before_init() {

[Mark Seaborn, 2015/08/12 01:43:08]

How about "before_set_handler", since there is no public "init" function?

[Luis Héctor Chávez, 2015/08/12 22:14:27]

Done.

+  int retval = send_async_signal(0);
+  ASSERT_EQ(retval, ESRCH);
+}
+
+/*
+ * Check that nacl_tls_get() is async-signal-safe.
+ */
+void tls_get_signal_handler(NaClExceptionContext *exc) {
+  if (!g_test_running)
+    return;
+  ASSERT_EQ(nacl_tls_get(), g_expected_tls);
+  g_signal_count++;
+  g_signal_arrived = 1;
+}
+
+void *tls_get_thread_func(void *arg) {
+  g_expected_tls = nacl_tls_get();
+  CHECK_OK(sem_post(&g_sem));
+  while (g_test_running) {
+    ASSERT_EQ(nacl_tls_get(), g_expected_tls);
+    if (__sync_bool_compare_and_swap(&g_signal_arrived, 1, 0)) {
+      CHECK_OK(sem_post(&g_sem));
+    }
+  }
+  return NULL;
+}
+
+void test_async_safe_tls_get() {
+  CHECK_OK(sem_init(&g_sem, 0, 0));
+  CHECK_OK(set_async_signal_handler(tls_get_signal_handler));
+
+  pthread_t tid;
+  g_signal_count = 0;
+  g_signal_arrived = 0;
+  g_test_running = true;
+  CHECK_OK(pthread_create(&tid, NULL, tls_get_thread_func, NULL));
+
+  CHECK_OK(sem_wait(&g_sem));
+  const int kSignalCount = 1000;
+  for (int i = 0; i < kSignalCount; i++) {
+    CHECK_OK(send_async_signal(g_child_tid));
+    CHECK_OK(sem_wait(&g_sem));
+  }
+  g_test_running = false;
+  /* Send a last signal to make sure any waiting syscalls get interrupted. */
+  CHECK_OK(send_async_signal(g_child_tid));
+  CHECK_OK(pthread_join(tid, NULL));
+  ASSERT_EQ(g_signal_count, kSignalCount);
+  CHECK_OK(sem_destroy(&g_sem));
+}
+
+/*
+ * Check that both futex_wake() and futex_wait_abs() are signal-async-safe.
+ */
+void futex_signal_handler(NaClExceptionContext *exc) {
+  int count = 0;
+  ASSERT_EQ(__sync_bool_compare_and_swap(&g_signal_arrived, 0, 1), 1);
+  CHECK_OK(__libnacl_irt_futex.futex_wake(&g_signal_arrived, INT_MAX, &count));
+  /*
+   * |count| is always 0 since the thread waiting is now running the signal
+   * handler, so it did not actually counts as a wakeup.

[Mark Seaborn, 2015/08/12 01:43:08]

"count"

[Luis Héctor Chávez, 2015/08/12 22:14:27]

Done.

[Mark Seaborn, 2015/08/13 21:17:50]

Er, I didn't mean you should replace '|count|' with '"count"'.  The former was
fine.

I put the comment on the wrong line.  It should have been the next line...

+   */
+  ASSERT_EQ(count, 0);
+  if (g_test_running)
+    g_signal_count++;
+}
+
+void *futex_thread_func(void *arg) {
+  CHECK_OK(sem_post(&g_sem));
+  struct timespec timeout;
+  /*
+   * Make the timeout be the current time plus 10 seconds. This timeout should
+   * never kick in, but if it does it means we deadlocked, so it's better to
+   * assert than letting the job itself time out.
+   */
+  clock_gettime(CLOCK_REALTIME, &timeout);
+  timeout.tv_sec += 10;
+  while (g_test_running) {
+    int retval = __libnacl_irt_futex.futex_wait_abs(&g_signal_arrived, 0,
+                                                    &timeout);
+    if (retval == EWOULDBLOCK) {
+      /*
+       * The signal handler executed before we could wait and changed the value
+       * of |g_signal_arrived|.
+       */
+    } else {
+      ASSERT_EQ(retval, EINTR);

[Mark Seaborn, 2015/08/12 01:43:08]

Is the current implementation giving us EINTR?  I don't think we want that.

The idea is that user code should not have to retry syscalls.  That's a common
source of bugs, when programmers don't put a retry loop around a syscall that
can be interrupted.

Should we be registering signal handlers with SA_RESTART?

[Luis Héctor Chávez, 2015/08/12 22:14:27]

It appears that FUTEX_WAIT with a timeout does return EINTR when interrupted,
even with SA_RESTART (could not find docs that mention this, but verified
experimentally). With no timeout, SA_RESTART does what's expected.

I'll add a comment to clarify and will add SA_RESTART to the sigaction flags.

[Mark Seaborn, 2015/08/17 23:55:56]

OK.  Are there any test cases in this file that do cover a syscall being
restarted automatically?  If not, could you add one?  e.g. You could cover
futex_wait() without a timeout.

[Luis Héctor Chávez, 2015/08/18 02:53:31]

Done. There is one small caveat: the test will still need a retry loop since
apparently at some point, Linux started allowing spurious wakeups with
futex_wait() also when it returns 0. Here is the thread in LKML where they are
trying to update the documentation to reflect this:

http://thread.gmane.org/gmane.linux.kernel/1919555/focus=9795

The important snippet:

RETURN VALUE
       [...]

       FUTEX_WAIT
              Returns 0 if the caller was woken up.  Note that a  wake-up  can
              also  be caused by common futex usage patterns in unrelated code
              that happened to have previously used the  futex  word's  memory
              location  (e.g., typical futex-based implementations of Pthreads
              mutexes can cause this under some conditions).  Therefore, call‐
              ers should always conservatively assume that a return value of 0
              can mean a spurious wake-up, and  use  the  futex  word's  value
              (i.e., the user space synchronization scheme)
                  to decide whether to continue to block or not.

+    }
+    ASSERT_EQ(__sync_bool_compare_and_swap(&g_signal_arrived, 1, 0), 1);
+    /*
+     * Have to test again since we could have gone sleeping again after the last
+     * iteration.
+     */
+    if (g_test_running)
+      CHECK_OK(sem_post(&g_sem));
+  }
+  return NULL;
+}
+
+void test_async_safe_futex() {
+  CHECK_OK(sem_init(&g_sem, 0, 0));
+  CHECK_OK(set_async_signal_handler(futex_signal_handler));
+
+  pthread_t tid;
+  g_signal_count = 0;
+  g_signal_arrived = 0;
+  g_test_running = true;
+  CHECK_OK(pthread_create(&tid, NULL, futex_thread_func, NULL));
+
+  CHECK_OK(sem_wait(&g_sem));
+  const int kSignalCount = 1000;
+  for (int i = 0; i < kSignalCount; i++) {
+    CHECK_OK(send_async_signal(g_child_tid));
+    CHECK_OK(sem_wait(&g_sem));
+  }
+  g_test_running = false;
+  /* Send a last signal to make sure any waiting syscalls get interrupted. */
+  CHECK_OK(send_async_signal(g_child_tid));
+  CHECK_OK(pthread_join(tid, NULL));
+  ASSERT_EQ(g_signal_count, kSignalCount);
+  CHECK_OK(sem_destroy(&g_sem));
+}
+
+/*
+ * Check that send_async_signal() is async-signal-safe.
+ */
+void signal_signal_handler(NaClExceptionContext *exc) {
+  if (!g_test_running)
+    return;
+  if (++g_signal_count % 2 == 1) {
+    CHECK_OK(send_async_signal(g_child_tid));
+    g_signal_arrived = 1;
+  }
+}
+
+void *signal_thread_func(void *arg) {
+  CHECK_OK(sem_post(&g_sem));
+  struct timespec req, rem;
+  /*
+   * In case we are unlucky and the signal arrives before the first sleep, limit
+   * the time sleeping to 10 msec.
+   */
+  req.tv_sec = 0;
+  req.tv_nsec = 10000000;
+  while (g_test_running) {
+    while (g_test_running && !g_signal_arrived) {
+      int retval = nanosleep(&req, &rem);
+      if (retval != 0)
+        ASSERT_EQ(errno, EINTR);
+    }
+    /*
+     * Have to test again since we could have gone sleeping again after the last
+     * iteration.
+     */
+    if (!g_test_running)
+      break;
+    g_signal_arrived = 0;
+    CHECK_OK(sem_post(&g_sem));
+  }
+  return NULL;
+}
+
+void test_async_safe_signal() {
+  CHECK_OK(sem_init(&g_sem, 0, 0));
+  CHECK_OK(set_async_signal_handler(signal_signal_handler));
+
+  pthread_t tid;
+  g_test_running = true;
+  g_signal_count = 0;
+  g_signal_arrived = 0;
+  CHECK_OK(pthread_create(&tid, NULL, signal_thread_func, NULL));
+
+  CHECK_OK(sem_wait(&g_sem));
+  const int kSignalCount = 1000;
+  for (int i = 0; i < kSignalCount; i++) {
+    CHECK_OK(send_async_signal(g_child_tid));
+    CHECK_OK(sem_wait(&g_sem));
+  }
+  g_test_running = false;
+  /* Send a last signal to make sure any waiting syscalls get interrupted. */
+  CHECK_OK(send_async_signal(g_child_tid));
+  CHECK_OK(pthread_join(tid, NULL));
+  ASSERT_EQ(g_signal_count, 2 * kSignalCount);
+  CHECK_OK(sem_destroy(&g_sem));
+}
+
+/*
+ * Check that passing 0 as |tid| to send_async_signal() works and
+ * sends a signal to the main thread.
+ */
+void main_signal_handler(NaClExceptionContext *exc) {
+  g_signal_count = 1;
+}
+
+void test_main_signal() {
+  CHECK_OK(set_async_signal_handler(main_signal_handler));
+
+  g_signal_count = 0;
+  CHECK_OK(send_async_signal(NACL_IRT_MAIN_THREAD_TID));
+  ASSERT_EQ(g_signal_count, 1);
+}
+
+void run_test(const char *test_name, void (*test_func)(void)) {
+  printf("Running %s...\n", test_name);
+  test_func();
+}
+
+#define RUN_TEST(test_func) (run_test(#test_func, test_func))
+
+int main(void) {
+  size_t bytes;
+  bytes = nacl_interface_query(NACL_IRT_THREAD_v0_2, &__libnacl_irt_thread_v0_2,
+                               sizeof(__libnacl_irt_thread_v0_2));
+  ASSERT_EQ(bytes, sizeof(__libnacl_irt_thread_v0_2));
+
+  bytes = nacl_interface_query(NACL_IRT_ASYNC_SIGNAL_HANDLING_v0_1,
+                               &__libnacl_irt_async_signal_handling,
+                               sizeof(__libnacl_irt_async_signal_handling));
+  ASSERT_EQ(bytes, sizeof(__libnacl_irt_async_signal_handling));
+
+  /*
+   * In order to avoid modifying the libpthread implementation to save the
+   * native tid, wrap that functionality so the tid is stored in a global
+   * variable.
+   */
+  __libnacl_irt_thread.thread_create = &thread_create_wrapper;
+
+  RUN_TEST(test_send_signal_before_init);
+
+  RUN_TEST(test_async_safe_tls_get);
+#if !defined(__arm__)
+  /*
+   * Signals are sometimes delivered after the futex_wait syscall returns (as
+   * opposed to interrupting it), which breaks this test.

[Mark Seaborn, 2015/08/12 01:43:08]

Do you mean the futex_wait syscall doesn't get interrupted at all?

Does this problem only occur under QEMU?  If so, can you say that in the
comment, please?

[Luis Héctor Chávez, 2015/08/12 22:14:27]

Yes, futex_wait_abs returns ETIMEDOUT and the signal is delivered afterwards. I
also ran the test on real ARM hardware and does not seem to happen. Amended the
comment.

+   */
+  RUN_TEST(test_async_safe_futex);
+#endif
+  RUN_TEST(test_async_safe_signal);
+  RUN_TEST(test_main_signal);
+
+  printf("Done\n");
+
+  return 0;
+}