[Qemu-devel] [PATCH] Replace all setjmp()/longjmp() with sigsetjmp()/siglongjmp()

[Peter Maydell]

The setjmp() function doesn't specify whether signal masks are saved and
restored; on Linux they are not, but on BSD (including MacOSX) they are.
QEMU never wants to save and restore signal masks, because it uses threads,
and the signal-restoration may restore the whole process signal mask,
not just the mask for the thread which did the longjmp. In particular,
this resulted in a bug where ctrl-C was ignored on MacOSX because the
CPU thread did a longjmp which resulted in its signal mask being applied
to every thread, so that all threads had SIGINT and SIGTERM blocked.

The POSIX-sanctioned portable way to do a jump without affecting signal
masks is to use sigsetjmp() with a zero savemask parameter, so change
all uses of setjmp()/longjmp() accordingly.

For Windows we provide a trivial sigsetjmp/siglongjmp in terms of
setjmp/longjmp -- this is OK because no user will ever pass a non-zero
savemask (it would be a bug to do so because of the process-wide effects
described above).

The setjmp() uses in tests/tcg/test-i386.c and tests/tcg/linux-test.c
are left untouched because these are self-contained singlethreaded
test programs intended to be run under QEMU's Linux emulation, so they
have neither the portability nor the multithreading issues to deal with.

Signed-off-by: Peter Maydell <address@hidden>
---
This should have no visible effects on Linux where setjmp/longjmp
never messed with the signal state in the first place; it just makes
us behave right on the BSDs.

Stefan: I've put in the os-win32.h defines which I think are needed;
they're pretty trivial but I don't have a Win32 setup to compile with;
can I ask you to run a quick test for me, please?

I didn't feel it necessary to change every last comment that said
'longjmp' to say 'siglongjmp' where I felt the comment was talking
about 'longjmp the concept' rather than 'longjmp the specific
function implementation'.

checkpatch complains about the indent in the disassembler
sources, as usual, but is otherwise happy.

 coroutine-sigaltstack.c   | 26 +++++++++++++-------------
 coroutine-ucontext.c      | 27 ++++++++++++++-------------
 cpu-exec.c                |  6 +++---
 disas/i386.c              |  6 +++---
 disas/m68k.c              | 11 ++++++-----
 include/exec/cpu-defs.h   |  2 +-
 include/sysemu/os-win32.h |  8 ++++++++
 monitor.c                 |  6 +++---
 user-exec.c               |  2 +-
 9 files changed, 52 insertions(+), 42 deletions(-)

diff --git a/coroutine-sigaltstack.c b/coroutine-sigaltstack.c
index e37ebac..1fb41c9 100644
--- a/coroutine-sigaltstack.c
+++ b/coroutine-sigaltstack.c
@@ -45,7 +45,7 @@ static unsigned int pool_size;
 typedef struct {
     Coroutine base;
     void *stack;
-    jmp_buf env;
+    sigjmp_buf env;
 } CoroutineUContext;
 
 /**
@@ -59,7 +59,7 @@ typedef struct {
     CoroutineUContext leader;
 
     /** Information for the signal handler (trampoline) */
-    jmp_buf tr_reenter;
+    sigjmp_buf tr_reenter;
     volatile sig_atomic_t tr_called;
     void *tr_handler;
 } CoroutineThreadState;
