Re: ptrace attach in multi-threaded processes

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
_at__at_ -2726,7 +2726,20 _at__at_ 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);
_at__at_ -2743,7 +2756,7 _at__at_ 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.
_at__at_ -2846,6 +2859,8 _at__at_ 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);
_at__at_ -2853,7 +2868,7 _at__at_ 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.
_at__at_ -2884,6 +2899,7 _at__at_ 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
_at__at_ -900,6 +900,7 _at__at_ 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;
_at__at_ -919,6 +920,7 _at__at_ 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
_at__at_ -712,6 +712,7 _at__at_ 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 */