On 19/03/18(Mon) 15:38, Visa Hankala wrote:
> On Mon, Mar 19, 2018 at 12:27:10PM +0100, Martin Pieuchot wrote:
> > Thanks for the report.
> >
> > On 19/03/18(Mon) 09:49, Theo Buehler wrote:
> > > This is a regression that came with the TOCTOU race fix in kern_sig.c
> > > 1.216:
> > > https://cvsweb.openbsd.org/cgi-bin/cvsweb/src/sys/kern/kern_sig.c#rev1.216
> > > [...]
> > > Now gdb just hangs there and does nothing instead of exiting as
> > > expected. It doesn't react to ^C but one can easily kill it with
> > > ^Z and then kill %%.
> >
> > What happens is that the programs stays stopped. Or to be more precise
> > re-enter the SSTOP'd state after ptrace(PT_KILL...) has been issued by
> > gdb(1).
> > The problem comes from the fact that CURSIG() is now called twice in
> > userret(). That means that issignal() is also called twice. The fix
> > is to treat SIGKILL as special if the process is currently traced.
>
> As an alternative, the double call of issignal() could be avoided.
I like this. But I still think that we should handle SIGKILL correctly
in CURSIG(). However your fix seems safer for release.
> CURSIG(p) evaluates to zero if p->p_siglist is zero, or eventually
> issignal(p) returns zero if there are no unmasked signals (that is,
> if (p->p_siglist & ~p->p_sigmask) == 0).
But if the process is being traced issignal() is always called. Does
that mean that the `PS_TRACED' check is useless because issignal() also
starts with if (p->p_siglist & ~p->p_sigmask) == 0?
I'd prefer if you could used a function (inline) with an explicit name
like hassignal() or unmaskedsignal()?
> Index: kern/kern_sig.c
> ===================================================================
> RCS file: src/sys/kern/kern_sig.c,v
> retrieving revision 1.216
> diff -u -p -r1.216 kern_sig.c
> --- kern/kern_sig.c 26 Feb 2018 13:33:25 -0000 1.216
> +++ kern/kern_sig.c 19 Mar 2018 15:28:33 -0000
> @@ -1833,7 +1833,7 @@ userret(struct proc *p)
> KERNEL_UNLOCK();
> }
>
> - if (CURSIG(p) != 0) {
> + if ((p->p_siglist & ~p->p_sigmask) != 0) {
> KERNEL_LOCK();
> while ((signum = CURSIG(p)) != 0)
> postsig(p, signum);
