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:
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().
