On 20/03/18(Tue) 17:04, Visa Hankala wrote:
> On Tue, Mar 20, 2018 at 10:45:56AM +0100, Martin Pieuchot wrote:
> > 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?
>
> So it seems. The trace point is taken only if the signal mask allows
> signal delivery.
>
> > I'd prefer if you could used a function (inline) with an explicit name
> > like hassignal() or unmaskedsignal()?
>
> Updated diff:
I like it. I you don't return a boolean but the mask of pending signals
in the macro we could use it in issignal(). But that can be for a later
change.
ok mpi@
> 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 20 Mar 2018 16:53:25 -0000
> @@ -1833,7 +1833,7 @@ userret(struct proc *p)
> KERNEL_UNLOCK();
> }
>
> - if (CURSIG(p) != 0) {
> + if (SIGPENDING(p)) {
> KERNEL_LOCK();
> while ((signum = CURSIG(p)) != 0)
> postsig(p, signum);
> Index: sys/signalvar.h
> ===================================================================
> RCS file: src/sys/sys/signalvar.h,v
> retrieving revision 1.29
> diff -u -p -r1.29 signalvar.h
> --- sys/signalvar.h 26 Feb 2018 13:33:25 -0000 1.29
> +++ sys/signalvar.h 20 Mar 2018 16:53:25 -0000
> @@ -66,6 +66,11 @@ struct sigacts {
> #define SIG_HOLD (void (*)(int))3
>
> /*
> + * Check if process p has an unmasked signal pending.
> + */
> +#define SIGPENDING(p) (((p)->p_siglist & ~(p)->p_sigmask) != 0)
> +
> +/*
> * Determine signal that should be delivered to process p, the current
> * process, 0 if none. If there is a pending stop signal with default
> * action, the process stops in issignal().
