Re: [Qemu-devel] [PATCH 1/5] linux-user: Fix sigismember() check

[Peter Maydell]

On Wed, 22 May 2019 at 20:00, Aleksandar Markovic
<address@hidden> wrote:
>
> From: Miloš Stojanović <address@hidden>
>
> Fix copying between the host and target signal sets for the case when
> the target set is larger than the host set.
>
> sigismember() returns 1 if the specified signal number is a member of
> the specified signal set, but it can also return -1 if an error occurs
> (e.g. an out of range signal number is specified). All non-zero values
> would cause the signal to be added, so a comparison with 1 is added to
> assure that only the signals which are really in the set get added to
> the other set.
>
> Signed-off-by: Miloš Stojanović <address@hidden>
> Signed-off-by: Aleksandar Markovic <address@hidden>
> ---
>  linux-user/signal.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/linux-user/signal.c b/linux-user/signal.c
> index 44b2d3b..c08a7fe 100644
> --- a/linux-user/signal.c
> +++ b/linux-user/signal.c
> @@ -116,7 +116,7 @@ void host_to_target_sigset_internal(target_sigset_t *d,
>      int i;
>      target_sigemptyset(d);
>      for (i = 1; i <= TARGET_NSIG; i++) {
> -        if (sigismember(s, i)) {
> +        if (sigismember(s, i) == 1) {
>              target_sigaddset(d, host_to_target_signal(i));
>          }
>      }

I think perhaps a better fix for this would be to correct
the loop termination condition to be "i < _NSIG". What
we're doing here is looking at all the host signals in
s to see if we should be adding them to d, so what we
should be looping over is the valid range of host
signal numbers, not the valid range of target signal numbers.
Then we don't need to care about what sigismember() does
if passed an invalid signal number, because we'll never do that.

I also think we may have some off-by-one errors in various
comparisons with TARGET_NSIG and _NSIG: sometimes we
compare with <=, ie taking _NSIG or TARGET_NSIG to be
the highest valid signal number, and sometimes we compare
with <, taking it to be one greater than the highest valid
signal number. For instance compare the signal_init() code loop
and the size of arrays like host_to_target_signal_table[]
versus the loop condition in host_to_target_sigset_internal()
and target_to_host_sigset_internal().

I *think* that the correct intepretation is "it's one
greater than the highest valid number" and we should fix
the conditions in:
 host_to_target_sigset_internal
 target_to_host_sigset_internal
 do_sigprocmask
 signal_init
 host_signal_handler
 do_sigaction
 process_pending_signals

but somebody should definitely check whether I'm right or not.

thanks
-- PMM