i#2161, i#2270: ignore alarms during attach and detach (#2313)

Sets the signal handler for the 3 alarm signals to SIG_IGN during attach
and detach, to reduce problems with races while we try to coordinate taking
over or sending native all of the app threads.

This is not a panacea, as timer_create can send out any signal, not just
the 3 classic alarm signals, on timer expiration.  Plus, non-timer-related
signals could arrive during attach or detach.  However, this will help with
the most typical cases.

Adds an itimer to the api.static_signal test, though it is not easy to
reproduce these problems in small applications.

I'm considering this to fix the filed issues despite the above-mentioned
remaining corner cases as I'm considering those to be pathological:

Fixes #2161
Fixes #2270

core/dynamo.c

@@ -2645,7 +2645,8 @@ dr_app_cleanup(void)
      */
     tr = thread_lookup(get_thread_id());
     if (tr != NULL && tr->dcontext != NULL) {
-        os_process_under_dynamorio(tr->dcontext);
+        os_process_under_dynamorio_initiate(tr->dcontext);
+        os_process_under_dynamorio_complete(tr->dcontext);
         dynamo_thread_under_dynamo(tr->dcontext);
     }
     return dynamorio_app_exit();
@@ -2764,7 +2765,7 @@ dynamorio_take_over_threads(dcontext_t *dcontext)
     bool found_threads;
     uint attempts = 0;
 
-    os_process_under_dynamorio(dcontext);
+    os_process_under_dynamorio_initiate(dcontext);
     /* XXX i#1305: we should suspend all the other threads for DR init to
      * satisfy the parts of the init process that assume there are no races.
      */
@@ -2774,6 +2775,7 @@ dynamorio_take_over_threads(dcontext_t *dcontext)
         if (found_threads && !bb_lock_start)
             bb_lock_start = true;
     } while (found_threads && attempts < MAX_TAKE_OVER_ATTEMPTS);
+    os_process_under_dynamorio_complete(dcontext);
 
     if (found_threads) {
         SYSLOG(SYSLOG_WARNING, INTERNAL_SYSLOG_WARNING,

core/os_shared.h

@@ -64,7 +64,8 @@ void os_thread_exit(dcontext_t *dcontext, bool other_thread);
 void os_thread_under_dynamo(dcontext_t *dcontext);
 /* must only be called for the executing thread */
 void os_thread_not_under_dynamo(dcontext_t *dcontext);
-void os_process_under_dynamorio(dcontext_t *dcontext);
+void os_process_under_dynamorio_initiate(dcontext_t *dcontext);
+void os_process_under_dynamorio_complete(dcontext_t *dcontext);
 void os_process_not_under_dynamorio(dcontext_t *dcontext);
 
 bool os_take_over_all_unknown_threads(dcontext_t *dcontext);

core/synch.c

@@ -1965,6 +1965,9 @@ detach_on_permanent_stack(bool internal, bool do_cleanup)
     /* Release the APC init lock and let any threads waiting there go native */
     LOG(GLOBAL, LOG_ALL, 1, "Detach : Releasing init_apc_go_native_pause\n");
     init_apc_go_native_pause = false;
+#else
+    /* i#2270: we ignore alarm signals during detach to reduce races. */
+    signal_remove_alarm_handlers(my_dcontext);
 #endif
 
     /* perform exit tasks that require full thread data structs */

core/unix/os.c

@@ -2539,14 +2539,22 @@ os_thread_not_under_dynamo(dcontext_t *dcontext)
 }
 
 void
-os_process_under_dynamorio(dcontext_t *dcontext)
+os_process_under_dynamorio_initiate(dcontext_t *dcontext)
 {
     LOG(GLOBAL, LOG_THREADS, 1, "process now under DR\n");
     /* We only support regular process-wide signal handlers for delayed takeover. */
-    signal_reinstate_handlers(dcontext);
+    /* i#2161: we ignore alarm signals during the attach process to avoid races. */
+    signal_reinstate_handlers(dcontext, true/*ignore alarm*/);
     hook_vsyscall(dcontext, false);
 }
 
+void
+os_process_under_dynamorio_complete(dcontext_t *dcontext)
+{
+    /* i#2161: only now do we un-ignore alarm signals. */
+    signal_reinstate_alarm_handlers(dcontext);
+}
+
 void
 os_process_not_under_dynamorio(dcontext_t *dcontext)
 {

core/unix/os_exports.h

@@ -1,5 +1,5 @@
 /* **********************************************************
- * Copyright (c) 2011-2016 Google, Inc.  All rights reserved.
+ * Copyright (c) 2011-2017 Google, Inc.  All rights reserved.
  * Copyright (c) 2000-2010 VMware, Inc.  All rights reserved.
  * **********************************************************/
 
@@ -409,10 +409,7 @@ void
 signal_fork_init(dcontext_t *dcontext);
 
 void
-signal_remove_handlers(dcontext_t *dcontext);
-
-void
-signal_reinstate_handlers(dcontext_t *dcontext);
+signal_remove_alarm_handlers(dcontext_t *dcontext);
 
 bool
 set_itimer_callback(dcontext_t *dcontext, int which, uint millisec,

core/unix/os_private.h

@@ -1,5 +1,5 @@
 /* **********************************************************
- * Copyright (c) 2011-2016 Google, Inc.  All rights reserved.
+ * Copyright (c) 2011-2017 Google, Inc.  All rights reserved.
  * Copyright (c) 2008-2010 VMware, Inc.  All rights reserved.
  * **********************************************************/
 
@@ -253,6 +253,9 @@ void signal_exit(void);
 void signal_thread_init(dcontext_t *dcontext);
 void signal_thread_exit(dcontext_t *dcontext, bool other_thread);
 bool is_thread_signal_info_initialized(dcontext_t *dcontext);
+void signal_remove_handlers(dcontext_t *dcontext);
+void signal_reinstate_handlers(dcontext_t *dcontext, bool ignore_alarm);
+void signal_reinstate_alarm_handlers(dcontext_t *dcontext);
 void handle_clone(dcontext_t *dcontext, uint flags);
 /* If returns false to skip the syscall, the result is in "result". */
 bool handle_sigaction(dcontext_t *dcontext, int sig,

core/unix/signal.c

@@ -365,6 +365,18 @@ set_default_signal_action(int sig)
     return (rc == 0);
 }
 
+static bool
+set_ignore_signal_action(int sig)
+{
+    kernel_sigaction_t act;
+    int rc;
+    memset(&act, 0, sizeof(act));
+    act.handler = (handler_t) SIG_IGN;
+    /* arm the signal */
+    rc = sigaction_syscall(sig, &act, NULL);
+    return (rc == 0);
+}
+
 /* We assume that signal handlers will be shared most of the time
  * (pthreads shares them)
  * Rather than start out with the handler table in local memory and then
@@ -765,7 +777,8 @@ signal_info_exit_sigaction(dcontext_t *dcontext, thread_sig_info_t *info,
             if (info->app_sigaction[i] != NULL) {
                 /* Restore to old handler, but not if exiting whole
                  * process: else may get itimer during cleanup, so we
-                 * set to SIG_IGN.  XXX: we need to handle for detach.
+                 * set to SIG_IGN.  We do this for detach in
+                 * signal_remove_alarm_handlers().
                  */
                 if (dynamo_exited && !doing_detach) {
                     info->app_sigaction[i]->handler = (handler_t) SIG_IGN;
@@ -1320,22 +1333,16 @@ set_our_handler_sigact(kernel_sigaction_t *act, int sig)
         "mask for our handler is "PFX" "PFX"\n", mask_sig[0], mask_sig[1]);
 }
 
-/* Set up master_signal_handler as the handler for signal "sig",
- * for the current thread.  Since we deal with kernel data structures
- * in our interception of system calls, we use them here as well,
- * to avoid having to translate to/from libc data structures.
- */
 static void
-intercept_signal(dcontext_t *dcontext, thread_sig_info_t *info, int sig)
+set_handler_and_record_app(dcontext_t *dcontext, thread_sig_info_t *info, int sig,
+                           kernel_sigaction_t *act)
 {
     int rc;
-    kernel_sigaction_t act;
     kernel_sigaction_t oldact;
     ASSERT(sig <= MAX_SIGNUM);
 
-    set_our_handler_sigact(&act, sig);
     /* arm the signal */
-    rc = sigaction_syscall(sig, &act, &oldact);
+    rc = sigaction_syscall(sig, act, &oldact);
     ASSERT(rc == 0
            /* Workaround for PR 223720, which was fixed in ESX4.0 but
             * is present in ESX3.5 and earlier: vmkernel treats
@@ -1377,11 +1384,45 @@ intercept_signal(dcontext_t *dcontext, thread_sig_info_t *info, int sig)
         }
 #endif
     }
-
     LOG(THREAD, LOG_ASYNCH, 3, "\twe intercept signal %d\n", sig);
     info->we_intercept[sig] = true;
 }
 
+/* Set up master_signal_handler as the handler for signal "sig",
+ * for the current thread.  Since we deal with kernel data structures
+ * in our interception of system calls, we use them here as well,
+ * to avoid having to translate to/from libc data structures.
+ */
+static void
+intercept_signal(dcontext_t *dcontext, thread_sig_info_t *info, int sig)
+{
+    kernel_sigaction_t act;
+    ASSERT(sig <= MAX_SIGNUM);
+    set_our_handler_sigact(&act, sig);
+    set_handler_and_record_app(dcontext, info, sig, &act);
+}
+
+static void
+intercept_signal_ignore_initially(dcontext_t *dcontext, thread_sig_info_t *info, int sig)
+{
+    kernel_sigaction_t act;
+    ASSERT(sig <= MAX_SIGNUM);
+    memset(&act, 0, sizeof(act));
+    act.handler = (handler_t) SIG_IGN;
+    set_handler_and_record_app(dcontext, info, sig, &act);
+}
+
+static void
+intercept_signal_no_longer_ignore(dcontext_t *dcontext, thread_sig_info_t *info, int sig)
+{
+    kernel_sigaction_t act;
+    int rc;
+    ASSERT(sig <= MAX_SIGNUM);
+    set_our_handler_sigact(&act, sig);
+    rc = sigaction_syscall(sig, &act, NULL);
+    ASSERT(rc == 0);
+}
+
 /* i#1921: For proper single-threaded native execution with re-takeover we need
  * to propagate signals.  For now we only support going completely native in
  * this thread but without a full detach, so we abandon our signal handlers w/o
@@ -1400,10 +1441,6 @@ signal_remove_handlers(dcontext_t *dcontext)
     kernel_sigemptyset(&act.mask);
     for (i = 1; i <= MAX_SIGNUM; i++) {
         if (info->app_sigaction[i] != NULL) {
-            /* restore to old handler, but not if exiting whole
-             * process: else may get itimer during cleanup, so we
-             * set to SIG_IGN (we'll have to fix once we impl detach)
-             */
             LOG(THREAD, LOG_ASYNCH, 2, "\trestoring "PFX" as handler for %d\n",
                 info->app_sigaction[i]->handler, i);
             sigaction_syscall(i, info->app_sigaction[i], NULL);
@@ -1415,22 +1452,54 @@ signal_remove_handlers(dcontext_t *dcontext)
     }
 }
 
+void
+signal_remove_alarm_handlers(dcontext_t *dcontext)
+{
+    thread_sig_info_t *info = (thread_sig_info_t *) dcontext->signal_field;
+    int i;
+    for (i = 1; i <= MAX_SIGNUM; i++) {
+        if (!info->we_intercept[i])
+            continue;
+        if (sig_is_alarm_signal(i)) {
+            set_ignore_signal_action(i);
+        }
+    }
+}
+
 /* We assume regular POSIX with handlers global to just one thread group in the
  * process.
  */
 void
-signal_reinstate_handlers(dcontext_t *dcontext)
+signal_reinstate_handlers(dcontext_t *dcontext, bool ignore_alarm)
 {
     thread_sig_info_t *info = (thread_sig_info_t *) dcontext->signal_field;
     int i;
     for (i = 1; i <= MAX_SIGNUM; i++) {
-        if (info->we_intercept[i]) {
+        if (!info->we_intercept[i])
+            continue;
+        if (sig_is_alarm_signal(i) && ignore_alarm) {
+            LOG(THREAD, LOG_ASYNCH, 2, "\tignoring %d initially\n", i);
+            intercept_signal_ignore_initially(dcontext, info, i);
+        } else {
             LOG(THREAD, LOG_ASYNCH, 2, "\trestoring DR handler for %d\n", i);
             intercept_signal(dcontext, info, i);
         }
     }
 }
 
+void
+signal_reinstate_alarm_handlers(dcontext_t *dcontext)
+{
+    thread_sig_info_t *info = (thread_sig_info_t *) dcontext->signal_field;
+    int i;
+    for (i = 1; i <= MAX_SIGNUM; i++) {
+        if (!info->we_intercept[i] || !sig_is_alarm_signal(i))
+            continue;
+        LOG(THREAD, LOG_ASYNCH, 2, "\trestoring DR handler for %d\n", i);
+        intercept_signal_no_longer_ignore(dcontext, info, i);
+    }
+}
+
 /**** system call handlers ***********************************************/
 
 /* FIXME: invalid pointer passed to kernel will currently show up

core/win32/os.c

@@ -1702,13 +1702,19 @@ os_thread_not_under_dynamo(dcontext_t *dcontext)
 }
 
 void
-os_process_under_dynamorio(dcontext_t *dcontext)
+os_process_under_dynamorio_initiate(dcontext_t *dcontext)
 {
     SELF_UNPROTECT_DATASEC(DATASEC_RARELY_PROT);
     init_apc_go_native = false;
     SELF_PROTECT_DATASEC(DATASEC_RARELY_PROT);
 }
 
+void
+os_process_under_dynamorio_complete(dcontext_t *dcontext)
+{
+    /* Nothing. */
+}
+
 void
 os_process_not_under_dynamorio(dcontext_t *dcontext)
 {

suite/tests/api/static_signal.c

@@ -1,5 +1,5 @@
 /* **********************************************************
- * Copyright (c) 2016 Google, Inc.  All rights reserved.
+ * Copyright (c) 2016-2017 Google, Inc.  All rights reserved.
  * **********************************************************/
 
 /*
@@ -43,6 +43,7 @@
 #include <signal.h>
 #include <stdlib.h>
 #include <setjmp.h>
+#include <sys/time.h> /* itimer */
 
 #define ALT_STACK_SIZE  (SIGSTKSZ*2)
 
@@ -64,6 +65,8 @@ signal_handler(int sig, siginfo_t *siginfo, ucontext_t *ucxt)
     } else if (sig == SIGSEGV) {
         print("Got SIGSEGV\n");
         SIGLONGJMP(mark, 1);
+    } else if (sig == SIGPROF) {
+        /* Do nothing. */
     } else {
         print("Got unexpected signal %d\n", sig);
     }
@@ -104,7 +107,7 @@ static void
 event_exit(void)
 {
     dr_fprintf(STDERR, "Saw %s bb events\n", num_bbs > 0 ? "some" : "no");
-    dr_fprintf(STDERR, "Saw %d signal(s)\n", num_signals);
+    dr_fprintf(STDERR, "Saw %s signals\n", num_signals > 2 ? ">2" : "<=2");
 }
 
 DR_EXPORT void
@@ -132,14 +135,33 @@ int
 main(int argc, const char *argv[])
 {
     pthread_t thread;
+    struct itimerval timer;
+    sigset_t mask;
+    int rc;
     intercept_signal(SIGUSR1, signal_handler, true/*sigstack*/);
     intercept_signal(SIGSEGV, signal_handler, true/*sigstack*/);
     thread_ready = create_cond_var();
     thread_exit = create_cond_var();
     got_signal = create_cond_var();
+
+    intercept_signal(SIGPROF, signal_handler, true/*sigstack*/);
+    timer.it_interval.tv_sec = 0;
+    timer.it_interval.tv_usec = 1000;
+    timer.it_value.tv_sec = 0;
+    timer.it_value.tv_usec = 1000;
+    rc = setitimer(ITIMER_PROF, &timer, NULL);
+    assert(rc == 0);
+
     pthread_create(&thread, NULL, thread_func, NULL);
     wait_cond_var(thread_ready);
 
+#if 0 /* FIXME i#2311: once fixed in DR we can enable this. */
+    /* Block SIGPROF in the main thread to better test races. */
+    sigemptyset(&mask);
+    sigaddset(&mask, SIGPROF);
+    sigprocmask(SIG_BLOCK, &mask, NULL);
+#endif
+
     print("Sending SIGUSR1 pre-DR-init\n");
     pthread_kill(thread, SIGUSR1);
     wait_cond_var(got_signal);

suite/tests/api/static_signal.expect

@@ -11,7 +11,7 @@ pre-raise SIGSEGV under DR
 Got SIGSEGV
 pre-DR stop
 Saw some bb events
-Saw 2 signal(s)
+Saw >2 signals
 Sending SIGUSR1 post-DR-stop
 Got SIGUSR1
 pre-raise SIGSEGV native