Date: Tue, 12 Jan 2010 22:54:49 +0100 From: Tijl Coosemans <tijl@coosemans.org> To: FreeBSD-gnats-submit@FreeBSD.org Subject: kern/142757: [patch] race condition in traced process signal handling Message-ID: <201001122255.02052.tijl@coosemans.org> Resent-Message-ID: <201001122200.o0CM0Af7091646@freefall.freebsd.org>
next in thread | raw e-mail | index | archive | help
>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:
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201001122255.02052.tijl>