Date: Thu, 2 Aug 2007 23:59:24 +0200 From: Tijl Coosemans <tijl@ulyssis.org> To: freebsd-hackers@freebsd.org Subject: ptrace(2) race testcase + proposed patch Message-ID: <200708022359.25700.tijl@ulyssis.org>
next in thread | raw e-mail | index | archive | help
--Boundary-00=_9OlsGsmBmDL0Wv2
Content-Type: text/plain;
charset="us-ascii"
Content-Transfer-Encoding: 7bit
Content-Disposition: inline
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.
--Boundary-00=_9OlsGsmBmDL0Wv2
Content-Type: text/plain;
charset="us-ascii";
name="race.c"
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
filename="race.c"
#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;
}
--Boundary-00=_9OlsGsmBmDL0Wv2
Content-Type: text/plain;
charset="us-ascii";
name="patch-ptrace"
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
filename="patch-ptrace"
--- 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:
--Boundary-00=_9OlsGsmBmDL0Wv2--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200708022359.25700.tijl>