Date: Sat, 16 Jul 2016 12:52:50 +0300 From: Konstantin Belousov <kostikbel@gmail.com> To: Mark Johnston <markj@FreeBSD.org> Cc: freebsd-current@FreeBSD.org Subject: Re: ptrace attach in multi-threaded processes Message-ID: <20160716095250.GH38613@kib.kiev.ua> In-Reply-To: <20160715180159.GA4487@wkstn-mjohnston.west.isilon.com> References: <20160713033036.GR38613@kib.kiev.ua> <20160713040210.GA89573@wkstn-mjohnston.west.isilon.com> <20160713045439.GT38613@kib.kiev.ua> <20160713164247.GA2066@wkstn-mjohnston.west.isilon.com> <20160713191947.GW38613@kib.kiev.ua> <20160713200139.GC2066@wkstn-mjohnston.west.isilon.com> <20160714052537.GZ38613@kib.kiev.ua> <20160714181605.GA17310@wkstn-mjohnston.west.isilon.com> <20160715072720.GB38613@kib.kiev.ua> <20160715180159.GA4487@wkstn-mjohnston.west.isilon.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On Fri, Jul 15, 2016 at 11:01:59AM -0700, Mark Johnston wrote:
> Thanks, this seems to give the desired behaviour in the single-threaded
> case. I'll write a test case for the multi-threaded case next.
>
> Am I correct in thinking that r302179 could be reverted if your change
> is committed?
I suspect that it is not.
Suppose that we have a single-threaded process which only thread
is right on the syscall exit path when the debugger is attached,
and debugger requested PTRACE_SCX stops. Then the debuggee reaches
the ptracestop(SIGTRAP) stop point before cursig(9) is ever called.
So despite the patch, first reported signal is SIGTRAP and not the
attaching STOP. If debugger detaches right after that, the process
should still be left in stopped state.
You may object that gcore(1) does not request SCX, but my point is that
even with the patch, first reported signal could be other than the
STOP. I suspect that some other ptracestop() call might cause that
behaviour either now or in future even with the default event mask.
So I would left the r302179 in place.
Below is the patch with reverted next_xthread() bits. I reverted them
because Peter found the changes to require much more work to not cause
regressions.
diff --git a/sys/kern/kern_sig.c b/sys/kern/kern_sig.c
index 1da4b99..9e1a494 100644
--- a/sys/kern/kern_sig.c
+++ b/sys/kern/kern_sig.c
@@ -2726,7 +2726,20 @@ issignal(struct thread *td)
SIG_STOPSIGMASK(sigpending);
if (SIGISEMPTY(sigpending)) /* no signal to send */
return (0);
- sig = sig_ffs(&sigpending);
+ if ((p->p_flag & (P_TRACED | P_PPTRACE)) == P_TRACED &&
+ (p->p_flag2 & P2_PTRACE_FSTP) != 0 &&
+ SIGISMEMBER(sigpending, SIGSTOP)) {
+ /*
+ * If debugger just attached, always consume
+ * SIGSTOP from ptrace(PT_ATTACH) first, to
+ * execute the debugger attach ritual in
+ * order.
+ */
+ sig = SIGSTOP;
+ p->p_flag2 &= ~P2_PTRACE_FSTP;
+ } else {
+ sig = sig_ffs(&sigpending);
+ }
if (p->p_stops & S_SIG) {
mtx_unlock(&ps->ps_mtx);
@@ -2743,7 +2756,7 @@ issignal(struct thread *td)
sigqueue_delete(&p->p_sigqueue, sig);
continue;
}
- if (p->p_flag & P_TRACED && (p->p_flag & P_PPTRACE) == 0) {
+ if ((p->p_flag & (P_TRACED | P_PPTRACE)) == P_TRACED) {
/*
* If traced, always stop.
* Remove old signal from queue before the stop.
@@ -2846,6 +2859,8 @@ issignal(struct thread *td)
mtx_unlock(&ps->ps_mtx);
WITNESS_WARN(WARN_GIANTOK | WARN_SLEEPOK,
&p->p_mtx.lock_object, "Catching SIGSTOP");
+ sigqueue_delete(&td->td_sigqueue, sig);
+ sigqueue_delete(&p->p_sigqueue, sig);
p->p_flag |= P_STOPPED_SIG;
p->p_xsig = sig;
PROC_SLOCK(p);
@@ -2853,7 +2868,7 @@ issignal(struct thread *td)
thread_suspend_switch(td, p);
PROC_SUNLOCK(p);
mtx_lock(&ps->ps_mtx);
- break;
+ goto next;
} else if (prop & SA_IGNORE) {
/*
* Except for SIGCONT, shouldn't get here.
@@ -2884,6 +2899,7 @@ issignal(struct thread *td)
}
sigqueue_delete(&td->td_sigqueue, sig); /* take the signal! */
sigqueue_delete(&p->p_sigqueue, sig);
+next:;
}
/* NOTREACHED */
}
diff --git a/sys/kern/sys_process.c b/sys/kern/sys_process.c
index f1477ce..86e7c52 100644
--- a/sys/kern/sys_process.c
+++ b/sys/kern/sys_process.c
@@ -900,6 +900,7 @@ kern_ptrace(struct thread *td, int req, pid_t pid, void *addr, int data)
case PT_TRACE_ME:
/* set my trace flag and "owner" so it can read/write me */
p->p_flag |= P_TRACED;
+ p->p_flag2 |= P2_PTRACE_FSTP;
p->p_ptevents = PTRACE_DEFAULT;
if (p->p_flag & P_PPWAIT)
p->p_flag |= P_PPTRACE;
@@ -919,6 +920,7 @@ kern_ptrace(struct thread *td, int req, pid_t pid, void *addr, int data)
* on a "detach".
*/
p->p_flag |= P_TRACED;
+ p->p_flag2 |= P2_PTRACE_FSTP;
p->p_ptevents = PTRACE_DEFAULT;
p->p_oppid = p->p_pptr->p_pid;
if (p->p_pptr != td->td_proc) {
diff --git a/sys/sys/proc.h b/sys/sys/proc.h
index 0cde084..7b25083 100644
--- a/sys/sys/proc.h
+++ b/sys/sys/proc.h
@@ -712,6 +712,7 @@ struct proc {
#define P2_NOTRACE 0x00000002 /* No ptrace(2) attach or coredumps. */
#define P2_NOTRACE_EXEC 0x00000004 /* Keep P2_NOPTRACE on exec(2). */
#define P2_AST_SU 0x00000008 /* Handles SU ast for kthreads. */
+#define P2_PTRACE_FSTP 0x00000010 /* First issignal() after PT_ATTACH. */
/* Flags protected by proctree_lock, kept in p_treeflags. */
#define P_TREE_ORPHANED 0x00000001 /* Reparented, on orphan list */
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20160716095250.GH38613>