| Message ID | 54FB4F00.5020000@members.leeds.ac.uk |
|---|---|
| State | New |
| Headers | show |
On 8 March 2015 at 04:18, Timothy Baldwin <T.E.Baldwin99@members.leeds.ac.uk> wrote: > From ebcb08f4f4ed90c8557cafc02593360c59e10a49 Mon Sep 17 00:00:00 2001 > From: Timothy E Baldwin <T.E.Baldwin99@members.leeds.ac.uk> > 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 <T.E.Baldwin99@members.leeds.ac.uk> > --- > 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; > 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]; > } You can't go messing around in the internals of the glibc representation like this. You need to use the public API functions, which in this case means looping around calling sigdelset(). -- PMM
On 8 March 2015 at 04:18, Timothy Baldwin <T.E.Baldwin99@members.leeds.ac.uk> wrote: > From ebcb08f4f4ed90c8557cafc02593360c59e10a49 Mon Sep 17 00:00:00 2001 > From: Timothy E Baldwin <T.E.Baldwin99@members.leeds.ac.uk> > 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 <T.E.Baldwin99@members.leeds.ac.uk> 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
On 08/03/15 10:23, Peter Maydell wrote: > On 8 March 2015 at 04:18, Timothy Baldwin > <T.E.Baldwin99@members.leeds.ac.uk> wrote: >> From ebcb08f4f4ed90c8557cafc02593360c59e10a49 Mon Sep 17 00:00:00 2001 >> From: Timothy E Baldwin <T.E.Baldwin99@members.leeds.ac.uk> >> 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 <T.E.Baldwin99@members.leeds.ac.uk> > 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... I do have SIGILL being handled in a guest to emulate ARM floating point (after disabling Qemu internal floating point support) I don't know what else to test it with, I could try Qemu itself. > >> --- >> 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. It is, note the deleted lines. > >> 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? Yes. I don't think this is correct in the case of execve(). > > 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? No, that would be the old value, we could make another call to sigprocmask(). We could abandon the above changes and just call sigprocmask() in process_pending_signals(), making this a 3 line change.
On 9 March 2015 at 20:19, Timothy Baldwin <T.E.Baldwin99@members.leeds.ac.uk> wrote: > > On 08/03/15 10:23, Peter Maydell wrote: >> >> On 8 March 2015 at 04:18, Timothy Baldwin >> <T.E.Baldwin99@members.leeds.ac.uk> wrote: >>> 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. > > It is, note the deleted lines. Immediately preceding line is now "temp = &val;", which isn't a declaration. >> 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? > > No, that would be the old value Oops, yes. -- PMM
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; 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); 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)) { 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.