Re: ptrace attach in multi-threaded processes

On Thu, Jul 14, 2016 at 11:16:05AM -0700, Mark Johnston wrote:
> Please see the program here:
> https://people.freebsd.org/~markj/ptrace_stop.c
> 
> It cheats a bit: it uses SIGSTOP to stop the child before sending a
> SIGHUP to it. However, this is just for convenience; note that PT_ATTACH
> will result in a call to thread_unsuspend() on the child, so PT_ATTACH's
> SIGSTOP will be delivered to a running process. When ptrace attaches,
> the child stops and WSTOPSIG(status) == SIGHUP. When ptrace detaches,
> the child is left stopped.
No, it is not for convenience, it relies on another bug to get the effect,
see below.

As I understand you intent, you prefer to get SIGSTOP from the first
waitpid(2) call after successful PT_ATTACH, am I right ?  At least for
single-threaded case, this can be achieved with a flag indicating that
we a doing first cursig(9) action after the attach, and preferring
SIGSTOP over any other queued signal.  The new flag P2_PTRACE_FSTP
does just that.  For mt case, I believe that some enchancements to
my proc_next_xthread() would fix that.

But when debugging the code, I found that it still does not work reliably
for your test.  The reason is that issignal() consumes a queued stop signal
after the thread_suspend_switch().  It allows the attach to occur, but then
sigqueue_delete() calls ('take the signal!') eat the signal for attach. It
seems that we should consume stops before going to stop state.  An open
question is how much this hurts when another (non-debugging) SIGSTOP is
queued while in stopped state.

Please try this.

diff --git a/sys/kern/kern_sig.c b/sys/kern/kern_sig.c
index 2a5e6de..36ed15f 100644
--- a/sys/kern/kern_sig.c
+++ b/sys/kern/kern_sig.c
_at__at_ -2525,22 +2525,21 _at__at_ ptracestop(struct thread *td, int sig)
 			PROC_SUNLOCK(p);
 			return (sig);
 		}
-		/*
-		 * Just make wait() to work, the last stopped thread
-		 * will win.
-		 */
-		p->p_xsig = sig;
-		p->p_xthread = td;
-		p->p_flag |= (P_STOPPED_SIG|P_STOPPED_TRACE);
-		sig_suspend_threads(td, p, 0);
-		if ((td->td_dbgflags & TDB_STOPATFORK) != 0) {
-			td->td_dbgflags &= ~TDB_STOPATFORK;
-			cv_broadcast(&p->p_dbgwait);
+		if (p->p_xthread == NULL)
+			p->p_xthread = td;
+		if (p->p_xthread == td) {
+			p->p_xsig = sig;
+			p->p_flag |= P_STOPPED_SIG | P_STOPPED_TRACE;
+			sig_suspend_threads(td, p, 0);
+			if ((td->td_dbgflags & TDB_STOPATFORK) != 0) {
+				td->td_dbgflags &= ~TDB_STOPATFORK;
+				cv_broadcast(&p->p_dbgwait);
+			}
 		}
 stopme:
 		thread_suspend_switch(td, p);
 		if (p->p_xthread == td)
-			p->p_xthread = NULL;
+			proc_next_xthread(p);
 		if (!(p->p_flag & P_TRACED))
 			break;
 		if (td->td_dbgflags & TDB_SUSPEND) {
_at__at_ -2725,7 +2724,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_ -2742,7 +2754,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_ -2845,6 +2857,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_ -2852,7 +2866,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_ -2883,6 +2897,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 a6037e3..af6b231 100644
--- a/sys/kern/sys_process.c
+++ b/sys/kern/sys_process.c
_at__at_ -885,6 +885,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;
 		if (p->p_flag & P_PPWAIT)
 			p->p_flag |= P_PPTRACE;
 		p->p_oppid = p->p_pptr->p_pid;
_at__at_ -903,6 +904,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_oppid = p->p_pptr->p_pid;
 		if (p->p_pptr != td->td_proc) {
 			proc_reparent(p, td->td_proc);
_at__at_ -1057,7 +1059,7 _at__at_ kern_ptrace(struct thread *td, int req, pid_t pid, void *addr, int data)
 			proctree_locked = 0;
 		}
 		p->p_xsig = data;
-		p->p_xthread = NULL;
+		proc_next_xthread(p);
 		if ((p->p_flag & (P_STOPPED_SIG | P_STOPPED_TRACE)) != 0) {
 			/* deliver or queue signal */
 			td2->td_dbgflags &= ~TDB_XSIG;
_at__at_ -1065,7 +1067,8 _at__at_ kern_ptrace(struct thread *td, int req, pid_t pid, void *addr, int data)
 
 			if (req == PT_DETACH) {
 				FOREACH_THREAD_IN_PROC(p, td3)
-					td3->td_dbgflags &= ~TDB_SUSPEND; 
+					td3->td_dbgflags &= ~(TDB_SUSPEND |
+					    TDB_XSIG);
 			}
 			/*
 			 * unsuspend all threads, to not let a thread run,
_at__at_ -1376,9 +1379,24 _at__at_ stopevent(struct proc *p, unsigned int event, unsigned int val)
 	do {
 		if (event != S_EXIT)
 			p->p_xsig = val;
-		p->p_xthread = NULL;
+		proc_next_xthread(p);
 		p->p_stype = event;	/* Which event caused the stop? */
 		wakeup(&p->p_stype);	/* Wake up any PIOCWAIT'ing procs */
 		msleep(&p->p_step, &p->p_mtx, PWAIT, "stopevent", 0);
 	} while (p->p_step);
 }
+
+void
+proc_next_xthread(struct proc *p)
+{
+	struct thread *td;
+
+	PROC_LOCK_ASSERT(p, MA_OWNED);
+	FOREACH_THREAD_IN_PROC(p, td) {
+		if (td == p->p_xthread)
+			continue;
+		if ((td->td_dbgflags & TDB_XSIG) != 0)
+			break;
+	}
+	p->p_xthread = td;
+}
diff --git a/sys/sys/proc.h b/sys/sys/proc.h
index f533db6..7ab7410 100644
--- a/sys/sys/proc.h
+++ b/sys/sys/proc.h
_at__at_ -712,6 +712,7 _at__at_ struct proc {
 #define	P2_NOTRACE_EXEC 0x00000004	/* Keep P2_NOPTRACE on exec(2). */
 #define	P2_AST_SU	0x00000008	/* Handles SU ast for kthreads. */
 #define	P2_LWP_EVENTS	0x00000010	/* Report LWP events via ptrace(2). */
+#define	P2_PTRACE_FSTP	0x00000020	/* First issignal() after PT_ATTACH. */
 
 /* Flags protected by proctree_lock, kept in p_treeflags. */
 #define	P_TREE_ORPHANED		0x00000001	/* Reparented, on orphan list */
_at__at_ -999,6 +1000,7 _at__at_ int	proc_getenvv(struct thread *td, struct proc *p, struct sbuf *sb);
 void	procinit(void);
 void	proc_linkup0(struct proc *p, struct thread *td);
 void	proc_linkup(struct proc *p, struct thread *td);
+void	proc_next_xthread(struct proc *p);
 struct proc *proc_realparent(struct proc *child);
 void	proc_reap(struct thread *td, struct proc *p, int *status, int options);
 void	proc_reparent(struct proc *child, struct proc *newparent);