[Qemu-devel] [PATCH 18/34] linux-user: Fix race between multiple signals

[Timothy E Baldwin]

If multiple host signals are recieved in quick succession they would
be queued in TaskState then delivered to the guest in spite of
signals being blocked. Fixed by keeping host signals blocked until
process_pending_signals() runs, this needs the guest signal state
to be maintained by Qemu.

Blocking host signals also ensures the correct behaviour with respect
to multiple threads and the overrun count of timer related signals.
Alas blocking and queuing in qemu is still needed because of virtual
processor exceptions, SIGSEGV and SIGBUS.

Blocking signals inside process_pending_signals() protects against
concurrency problems with respect to signal handlers.

Signed-off-by: Timothy Edward Baldwin <address@hidden>

Conflicts:
        linux-user/qemu.h
---
 linux-user/qemu.h    |   8 +++-
 linux-user/signal.c  | 111 ++++++++++++++++++++++++++++++++++-----------------
 linux-user/syscall.c |  49 +++++++++++++++--------
 3 files changed, 113 insertions(+), 55 deletions(-)

diff --git a/linux-user/qemu.h b/linux-user/qemu.h
index 57c7e43..f2235eb3 100644
--- a/linux-user/qemu.h
+++ b/linux-user/qemu.h
@@ -127,14 +127,17 @@ typedef struct TaskState {
 #endif
     uint32_t stack_base;
     int used; /* non zero if used */
-    bool sigsegv_blocked; /* SIGSEGV blocked by guest */
     struct image_info *info;
     struct linux_binprm *bprm;
 
     struct emulated_sigtable sigtab[TARGET_NSIG];
     struct sigqueue sigqueue_table[MAX_SIGQUEUE_SIZE]; /* siginfo queue */
     struct sigqueue *first_free; /* first free siginfo queue entry */
-    int signal_pending; /* non zero if a signal may be pending */
+    sigset_t signal_mask;
+
+    /* non zero if host signals blocked, bit 1 set if signal pending */
+    volatile int signal_pending;
+
 } __attribute__((aligned(16))) TaskState;
 
 extern char *exec_path;
@@ -255,6 +258,7 @@ long do_sigreturn(CPUArchState *env);
 long do_rt_sigreturn(CPUArchState *env);
 abi_long do_sigaltstack(abi_ulong uss_addr, abi_ulong uoss_addr, abi_ulong sp);
 int do_sigprocmask(int how, const sigset_t *set, sigset_t *oldset);
+int block_signals(void); /* Returns non zero if signal pending */
 
 #ifdef TARGET_I386
 /* vm86.c */
diff --git a/linux-user/signal.c b/linux-user/signal.c
index 09551ba..3e272a5 100644
--- a/linux-user/signal.c
+++ b/linux-user/signal.c
@@ -197,53 +197,66 @@ void target_to_host_old_sigset(sigset_t *sigset,
     target_to_host_sigset(sigset, &d);
 }
 
