Re: [Qemu-devel] [PATCH] Actually block signals that have been queued by Qemu.

[Peter Maydell]

On 8 March 2015 at 04:18, Timothy Baldwin
<address@hidden> wrote:
> From ebcb08f4f4ed90c8557cafc02593360c59e10a49 Mon Sep 17 00:00:00 2001
> From: Timothy E Baldwin <address@hidden>
> Date: Sat, 7 Mar 2015 18:42:41 +0000
> Subject: [PATCH] Actually block signals that have been queued in TaskState.
>
> Actually block signals that have been queued in TaskState.
> For this purpose the set of block signals is stored in TaskState.
>
> Fixes bug #1429313
>
> Signed-off-by: Timothy Baldwin <address@hidden>

Thanks for this patch. How much testing of it have you done
beyond checking that it fixes the issue you've run into?
I ask because our signal handling behaviour in linux-user is
definitely dubious in several ways, so if we're unlucky then
fixing this might break some guest programs that happen to work
by fluke right now...

> ---
>  linux-user/qemu.h   |  2 +-
>  linux-user/signal.c | 39 ++++++++++++++++++++-------------------
>  2 files changed, 21 insertions(+), 20 deletions(-)
>
> diff --git a/linux-user/qemu.h b/linux-user/qemu.h
> index 8012cc2..7de543b 100644
> --- a/linux-user/qemu.h
> +++ b/linux-user/qemu.h
> @@ -127,13 +127,13 @@ 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 */
> +    sigset_t signal_mask;
>      int signal_pending; /* non zero if a signal may be pending */
>  } __attribute__((aligned(16))) TaskState;
>  diff --git a/linux-user/signal.c b/linux-user/signal.c
> index 5bb399e..0997c45 100644
> --- a/linux-user/signal.c
> +++ b/linux-user/signal.c
> @@ -209,40 +209,38 @@ int do_sigprocmask(int how, const sigset_t *set,
> sigset_t *oldset)
>      sigset_t *temp = NULL;
>      CPUState *cpu = thread_cpu;
>      TaskState *ts = (TaskState *)cpu->opaque;
> -    bool segv_was_blocked = ts->sigsegv_blocked;
> +
> +    if (oldset) {
> +        *oldset = ts->signal_mask;
> +    }
>       if (set) {
> -        bool has_sigsegv = sigismember(set, SIGSEGV);
>          val = *set;
>          temp = &val;
> -
> -        sigdelset(temp, SIGSEGV);
> +        int i;

Variable declarations should go at the start of a block.

>           switch (how) {
>          case SIG_BLOCK:
> -            if (has_sigsegv) {
> -                ts->sigsegv_blocked = true;
> -            }
> +            sigorset(&ts->signal_mask, &ts->signal_mask, &val);
>              break;
>          case SIG_UNBLOCK:
> -            if (has_sigsegv) {
> -                ts->sigsegv_blocked = false;
> +            /* There appears to be no signotset() */
> +            for(i = 0; i != sizeof(val.__val) / sizeof(val.__val[0]); ++i)
> {
> +                ts->signal_mask.__val[i] &= ~val.__val[i];
>              }
> +            ts->signal_pending = 1; /* May be unblocking pending signal */
>              break;
>          case SIG_SETMASK:
> -            ts->sigsegv_blocked = has_sigsegv;
> +            ts->signal_mask = val;
> +            ts->signal_pending = 1;
>              break;
>          default:
>              g_assert_not_reached();
>          }
> +        sigdelset(temp, SIGSEGV);
>      }
>  -    ret = sigprocmask(how, temp, oldset);
> -
> -    if (oldset && segv_was_blocked) {
> -        sigaddset(oldset, SIGSEGV);
> -    }
> -
> +    ret = sigprocmask(how, temp, 0);

So the effect of this patch is that we now have the guest's
signal mask in two places:
 1) the actual effective signal mask (except that SIGSEGV is never blocked)
 2) in ts->signal_mask

right?

Rather than manually updating the ts->signal_mask for each
of the set/unblock/block operations (which is pretty ugly for
unblock), can we just use the third argument of the sigprocmask()
call we're making anyway to get the previous effective signal
mask into ts->signal_mask, and then fix up the SIGSEGV entry in it?

>      return ret;
>  }
>  @@ -508,7 +506,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
> @@ -5591,8 +5589,11 @@ void process_pending_signals(CPUArchState *cpu_env)
>      /* FIXME: This is not threadsafe.  */
>      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)) {

I can sort of see why SEGV ends up special cased here. I can't
see a better way to deal with it offhand, anyway.

>              goto handle_signal;
> +        }
>          k++;
>      }
>      /* if no signal is pending, just return */
> @@ -5618,7 +5619,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.

thanks
-- PMM