@@ -115,8 +115,8 @@ static void __attribute__((constructor)) 
coroutine_init(void)
 static void coroutine_bootstrap(CoroutineUContext *self, Coroutine *co)
 {
     /* Initialize longjmp environment and switch back the caller */
-    if (!setjmp(self->env)) {
-        longjmp(*(jmp_buf *)co->entry_arg, 1);
+    if (!sigsetjmp(self->env, 0)) {
+        siglongjmp(*(sigjmp_buf *)co->entry_arg, 1);
     }
 
     while (true) {
@@ -145,14 +145,14 @@ static void coroutine_trampoline(int signal)
     /*
      * Here we have to do a bit of a ping pong between the caller, given that
      * this is a signal handler and we have to do a return "soon". Then the
-     * caller can reestablish everything and do a longjmp here again.
+     * caller can reestablish everything and do a siglongjmp here again.
      */
-    if (!setjmp(coTS->tr_reenter)) {
+    if (!sigsetjmp(coTS->tr_reenter, 0)) {
         return;
     }
 
     /*
-     * Ok, the caller has longjmp'ed back to us, so now prepare
+     * Ok, the caller has siglongjmp'ed back to us, so now prepare
      * us for the real machine state switching. We have to jump
      * into another function here to get a new stack context for
      * the auto variables (which have to be auto-variables
@@ -179,7 +179,7 @@ static Coroutine *coroutine_new(void)
 
     /* The way to manipulate stack is with the sigaltstack function. We
      * prepare a stack, with it delivering a signal to ourselves and then
-     * put setjmp/longjmp where needed.
+     * put sigsetjmp/siglongjmp where needed.
      * This has been done keeping coroutine-ucontext as a model and with the
      * pth ideas (GNU Portable Threads). See coroutine-ucontext for the basics
      * of the coroutines and see pth_mctx.c (from the pth project) for the
@@ -220,7 +220,7 @@ static Coroutine *coroutine_new(void)
 
     /*
      * Now transfer control onto the signal stack and set it up.
-     * It will return immediately via "return" after the setjmp()
+     * It will return immediately via "return" after the sigsetjmp()
      * was performed. Be careful here with race conditions.  The
      * signal can be delivered the first time sigsuspend() is
      * called.
@@ -261,8 +261,8 @@ static Coroutine *coroutine_new(void)
      * type-conversion warnings related to the `volatile' qualifier and
      * the fact that `jmp_buf' usually is an array type.
      */
-    if (!setjmp(old_env)) {
-        longjmp(coTS->tr_reenter, 1);
+    if (!sigsetjmp(old_env, 0)) {
+        siglongjmp(coTS->tr_reenter, 1);
     }
 
     /*
@@ -311,9 +311,9 @@ CoroutineAction qemu_coroutine_switch(Coroutine *from_, 
Coroutine *to_,
 
     s->current = to_;
 
-    ret = setjmp(from->env);
+    ret = sigsetjmp(from->env, 0);
     if (ret == 0) {
-        longjmp(to->env, action);
+        siglongjmp(to->env, action);
     }
     return ret;
 }
diff --git a/coroutine-ucontext.c b/coroutine-ucontext.c
index a9c30e9..bd20e38 100644
--- a/coroutine-ucontext.c
+++ b/coroutine-ucontext.c
@@ -46,7 +46,7 @@ static unsigned int pool_size;
 typedef struct {
     Coroutine base;
     void *stack;
-    jmp_buf env;
+    sigjmp_buf env;
 
 #ifdef CONFIG_VALGRIND_H
     unsigned int valgrind_stack_id;
@@ -130,8 +130,8 @@ static void coroutine_trampoline(int i0, int i1)
     co = &self->base;
 
     /* Initialize longjmp environment and switch back the caller */
-    if (!setjmp(self->env)) {
-        longjmp(*(jmp_buf *)co->entry_arg, 1);
+    if (!sigsetjmp(self->env, 0)) {
+        siglongjmp(*(sigjmp_buf *)co->entry_arg, 1);
     }
 
     while (true) {
@@ -145,14 +145,15 @@ static Coroutine *coroutine_new(void)
     const size_t stack_size = 1 << 20;
     CoroutineUContext *co;
     ucontext_t old_uc, uc;
-    jmp_buf old_env;
+    sigjmp_buf old_env;
     union cc_arg arg = {0};
 
-    /* The ucontext functions preserve signal masks which incurs a system call
-     * overhead.  setjmp()/longjmp() does not preserve signal masks but only
-     * works on the current stack.  Since we need a way to create and switch to
-     * a new stack, use the ucontext functions for that but setjmp()/longjmp()
-     * for everything else.
+    /* The ucontext functions preserve signal masks which incurs a
+     * system call overhead.  sigsetjmp(buf, 0)/siglongjmp() does not
+     * preserve signal masks but only works on the current stack.
+     * Since we need a way to create and switch to a new stack, use
+     * the ucontext functions for that but sigsetjmp()/siglongjmp() for
+     * everything else.
      */
 
     if (getcontext(&uc) == -1) {
@@ -178,8 +179,8 @@ static Coroutine *coroutine_new(void)
     makecontext(&uc, (void (*)(void))coroutine_trampoline,
                 2, arg.i[0], arg.i[1]);
 
-    /* swapcontext() in, longjmp() back out */
-    if (!setjmp(old_env)) {
+    /* swapcontext() in, siglongjmp() back out */
+    if (!sigsetjmp(old_env, 0)) {
         swapcontext(&old_uc, &uc);
     }
     return &co->base;
@@ -242,9 +243,9 @@ CoroutineAction qemu_coroutine_switch(Coroutine *from_, 
Coroutine *to_,
 
     s->current = to_;
 
-    ret = setjmp(from->env);
+    ret = sigsetjmp(from->env, 0);
     if (ret == 0) {
-        longjmp(to->env, action);
+        siglongjmp(to->env, action);
     }
     return ret;
 }
diff --git a/cpu-exec.c b/cpu-exec.c
index ff9a884..f257191 100644
--- a/cpu-exec.c
+++ b/cpu-exec.c
@@ -33,7 +33,7 @@ bool qemu_cpu_has_work(CPUState *cpu)
 void cpu_loop_exit(CPUArchState *env)
 {
     env->current_tb = NULL;
-    longjmp(env->jmp_env, 1);
+    siglongjmp(env->jmp_env, 1);
 }
 
 /* exit the current TB from a signal handler. The host registers are
@@ -45,7 +45,7 @@ void cpu_resume_from_signal(CPUArchState *env, void *puc)
     /* XXX: restore cpu registers saved in host registers */
 
     env->exception_index = -1;
-    longjmp(env->jmp_env, 1);
+    siglongjmp(env->jmp_env, 1);
 }
 #endif
 
@@ -231,7 +231,7 @@ int cpu_exec(CPUArchState *env)
 
     /* prepare setjmp context for exception handling */
     for(;;) {
-        if (setjmp(env->jmp_env) == 0) {
+        if (sigsetjmp(env->jmp_env, 0) == 0) {
             /* if an exception is pending, we execute it here */
             if (env->exception_index >= 0) {
                 if (env->exception_index >= EXCP_INTERRUPT) {
diff --git a/disas/i386.c b/disas/i386.c
index 3b006b1..175ea07 100644
--- a/disas/i386.c
+++ b/disas/i386.c
@@ -226,7 +226,7 @@ struct dis_private {
   bfd_byte the_buffer[MAX_MNEM_SIZE];
   bfd_vma insn_start;
   int orig_sizeflag;
-  jmp_buf bailout;
+  sigjmp_buf bailout;
 };
 
 enum address_mode
@@ -303,7 +303,7 @@ fetch_data2(struct disassemble_info *info, bfd_byte *addr)
         STATUS.  */
       if (priv->max_fetched == priv->the_buffer)
        (*info->memory_error_func) (status, start, info);
-      longjmp (priv->bailout, 1);
+      siglongjmp(priv->bailout, 1);
     }
   else
     priv->max_fetched = addr;
@@ -3661,7 +3661,7 @@ print_insn (bfd_vma pc, disassemble_info *info)
   start_codep = priv.the_buffer;
   codep = priv.the_buffer;
 
-  if (setjmp (priv.bailout) != 0)
+  if (sigsetjmp(priv.bailout, 0) != 0)
     {
       const char *name;
 
diff --git a/disas/m68k.c b/disas/m68k.c
index c950241..cc0db96 100644
--- a/disas/m68k.c
+++ b/disas/m68k.c
@@ -624,7 +624,7 @@ struct private
   bfd_byte *max_fetched;
   bfd_byte the_buffer[MAXLEN];
   bfd_vma insn_start;
-  jmp_buf bailout;
+  sigjmp_buf bailout;
 };
 
 /* Make sure that bytes from INFO->PRIVATE_DATA->BUFFER (inclusive)
@@ -644,7 +644,7 @@ fetch_data2(struct disassemble_info *info, bfd_byte *addr)
   if (status != 0)
     {
       (*info->memory_error_func) (status, start, info);
-      longjmp (priv->bailout, 1);
+      siglongjmp(priv->bailout, 1);
     }
   else
     priv->max_fetched = addr;
@@ -1912,9 +1912,10 @@ print_insn_m68k (bfd_vma memaddr, disassemble_info *info)
   priv.max_fetched = priv.the_buffer;
   priv.insn_start = memaddr;
 
-  if (setjmp (priv.bailout) != 0)
-    /* Error return.  */
-    return -1;
+  if (sigsetjmp(priv.bailout, 0) != 0) {
+      /* Error return.  */
+      return -1;
+  }
 
   switch (info->mach)
     {
diff --git a/include/exec/cpu-defs.h b/include/exec/cpu-defs.h
index 2911b9f..8958ed8 100644
--- a/include/exec/cpu-defs.h
+++ b/include/exec/cpu-defs.h
@@ -187,7 +187,7 @@ typedef struct CPUWatchpoint {
     struct GDBRegisterState *gdb_regs;                                  \
                                                                         \
     /* Core interrupt code */                                           \
-    jmp_buf jmp_env;                                                    \
+    sigjmp_buf jmp_env;                                                 \
     int exception_index;                                                \
                                                                         \
     CPUArchState *next_cpu; /* next CPU sharing TB cache */                 \
diff --git a/include/sysemu/os-win32.h b/include/sysemu/os-win32.h
index bf9edeb..71f5fa0 100644
--- a/include/sysemu/os-win32.h
+++ b/include/sysemu/os-win32.h
@@ -63,6 +63,14 @@
 # undef setjmp
 # define setjmp(env) _setjmp(env, NULL)
 #endif
+/* QEMU uses sigsetjmp()/siglongjmp() as the portable way to specify
+ * "longjmp and don't touch the signal masks". Since we know that the
+ * savemask parameter will always be zero we can safely define these
+ * in terms of setjmp/longjmp on Win32.
+ */
+#define sigjmp_buf jmp_buf
+#define sigsetjmp(env, savemask) setjmp(env)
+#define siglongjmp(env, val) longjmp(env, val)
 
 /* Declaration of ffs() is missing in MinGW's strings.h. */
 int ffs(int i);
diff --git a/monitor.c b/monitor.c
index 6a0f257..32a6e74 100644
--- a/monitor.c
+++ b/monitor.c
@@ -2740,7 +2740,7 @@ static const mon_cmd_t qmp_cmds[] = {
 /*******************************************************************/
 
 static const char *pch;
-static jmp_buf expr_env;
+static sigjmp_buf expr_env;
 
 #define MD_TLONG 0
 #define MD_I32   1
@@ -3135,7 +3135,7 @@ static const MonitorDef monitor_defs[] = {
 static void expr_error(Monitor *mon, const char *msg)
 {
     monitor_printf(mon, "%s\n", msg);
-    longjmp(expr_env, 1);
+    siglongjmp(expr_env, 1);
 }
 
 /* return 0 if OK, -1 if not found */
@@ -3345,7 +3345,7 @@ static int64_t expr_sum(Monitor *mon)
 static int get_expr(Monitor *mon, int64_t *pval, const char **pp)
 {
     pch = *pp;
-    if (setjmp(expr_env)) {
+    if (sigsetjmp(expr_env, 0)) {
         *pp = pch;
         return -1;
     }
diff --git a/user-exec.c b/user-exec.c
index c71acbc..71bd6c5 100644
--- a/user-exec.c
+++ b/user-exec.c
@@ -70,7 +70,7 @@ void cpu_resume_from_signal(CPUArchState *env1, void *puc)
 #endif
     }
     env1->exception_index = -1;
-    longjmp(env1->jmp_env, 1);
+    siglongjmp(env1->jmp_env, 1);
 }
 
 /* 'pc' is the host PC at which the exception was raised. 'address' is
-- 
1.7.11.4