Re: [Qemu-devel] [PATCH 1/1] Fix do_rt_sigreturn on m68k linux userspace emulation

[Michael Karcher <Michael.Karcher@fu-berlin.de>]

[-- Attachment #1: Type: text/plain, Size: 2256 bytes --]

On 09.12.2015 23:03, Laurent Vivier wrote:
>
> Le 09/12/2015 21:54, Michael Karcher a écrit :
>> do_rt_sigreturn forgets to initialize the signal mask variable before
>> trying to use it to restore the mask, so the signal mask is undefined
>> after do_rt_sigreturn. This bug has been in all the time since
>> 7181155d when do_rt_sigreturn was implemented for m68k.
>>
>> Signed-off-by: Michael Karcher <karcher@physik.fu-berlin.de>
>> ---
>>  linux-user/signal.c | 5 ++++-
>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/linux-user/signal.c b/linux-user/signal.c
>> index e03ed60..ae1014b 100644
>> --- a/linux-user/signal.c
>> +++ b/linux-user/signal.c
>> @@ -5260,11 +5260,14 @@ long do_rt_sigreturn(CPUM68KState *env)
>>      abi_ulong frame_addr = env->aregs[7] - 4;
>>      target_sigset_t target_set;
>>      sigset_t set;
>> -    int d0;
>> +    int d0, i;
>>  
>>      if (!lock_user_struct(VERIFY_READ, frame, frame_addr, 1))
>>          goto badframe;
>>  
>> +    for(i = 0; i < TARGET_NSIG_WORDS; i++) {
>> +        target_set.sig[i] = frame->uc.tuc_sigmask.sig[i];
>> +    }
>>      target_to_host_sigset_internal(&set, &target_set);
>>      do_sigprocmask(SIG_SETMASK, &set, NULL);
>>  
>>
> Nice catch.
>
> I agree with you that the current code is completely  broken, but on the
> other architectures, this operation seems to be done directly by
>
> target_to_host_sigset(&set, &frame->uc.tuc_sigmask);
>
> Could you have try with that ?
target_to_host_sigset does endianness swapping, while my loop does not
do it. I made a test program (attached to this mail) that tests mask
behaviour with "classic" and "siginfo" handlers, and shows that this
patch still doesn't fix the issue completely (output should be four
times 1). Instead of re-masking the SIGCHLD (bit 16) in that example, it
tries to mask SIGKILL (bit 8) which is due to the endian mismatch. I
will send a fixed patch shortly.

Feel free to extend the program to other architectures to test it there,
too.

BTW: documentation of the stack frame / signature for non-SA_SIGINFO
signal handlers seems to be quite lacking. There is a remark in the
sigaction manpage, but that one obviously only applies to i386...

Thanks for your comments,
  Michael Karcher

[-- Attachment #2: sigtest.c --]
[-- Type: text/x-csrc, Size: 1661 bytes --]

//#include <asm/sigcontext.h>
#include <signal.h>
#include <stdio.h>
#include <unistd.h>
#include <sys/types.h>
//#include <ucontext.h>

volatile int child_blocked;
volatile sig_atomic_t got_it;

#if defined(__i386)
#define HAVE_LEGACY
void handle(int signal, struct sigcontext sc)
{
	child_blocked = sc.oldmask == (1 << (SIGCHLD-1));
	got_it = 1;
}
#elif defined(__amd64)
#define HAVE_LEGACY
void handle(int signal, struct ucontext uc)
{
	child_blocked = ((struct sigcontext*)&uc.uc_mcontext)->oldmask == (1 << (SIGCHLD-1));
	got_it = 1;
}
#elif defined(__mc68000)
#define HAVE_LEGACY
void handle(int signal, int code, struct sigcontext *sc)
{
	child_blocked = sc->sc_mask == (1 << (SIGCHLD-1));
	got_it = 1;
}
#endif


void handle_siginfo(int signal, siginfo_t * info, void *ctx)
{
	child_blocked = sigismember(&((ucontext_t*)ctx)->uc_sigmask, SIGCHLD);
	got_it = 1;
}

int main(void)
{
	struct sigaction sa;
	sigset_t set;

#ifdef HAVE_LEGACY
	sa.sa_flags = 0;
	sa.sa_handler = handle;
	sigemptyset(&sa.sa_mask);
	sigaction(SIGUSR1, &sa, NULL);
	sigemptyset(&set);
	sigaddset(&set, SIGCHLD);
	sigprocmask(SIG_SETMASK, &set, NULL);
	got_it = 0;
	kill(getpid(), SIGUSR1);
	while(got_it == 0);
	sigprocmask(SIG_SETMASK, NULL, &set);
	printf("%d %d\n", child_blocked, sigismember(&set, SIGCHLD));
#endif

	got_it = 0;
	sa.sa_flags = SA_SIGINFO;
	sa.sa_sigaction = handle_siginfo;
	sigaction(SIGUSR1, &sa, NULL);
	sigemptyset(&set);
	sigaddset(&set, SIGCHLD);
	sigprocmask(SIG_SETMASK, &set, NULL);
	got_it = 0;
	kill(getpid(), SIGUSR1);
	while(got_it == 0);
	sigprocmask(SIG_SETMASK, NULL, &set);
	printf("%d %d\n", child_blocked, sigismember(&set, SIGCHLD));
}