+int block_signals(void)
+{
+    TaskState *ts = (TaskState *)thread_cpu->opaque;
+    sigset_t set;
+    int pending;
+
+    sigfillset(&set);
+    sigdelset(&set, SIGSEGV);
+    sigdelset(&set, SIGBUS);
+    sigprocmask(SIG_SETMASK, &set, 0);
+
+    pending = ts->signal_pending;
+    pending |= 2;
+    ts->signal_pending = pending;
+
+#ifdef TARGET_USE_ERESTARTSYS
+    return pending & 1;
+#endif
+    return 0;
+}
+
 /* Wrapper for sigprocmask function
  * Emulates a sigprocmask in a safe way for the guest. Note that set and oldset
- * are host signal set, not guest ones. This wraps the sigprocmask host calls
- * that should be protected (calls originated from guest)
+ * are host signal set, not guest ones.
  */
 int do_sigprocmask(int how, const sigset_t *set, sigset_t *oldset)
 {
-    int ret;
-    sigset_t val;
-    sigset_t *temp = NULL;
-    CPUState *cpu = thread_cpu;
-    TaskState *ts = (TaskState *)cpu->opaque;
-    bool segv_was_blocked = ts->sigsegv_blocked;
+    TaskState *ts = (TaskState *)thread_cpu->opaque;
+
+    if (oldset) {
+        *oldset = ts->signal_mask;
+    }
 
     if (set) {
-        bool has_sigsegv = sigismember(set, SIGSEGV);
-        val = *set;
-        temp = &val;
+        int i;
 
-        sigdelset(temp, SIGSEGV);
+        if (block_signals()) {
+            return -TARGET_ERESTARTSYS;
+        }
 
         switch (how) {
         case SIG_BLOCK:
-            if (has_sigsegv) {
-                ts->sigsegv_blocked = true;
-            }
+            sigorset(&ts->signal_mask, &ts->signal_mask, set);
             break;
         case SIG_UNBLOCK:
-            if (has_sigsegv) {
-                ts->sigsegv_blocked = false;
+            for (i = 0; i != NSIG; ++i) {
+                if (sigismember(set, i)) {
+                    sigdelset(&ts->signal_mask, i);
+                }
             }
             break;
         case SIG_SETMASK:
-            ts->sigsegv_blocked = has_sigsegv;
+            ts->signal_mask = *set;
             break;
         default:
             g_assert_not_reached();
         }
-    }
-
-    ret = sigprocmask(how, temp, oldset);
 
-    if (oldset && segv_was_blocked) {
-        sigaddset(oldset, SIGSEGV);
     }
-
-    return ret;
+    return 0;
 }
 
 /* siginfo conversion */
@@ -374,6 +387,7 @@ static int core_dump_signal(int sig)
 
 void signal_init(void)
 {
+    TaskState *ts = (TaskState *)thread_cpu->opaque;
     struct sigaction act;
     struct sigaction oact;
     int i, j;
@@ -389,6 +403,9 @@ void signal_init(void)
         target_to_host_signal_table[j] = i;
     }
 
+    /* Set the signal mask from the host mask. */
+    sigprocmask(0, 0, &ts->signal_mask);
+
     /* set all host signal handlers. ALL signals are blocked during
        the handlers to serialize them. */
     memset(sigact_table, 0, sizeof(sigact_table));
@@ -508,7 +525,7 @@ int queue_signal(CPUArchState *env, int sig, 
target_siginfo_t *info)
     queue = gdb_queuesig ();
     handler = sigact_table[sig - 1]._sa_handler;
 
-    if (ts->sigsegv_blocked && sig == TARGET_SIGSEGV) {
+    if (sig == TARGET_SIGSEGV && sigismember(&ts->signal_mask, SIGSEGV)) {
         /* Guest has blocked SIGSEGV but we got one anyway. Assume this
          * is a forced SIGSEGV (ie one the kernel handles via force_sig_info
          * because it got a real MMU fault). A blocked SIGSEGV in that
@@ -605,6 +622,11 @@ static void host_signal_handler(int host_signum, siginfo_t 
*info,
 
     host_to_target_siginfo_noswap(&tinfo, info);
     if (queue_signal(env, sig, &tinfo) == 1) {
+        /* Block host signals until target signal handler entered */
+        sigfillset(&uc->uc_sigmask);
+        sigdelset(&uc->uc_sigmask, SIGSEGV);
+        sigdelset(&uc->uc_sigmask, SIGBUS);
+
         /* interrupt the virtual CPU as soon as possible */
         cpu_exit(thread_cpu);
     }
@@ -5628,26 +5650,40 @@ void process_pending_signals(CPUArchState *cpu_env)
     CPUState *cpu = ENV_GET_CPU(cpu_env);
     int sig;
     abi_ulong handler;
-    sigset_t set, old_set;
+    sigset_t set;
     target_sigset_t target_old_set;
     struct emulated_sigtable *k;
     struct target_sigaction *sa;
     struct sigqueue *q;
     TaskState *ts = cpu->opaque;
 
-    if (!ts->signal_pending)
+restart:
+    if (!ts->signal_pending) {
         return;
+    }
 
     /* FIXME: This is not threadsafe.  */
+    sigfillset(&set);
+    sigprocmask(SIG_SETMASK, &set, 0);
+
+ next_signal:
     k = ts->sigtab;
     for(sig = 1; sig <= TARGET_NSIG; sig++) {
-        if (k->pending)
+        if (k->pending && (
+                    !sigismember(&ts->signal_mask, 
target_to_host_signal_table[sig])
+                    || sig == TARGET_SIGSEGV)) {
             goto handle_signal;
+        }
         k++;
     }
-    /* if no signal is pending, just return */
+
+    /* if no signal is pending, unblock signals and restart */
     ts->signal_pending = 0;
-    return;
+    set = ts->signal_mask;
+    sigdelset(&set, SIGSEGV);
+    sigdelset(&set, SIGBUS);
+    sigprocmask(SIG_SETMASK, &set, 0);
+    goto restart; /* Another signal? */
 
  handle_signal:
 #ifdef DEBUG_SIGNAL
@@ -5668,7 +5704,7 @@ void process_pending_signals(CPUArchState *cpu_env)
         handler = sa->_sa_handler;
     }
 
-    if (ts->sigsegv_blocked && sig == TARGET_SIGSEGV) {
+    if (sig == TARGET_SIGSEGV && sigismember(&ts->signal_mask, SIGSEGV)) {
         /* Guest has blocked SIGSEGV but we got one anyway. Assume this
          * is a forced SIGSEGV (ie one the kernel handles via force_sig_info
          * because it got a real MMU fault), and treat as if default handler.
@@ -5698,11 +5734,12 @@ void process_pending_signals(CPUArchState *cpu_env)
         if (!(sa->sa_flags & TARGET_SA_NODEFER))
             sigaddset(&set, target_to_host_signal(sig));
 
-        /* block signals in the handler using Linux */
-        do_sigprocmask(SIG_BLOCK, &set, &old_set);
         /* save the previous blocked signal state to restore it at the
            end of the signal execution (see do_sigreturn) */
-        host_to_target_sigset_internal(&target_old_set, &old_set);
+        host_to_target_sigset_internal(&target_old_set, &ts->signal_mask);
+
+        /* block signals in the handler */
+        sigorset(&ts->signal_mask, &ts->signal_mask, &set);
 
         /* if the CPU is in VM86 mode, we restore the 32 bit values */
 #if defined(TARGET_I386) && !defined(TARGET_X86_64)
@@ -5722,9 +5759,11 @@ void process_pending_signals(CPUArchState *cpu_env)
         else
             setup_frame(sig, sa, &target_old_set, cpu_env);
 #endif
-       if (sa->sa_flags & TARGET_SA_RESETHAND)
+        if (sa->sa_flags & TARGET_SA_RESETHAND)
             sa->_sa_handler = TARGET_SIG_DFL;
     }
     if (q != &k->info)
         free_sigqueue(cpu_env, q);
+
+    goto next_signal;
 }
diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 281fa2d..682090d 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -4676,6 +4676,7 @@ static int do_fork(CPUArchState *env, unsigned int flags, 
abi_ulong newsp,
         new_cpu->opaque = ts;
         ts->bprm = parent_ts->bprm;
         ts->info = parent_ts->info;
+        ts->signal_mask = parent_ts->signal_mask;
         nptl_flags = flags;
         flags &= ~CLONE_NPTL_FLAGS2;
 
@@ -6492,9 +6493,11 @@ abi_long do_syscall(void *cpu_env, int num, abi_long 
arg1,
         {
             sigset_t cur_set;
             abi_ulong target_set;
-            do_sigprocmask(0, NULL, &cur_set);
-            host_to_target_old_sigset(&target_set, &cur_set);
-            ret = target_set;
+            ret = do_sigprocmask(0, NULL, &cur_set);
+            if (!ret) {
+                host_to_target_old_sigset(&target_set, &cur_set);
+                ret = target_set;
+            }
         }
         break;
 #endif
@@ -6503,12 +6506,14 @@ abi_long do_syscall(void *cpu_env, int num, abi_long 
arg1,
         {
             sigset_t set, oset, cur_set;
             abi_ulong target_set = arg1;
-            do_sigprocmask(0, NULL, &cur_set);
-            target_to_host_old_sigset(&set, &target_set);
-            sigorset(&set, &set, &cur_set);
-            do_sigprocmask(SIG_SETMASK, &set, &oset);
-            host_to_target_old_sigset(&target_set, &oset);
-            ret = target_set;
+            ret = do_sigprocmask(0, NULL, &cur_set);
+            if (!ret) {
+                target_to_host_old_sigset(&set, &target_set);
+                sigorset(&set, &set, &cur_set);
+                do_sigprocmask(SIG_SETMASK, &set, &oset);
+                host_to_target_old_sigset(&target_set, &oset);
+                ret = target_set;
+            }
         }
         break;
 #endif
@@ -6537,7 +6542,7 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
             mask = arg2;
             target_to_host_old_sigset(&set, &mask);
 
-            ret = get_errno(do_sigprocmask(how, &set, &oldset));
+            ret = do_sigprocmask(how, &set, &oldset);
             if (!is_error(ret)) {
                 host_to_target_old_sigset(&mask, &oldset);
                 ret = mask;
@@ -6571,7 +6576,7 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
                 how = 0;
                 set_ptr = NULL;
             }
-            ret = get_errno(do_sigprocmask(how, set_ptr, &oldset));
+            ret = do_sigprocmask(how, set_ptr, &oldset);
             if (!is_error(ret) && arg3) {
                 if (!(p = lock_user(VERIFY_WRITE, arg3, 
sizeof(target_sigset_t), 0)))
                     goto efault;
@@ -6611,7 +6616,7 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
                 how = 0;
                 set_ptr = NULL;
             }
-            ret = get_errno(do_sigprocmask(how, set_ptr, &oldset));
+            ret = do_sigprocmask(how, set_ptr, &oldset);
             if (!is_error(ret) && arg3) {
                 if (!(p = lock_user(VERIFY_WRITE, arg3, 
sizeof(target_sigset_t), 0)))
                     goto efault;
@@ -6716,11 +6721,19 @@ abi_long do_syscall(void *cpu_env, int num, abi_long 
arg1,
         break;
 #ifdef TARGET_NR_sigreturn
     case TARGET_NR_sigreturn:
-        ret = do_sigreturn(cpu_env);
+        if (block_signals()) {
+            ret = -TARGET_ERESTARTSYS;
+        } else {
+            ret = do_sigreturn(cpu_env);
+        }
         break;
 #endif
     case TARGET_NR_rt_sigreturn:
-        ret = do_rt_sigreturn(cpu_env);
+        if (block_signals()) {
+            ret = -TARGET_ERESTARTSYS;
+        } else {
+            ret = do_rt_sigreturn(cpu_env);
+        }
         break;
     case TARGET_NR_sethostname:
         if (!(p = lock_user_string(arg1)))
@@ -8744,9 +8757,11 @@ abi_long do_syscall(void *cpu_env, int num, abi_long 
arg1,
             }
             mask = arg2;
             target_to_host_old_sigset(&set, &mask);
-            do_sigprocmask(how, &set, &oldset);
-            host_to_target_old_sigset(&mask, &oldset);
-            ret = mask;
+            ret = do_sigprocmask(how, &set, &oldset);
+            if (!ret) {
+                host_to_target_old_sigset(&mask, &oldset);
+                ret = mask;
+            }
         }
         break;
 #endif
-- 
2.1.4