Re: ptrace attach in multi-threaded processes

On Tue, Jul 12, 2016 at 09:02:10PM -0700, Mark Johnston wrote:
> Hm, the only SIGSTOP in my scenario is the one generated by PT_ATTACH.
> The problem occurs when this SIGSTOP races with *any* other signal that's
> being delivered to the process and for which the process has registered
> a handler. For instance, SIGHUP after a log rotation.
> 
> If a real SIGSTOP races with PT_ATTACH, then I would indeed expect to
> find the process in a stopped state after the detach. Does this make
> more sense?

I finally see.  Might be something like the patch below is a step in
the desired direction.  Idea is in the proc_next_xthread(): p_xthread
should be set to the next thread with a pending signal.  Do you have a
test case that demonstrates the issue ?

diff --git a/sys/kern/kern_sig.c b/sys/kern/kern_sig.c
index 2a5e6de..1106f3a 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) {
diff --git a/sys/kern/sys_process.c b/sys/kern/sys_process.c
index a6037e3..3d9950b 100644
--- a/sys/kern/sys_process.c
+++ b/sys/kern/sys_process.c
_at__at_ -1057,7 +1057,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 +1065,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 +1377,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..a3132d9 100644
--- a/sys/sys/proc.h
+++ b/sys/sys/proc.h
_at__at_ -999,6 +999,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);