ptrace(2) race testcase + proposed patch

[Tijl Coosemans]

While working on Wine I hit on a race between a ptrace PT_DETACH and
subsequent PT_ATTACH which causes the SIGSTOP of this attach to get
lost and never delivered. Attached are a test program and a proposed
patch.

The test program forks a child and loops attaching and detaching to it.
It can hang during the wait4 call.

The problem occurs when the stopped child process returns from the
ptracestop() call in sys/kern/kern_sig.c:issignal() after the parent
has already returned from the next PT_ATTACH. Then the signal that
caused the process to stop is taken off the sigqueue, but this may
already be a new SIGSTOP from the next PT_ATTACH. The solution is to
take the signal off the queue before calling ptracestop().

A second cause for the problem is in sys/kern/sys_process.c:kern_ptrace()
where PT_ATTACH sets td_xsig of a thread to SIGSTOP. If this thread
then returns from ptracestop() (returning td_xsig) and it was previously
stopped by a SIGSTOP, the new SIGSTOP is ignored and deleted at the end
of issignal(). The solution is to deliver the signal via td_xsig only
when the process is currently stopped (and now continuing) and
otherwise (PT_ATTACH) using psignal(). This is similar to equivalent
code in NetBSD.

I was hoping if someone could review this to make sure I did the right
thing, because I'm not entirely familiar with this code.
-------------- next part --------------
#include <sys/types.h>
#include <sys/ptrace.h>
#include <sys/wait.h>
#include <signal.h>
#include <stdio.h>
#include <unistd.h>

int main( void ) {

	pid_t pid;
	int status;

	/* fork dummy child process */
	pid = fork();
	if( pid == 0 ) {
		/* child does nothing */
		for( ; ; ) {
			sleep( 1 );
		}
	} else {
		/* parent */
		sleep( 1 );
		for( ; ; ) {
			/* loop: attach, wait, detach */
			printf( "attach " );
			fflush( stdout );
			ptrace( PT_ATTACH, pid, ( caddr_t ) 0, 0 );

			printf( "wait " );
			fflush( stdout );
			wait4( pid, &status, 0, NULL );

			printf( "detach " );
			fflush( stdout );
			ptrace( PT_DETACH, pid, ( caddr_t ) 1, 0 );

			printf( "\n" );
			fflush( stdout );
		}
	}

	return 0;
}
-------------- next part --------------
--- sys/kern/kern_sig.c.orig	2007-07-19 10:49:16.000000000 +0200
+++ sys/kern/kern_sig.c	2007-07-26 01:37:22.000000000 +0200
@@ -2543,6 +2543,20 @@
 			continue;
 		}
 		if (p->p_flag & P_TRACED && (p->p_flag & P_PPWAIT) == 0) {
+			ksiginfo_t ksi;
+
+			/*
+			 * clear old signal.
+			 * XXX shrug off debugger, it causes siginfo to
+			 * be thrown away.
+			 */
+			ksiginfo_init( &ksi );
+			sigqueue_get(&td->td_sigqueue, sig, &ksi);
+#ifdef KSE
+			if (td->td_pflags & TDP_SA)
+				SIGADDSET(td->td_sigmask, sig);
+#endif
+
 			/*
 			 * If traced, always stop.
 			 */
@@ -2550,20 +2564,7 @@
 			newsig = ptracestop(td, sig);
 			mtx_lock(&ps->ps_mtx);
 
-#ifdef KSE
-			if (td->td_pflags & TDP_SA)
-				SIGADDSET(td->td_sigmask, sig);
-
-#endif
 			if (sig != newsig) {
-				ksiginfo_t ksi;
-				/*
-				 * clear old signal.
-				 * XXX shrug off debugger, it causes siginfo to
-				 * be thrown away.
-				 */
-				sigqueue_get(&td->td_sigqueue, sig, &ksi);
-
 				/*
 				 * If parent wants us to take the signal,
 				 * then it will leave it in p->p_xstat;
@@ -2585,6 +2586,9 @@
 				if (SIGISMEMBER(td->td_sigmask, sig))
 					continue;
 				signotify(td);
+			} else {
+				/* Add old signal back. */
+				sigqueue_add(&td->td_sigqueue, sig, &ksi);
 			}
 
 			/*
--- sys/kern/sys_process.c.orig	2007-08-02 15:53:10.000000000 +0200
+++ sys/kern/sys_process.c	2007-08-02 19:49:56.000000000 +0200
@@ -779,14 +779,15 @@
 			sx_xunlock(&proctree_lock);
 			proctree_locked = 0;
 		}
-		/* deliver or queue signal */
-		thread_lock(td2);
-		td2->td_flags &= ~TDF_XSIG;
-		thread_unlock(td2);
-		td2->td_xsig = data;
 		p->p_xstat = data;
 		p->p_xthread = NULL;
 		if ((p->p_flag & (P_STOPPED_SIG | P_STOPPED_TRACE)) != 0) {
+			/* deliver or queue signal */
+			thread_lock(td2);
+			td2->td_flags &= ~TDF_XSIG;
+			thread_unlock(td2);
+			td2->td_xsig = data;
+
 			PROC_SLOCK(p);
 			if (req == PT_DETACH) {
 				struct thread *td3;
@@ -809,11 +810,10 @@
 			p->p_flag &= ~(P_STOPPED_TRACE|P_STOPPED_SIG|P_WAITED);
 			thread_unsuspend(p);
 			PROC_SUNLOCK(p);
+		} else {
+			if (data)
+				psignal(p, data);
 		}
-
-		if (data)
-			psignal(p, data);
-
 		break;
 
 	case PT_WRITE_I: