Re: [Qemu-devel] [PATCH 1/2] signal: added a wrapper for sigprocmask function

[Peter Maydell]

On 29 September 2012 17:11, Alex Barcelo <address@hidden> wrote:

Hi; thanks for this patch.

> A transparent wrapper for sigprocmask function.

As I mentioned in my reply to the cover letter, this needs a
Signed-off-by: line.

The commit message could also be a bit more verbose, for instance
something like:

Create a wrapper for signal mask changes initiated by the guest;
this will give us a place to put code which prevents the guest
from changing the handling of signals used by QEMU itself
internally.

(To some extent this is a personal style thing, but I tend
to favour fairly verbose commentary and commit messages rather
than terseness.)

>
> ---
>  linux-user/qemu.h    |    1 +
>  linux-user/signal.c  |    8 ++++++++
>  linux-user/syscall.c |   20 ++++++++++----------
>  3 files changed, 19 insertions(+), 10 deletions(-)
>
> diff --git a/linux-user/qemu.h b/linux-user/qemu.h
> index fc4cc00..e2dd6a6 100644
> --- a/linux-user/qemu.h
> +++ b/linux-user/qemu.h
> @@ -237,6 +237,7 @@ int host_to_target_signal(int sig);
>  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);
>
>  #ifdef TARGET_I386
>  /* vm86.c */
> diff --git a/linux-user/signal.c b/linux-user/signal.c
> index 7869147..b8b8268 100644
> --- a/linux-user/signal.c
> +++ b/linux-user/signal.c
> @@ -5463,6 +5463,14 @@ long do_rt_sigreturn(CPUArchState *env)
>
>  #endif
>
> +/* Wrapper for sigprocmask function
> + * Emulates a sigprocmask in a safe way for the guest

You might like to add:
 * Note that set and oldset are host signal sets, not guest ones.

> + */
> +int do_sigprocmask(int how, const sigset_t *set, sigset_t *oldset)
> +{
> +    return sigprocmask(how, set, oldset);
> +}
> +
>  void process_pending_signals(CPUArchState *cpu_env)
>  {
>      int sig;
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index 471d060..1117bbe 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -4260,7 +4260,7 @@ static void *clone_func(void *arg)
>      if (info->parent_tidptr)
>          put_user_u32(info->tid, info->parent_tidptr);
>      /* Enable signals.  */
> -    sigprocmask(SIG_SETMASK, &info->sigmask, NULL);
> +    do_sigprocmask(SIG_SETMASK, &info->sigmask, NULL);
>      /* Signal to the parent that we're ready.  */
>      pthread_mutex_lock(&info->mutex);
>      pthread_cond_broadcast(&info->cond);
> @@ -4351,12 +4351,12 @@ static int do_fork(CPUArchState *env, unsigned int 
> flags, abi_ulong newsp,
>          /* It is not safe to deliver signals until the child has finished
>             initializing, so temporarily block all signals.  */
>          sigfillset(&sigmask);
> -        sigprocmask(SIG_BLOCK, &sigmask, &info.sigmask);
> +        do_sigprocmask(SIG_BLOCK, &sigmask, &info.sigmask);
>
>          ret = pthread_create(&info.thread, &attr, clone_func, &info);
>          /* TODO: Free new CPU state if thread creation failed.  */
>
> -        sigprocmask(SIG_SETMASK, &info.sigmask, NULL);
> +        do_sigprocmask(SIG_SETMASK, &info.sigmask, NULL);
>          pthread_attr_destroy(&attr);
>          if (ret == 0) {
>              /* Wait for the child to initialize.  */

The three sigprocmask() calls above are all QEMU itself manipulating
its own signal mask (to block signals temporarily across a thread
creation). They're not the guest asking for a signal mask change.
So these three can all just use sigprocmask() and shouldn't go via
do_sigprocmask() I think.

> @@ -5875,7 +5875,7 @@ abi_long do_syscall(void *cpu_env, int num, abi_long 
> arg1,
>          {
>              sigset_t cur_set;
>              abi_ulong target_set;
> -            sigprocmask(0, NULL, &cur_set);
> +            do_sigprocmask(0, NULL, &cur_set);
>              host_to_target_old_sigset(&target_set, &cur_set);
>              ret = target_set;
>          }
> @@ -5886,10 +5886,10 @@ abi_long do_syscall(void *cpu_env, int num, abi_long 
> arg1,
>          {
>              sigset_t set, oset, cur_set;
>              abi_ulong target_set = arg1;
> -            sigprocmask(0, NULL, &cur_set);
> +            do_sigprocmask(0, NULL, &cur_set);
>              target_to_host_old_sigset(&set, &target_set);
>              sigorset(&set, &set, &cur_set);
> -            sigprocmask(SIG_SETMASK, &set, &oset);
> +            do_sigprocmask(SIG_SETMASK, &set, &oset);
>              host_to_target_old_sigset(&target_set, &oset);
>              ret = target_set;
>          }
> @@ -5920,7 +5920,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(sigprocmask(how, &set, &oldset));
> +            ret = get_errno(do_sigprocmask(how, &set, &oldset));
>              if (!is_error(ret)) {
>                  host_to_target_old_sigset(&mask, &oldset);
>                  ret = mask;
> @@ -5954,7 +5954,7 @@ abi_long do_syscall(void *cpu_env, int num, abi_long 
> arg1,
>                  how = 0;
>                  set_ptr = NULL;
>              }
> -            ret = get_errno(sigprocmask(how, set_ptr, &oldset));
> +            ret = get_errno(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;
> @@ -5994,7 +5994,7 @@ abi_long do_syscall(void *cpu_env, int num, abi_long 
> arg1,
>                  how = 0;
>                  set_ptr = NULL;
>              }
> -            ret = get_errno(sigprocmask(how, set_ptr, &oldset));
> +            ret = get_errno(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;
> @@ -7870,7 +7870,7 @@ abi_long do_syscall(void *cpu_env, int num, abi_long 
> arg1,
>              }
>              mask = arg2;
>              target_to_host_old_sigset(&set, &mask);
> -            sigprocmask(how, &set, &oldset);
> +            do_sigprocmask(how, &set, &oldset);
>              host_to_target_old_sigset(&mask, &oldset);
>              ret = mask;
>          }


I think all the uses of sigprocmask() in linux-user/signal.c also
need to be do_sigprocmask(), as they are the guest trying to control
its signal mask (via the mask it specifies for running signal handlers,
or the mask it passes back when restoring context on return from a
signal handler).

-- PMM