kern/142757: [patch] race condition in traced process signal
handling
Tijl Coosemans
tijl at coosemans.org
Tue Jan 12 22:00:10 UTC 2010
>Number: 142757
>Category: kern
>Synopsis: [patch] race condition in traced process signal handling
>Confidential: no
>Severity: non-critical
>Priority: low
>Responsible: freebsd-bugs
>State: open
>Quarter:
>Keywords:
>Date-Required:
>Class: sw-bug
>Submitter-Id: current-users
>Arrival-Date: Tue Jan 12 22:00:09 UTC 2010
>Closed-Date:
>Last-Modified:
>Originator: Tijl Coosemans
>Release: FreeBSD 8.0-STABLE i386
>Organization:
>Environment:
>Description:
There's a race condition in the kernel signal handling code that causes
signals to not be delivered to traced processes.
When a process is being traced it is stopped whenever it receives a
signal to allow the parent to intervene. The parent can let the signal
through or not or send a different signal. In the latter two cases the
original signal has to be taken off the child's signal queue. Currently
this happens after the child has resumed, but between resuming and
removing the signal the same signal number could be sent which
shouldn't be removed.
>How-To-Repeat:
I've attached two test cases that lock up (easily on a UP system).
race1.c:
This program calls ptrace(PT_ATTACH,...) and ptrace(PT_DETACH,...) in a
loop. It locks up while waiting for the child to stop after a PT_ATTACH
call. In that case the SIGSTOP from the PT_ATTACH isn't delivered. This
happens when the child only resumes from the previous PT_DETACH when
the parent has already done the next PT_ATTACH. The child removes the
next SIGSTOP from its set of pending signals when it thinks it removes
the previous one. Instead, the wait4 call should have acted as a
barrier guaranteeing that the previous SIGSTOP has been delivered,
handled and removed such that a new SIGSTOP can be sent.
This code was derived from a Windows API function in emulators/wine
to read the memory of another process. Calling this function multiple
times in a row tends to fail.
race2.c:
This program shows the same problem but looks more like a GDB debugging
session involving signals. It locks up when the signal sent by the kill
call is never delivered.
For the sake of simplicity the signal is sent by the parent which GDB
would never do, but imagine debugging a multi-threaded program where
threads send signals to one another. Under certain conditions those
signals may not be delivered.
>Fix:
I've attached a patch (RELENG_8) that I believe fixes the problem.
Basically it takes the signal off the queue before the process is
stopped instead of after the process resumes. If the parent decides to
let the signal through, it is added to the queue again. However, one
cannot simply use sigqueue_get() and sigqueue_add() for this because
sigqueue_add() would put the siginfo data back at the end of the queue
which would change the delivery order. Moreover, sigqueue_add() might
fail and return error.
So the patch introduces a sigqueue_get_partial() function which removes
the signal from sigsets like sigqueue_get() does but leaves siginfo
data on the queue. If the parent decides to let the signal through, it
is simply added to the same sigsets again. If the signal shouldn't go
through or another signal should be delivered the siginfo data is taken
off the queue.
Additionally, the patch also adds a SIGADDSET(queue->sq_kill, sig) call
in the case where a different signal should be delivered. I believe it
is needed to match what sigqueue_add() would do in that case (adding a
signal without siginfo data).
--- race1.c begins here ---
#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( "ok\n" ); fflush( stdout );
}
}
return 0;
}
--- race1.c ends here ---
--- race2.c begins here ---
#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 );
ptrace( PT_ATTACH, pid, ( caddr_t ) 0, 0 );
wait4( pid, &status, 0, NULL );
for( ;; ) {
/* loop: continue, kill, wait */
printf( "continue " ); fflush( stdout );
ptrace( PT_CONTINUE, pid, ( caddr_t ) 1, 0 );
printf( "kill " ); fflush( stdout );
kill( pid, SIGINT );
printf( "wait " ); fflush( stdout );
wait4( pid, &status, 0, NULL );
printf( "ok\n" ); fflush( stdout );
}
}
return 0;
}
--- race2.c ends here ---
--- patch-ptrace begins here ---
--- sys/kern/kern_sig.c.orig 2010-01-12 15:34:42.000000000 +0100
+++ sys/kern/kern_sig.c 2010-01-12 16:24:22.000000000 +0100
@@ -312,6 +312,38 @@ sigqueue_get(sigqueue_t *sq, int signo,
return (signo);
}
+int
+sigqueue_get_partial(sigqueue_t *sq, int signo, ksiginfo_t **si)
+{
+ struct ksiginfo *ksi;
+ int count = 0;
+
+ KASSERT(sq->sq_flags & SQ_INIT, ("sigqueue not inited"));
+
+ *si = NULL;
+
+ if (!SIGISMEMBER(sq->sq_signals, signo))
+ return (0);
+
+ if (SIGISMEMBER(sq->sq_kill, signo)) {
+ count++;
+ SIGDELSET(sq->sq_kill, signo);
+ }
+
+ TAILQ_FOREACH(ksi, &sq->sq_list, ksi_link) {
+ if (ksi->ksi_signo == signo) {
+ if (count == 0)
+ *si = ksi;
+ if (++count > 1)
+ break;
+ }
+ }
+
+ if (count <= 1)
+ SIGDELSET(sq->sq_signals, signo);
+ return (signo);
+}
+
void
sigqueue_take(ksiginfo_t *ksi)
{
@@ -2523,6 +2555,17 @@ issignal(struct thread *td, int stop_all
continue;
}
if (p->p_flag & P_TRACED && (p->p_flag & P_PPWAIT) == 0) {
+ ksiginfo_t *ksi;
+
+ /*
+ * Clear old signal, but keep siginfo on queue.
+ */
+ queue = &td->td_sigqueue;
+ if (sigqueue_get_partial(queue, sig, &ksi) == 0) {
+ queue = &p->p_sigqueue;
+ sigqueue_get_partial(queue, sig, &ksi);
+ }
+
/*
* If traced, always stop.
*/
@@ -2531,17 +2574,12 @@ issignal(struct thread *td, int stop_all
mtx_lock(&ps->ps_mtx);
if (sig != newsig) {
- ksiginfo_t ksi;
-
- queue = &td->td_sigqueue;
/*
- * clear old signal.
- * XXX shrug off debugger, it causes siginfo to
- * be thrown away.
+ * Remove old signal from queue.
*/
- if (sigqueue_get(queue, sig, &ksi) == 0) {
- queue = &p->p_sigqueue;
- sigqueue_get(queue, sig, &ksi);
+ if( ksi != NULL ) {
+ sigqueue_take(ksi);
+ ksiginfo_tryfree(ksi);
}
/*
@@ -2557,10 +2595,18 @@ issignal(struct thread *td, int stop_all
* Put the new signal into td_sigqueue. If the
* signal is being masked, look for other signals.
*/
+ SIGADDSET(queue->sq_kill, sig);
SIGADDSET(queue->sq_signals, sig);
if (SIGISMEMBER(td->td_sigmask, sig))
continue;
signotify(td);
+ } else {
+ /*
+ * Restore old signal.
+ */
+ if( ksi == NULL )
+ SIGADDSET(queue->sq_kill, sig);
+ SIGADDSET(queue->sq_signals, sig);
}
/*
--- sys/sys/signalvar.h.orig 2010-01-12 16:25:43.000000000 +0100
+++ sys/sys/signalvar.h 2010-01-12 16:26:16.000000000 +0100
@@ -358,6 +358,7 @@ void sigqueue_delete_set(struct sigqueue
void sigqueue_delete(struct sigqueue *queue, int sig);
void sigqueue_move_set(struct sigqueue *src, sigqueue_t *dst, sigset_t *);
int sigqueue_get(struct sigqueue *queue, int sig, ksiginfo_t *info);
+int sigqueue_get_partial(struct sigqueue *queue, int sig, ksiginfo_t **info);
int sigqueue_add(struct sigqueue *queue, int sig, ksiginfo_t *info);
void sigqueue_collect_set(struct sigqueue *queue, sigset_t *set);
void sigqueue_move(struct sigqueue *, struct sigqueue *, int sig);
--- patch-ptrace ends here ---
>Release-Note:
>Audit-Trail:
>Unformatted:
More information about the freebsd-bugs
mailing list