From 4e139bdeadef4857d23e1687cfb27ab8085eddc2 Mon Sep 17 00:00:00 2001 From: Philippe Waroquiers Date: Sat, 19 Nov 2016 13:51:41 +0000 Subject: [PATCH] Fix Bug 372600 - process loops forever when fatal signals are arriving quickly git-svn-id: svn://svn.valgrind.org/valgrind/trunk@16140 --- NEWS | 2 +- coregrind/m_signals.c | 48 ++++++++++++++++++++++++++++------ none/tests/Makefile.am | 4 ++- none/tests/pth_2sig.c | 45 +++++++++++++++++++++++++++++++ none/tests/pth_2sig.stderr.exp | 0 none/tests/pth_2sig.vgtest | 2 ++ 6 files changed, 91 insertions(+), 10 deletions(-) create mode 100644 none/tests/pth_2sig.c create mode 100644 none/tests/pth_2sig.stderr.exp create mode 100644 none/tests/pth_2sig.vgtest diff --git a/NEWS b/NEWS index 33699ae961..d22fd403d7 100644 --- a/NEWS +++ b/NEWS @@ -80,7 +80,7 @@ where XXXXXX is the bug number as listed below. 371869 support '%' in symbol Z-encoding 371916 execution tree xtree concept 372120 c++ demangler demangles symbols which are not c++ - +372600 process loops forever when fatal signals are arriving quickly Release 3.12.0 (20 October 2016) diff --git a/coregrind/m_signals.c b/coregrind/m_signals.c index 168b681e97..b398882424 100644 --- a/coregrind/m_signals.c +++ b/coregrind/m_signals.c @@ -2430,8 +2430,14 @@ void async_signalhandler ( Int sigNo, info->si_code = sanitize_si_code(info->si_code); if (VG_(clo_trace_signals)) - VG_(dmsg)("async signal handler: signal=%d, tid=%u, si_code=%d\n", - sigNo, tid, info->si_code); + VG_(dmsg)("async signal handler: signal=%d, tid=%u, si_code=%d, " + "exitreason %s\n", + sigNo, tid, info->si_code, + VG_(name_of_VgSchedReturnCode)(tst->exitreason)); + + /* */ + if (tst->exitreason == VgSrc_FatalSig) + resume_scheduler(tid); /* Update thread state properly. The signal can only have been delivered whilst we were in @@ -2479,8 +2485,16 @@ void async_signalhandler ( Int sigNo, ); /* (2) */ - /* Set up the thread's state to deliver a signal */ - if (!is_sig_ign(info, tid)) + /* Set up the thread's state to deliver a signal. + However, if exitreason is VgSrc_FatalSig, then thread tid was + taken out of a syscall by VG_(nuke_all_threads_except). + But after the emission of VKI_SIGKILL, another (fatal) async + signal might be sent. In such a case, we must not handle this + signal, as the thread is supposed to die first. + => resume the scheduler for such a thread, so that the scheduler + can let the thread die. */ + if (tst->exitreason != VgSrc_FatalSig + && !is_sig_ign(info, tid)) deliver_signal(tid, info, uc); /* It's crucial that (1) and (2) happen in the order (1) then (2) @@ -2946,6 +2960,20 @@ void VG_(poll_signals)(ThreadId tid) ThreadState *tst = VG_(get_ThreadState)(tid); vki_sigset_t saved_mask; + if (tst->exitreason == VgSrc_FatalSig) { + /* This task has been requested to die due to a fatal signal + received by the process. So, we cannot poll new signals, + as we are supposed to die asap. If we would poll and deliver + a new (maybe fatal) signal, this could cause a deadlock, as + this thread would believe it has to terminate the other threads + and wait for them to die, while we already have a thread doing + that. */ + if (VG_(clo_trace_signals)) + VG_(dmsg)("poll_signals: not polling as thread %u is exitreason %s\n", + tid, VG_(name_of_VgSchedReturnCode)(tst->exitreason)); + return; + } + /* look for all the signals this thread isn't blocking */ /* pollset = ~tst->sig_mask */ VG_(sigcomplementset)( &pollset, &tst->sig_mask ); @@ -2961,15 +2989,18 @@ void VG_(poll_signals)(ThreadId tid) /* If there was nothing queued, ask the kernel for a pending signal */ if (sip == NULL && VG_(sigtimedwait_zero)(&pollset, &si) > 0) { if (VG_(clo_trace_signals)) - VG_(dmsg)("poll_signals: got signal %d for thread %u\n", - si.si_signo, tid); + VG_(dmsg)("poll_signals: got signal %d for thread %u exitreason %s\n", + si.si_signo, tid, + VG_(name_of_VgSchedReturnCode)(tst->exitreason)); sip = &si; } if (sip != NULL) { /* OK, something to do; deliver it */ if (VG_(clo_trace_signals)) - VG_(dmsg)("Polling found signal %d for tid %u\n", sip->si_signo, tid); + VG_(dmsg)("Polling found signal %d for tid %u exitreason %s\n", + sip->si_signo, tid, + VG_(name_of_VgSchedReturnCode)(tst->exitreason)); if (!is_sig_ign(sip, tid)) deliver_signal(tid, sip, NULL); else if (VG_(clo_trace_signals)) @@ -3073,7 +3104,8 @@ void VG_(sigstartup_actions) ( void ) } if (VG_(clo_trace_signals)) - VG_(dmsg)("Max kernel-supported signal is %d\n", VG_(max_signal)); + VG_(dmsg)("Max kernel-supported signal is %d, VG_SIGVGKILL is %d\n", + VG_(max_signal), VG_SIGVGKILL); /* Our private internal signals are treated as ignored */ scss.scss_per_sig[VG_SIGVGKILL].scss_handler = VKI_SIG_IGN; diff --git a/none/tests/Makefile.am b/none/tests/Makefile.am index ffe55d4e94..987218a7d6 100644 --- a/none/tests/Makefile.am +++ b/none/tests/Makefile.am @@ -168,6 +168,7 @@ EXTRA_DIST = \ pth_rwlock.stderr.exp pth_rwlock.vgtest \ pth_stackalign.stderr.exp \ pth_stackalign.stdout.exp pth_stackalign.vgtest \ + pth_2sig.stderr.exp pth_2sig.vgtest \ pth_term_signal.stderr.exp pth_term_signal.vgtest \ rcrl.stderr.exp rcrl.stdout.exp rcrl.vgtest \ readline1.stderr.exp readline1.stdout.exp \ @@ -227,7 +228,7 @@ check_PROGRAMS = \ pselect_sigmask_null \ pth_atfork1 pth_blockedsig pth_cancel1 pth_cancel2 pth_cvsimple \ pth_empty pth_exit pth_exit2 pth_mutexspeed pth_once pth_rwlock \ - pth_stackalign pth_term_signal\ + pth_stackalign pth_2sig pth_term_signal\ rcrl readline1 \ require-text-symbol \ res_search resolv \ @@ -318,6 +319,7 @@ if VGCONF_OS_IS_SOLARIS pth_rwlock_CFLAGS += --std=c99 endif pth_stackalign_LDADD = -lpthread +pth_2sig_LDADD = -lpthread pth_term_signal_LDADD = -lpthread res_search_LDADD = -lresolv -lpthread resolv_CFLAGS = $(AM_CFLAGS) diff --git a/none/tests/pth_2sig.c b/none/tests/pth_2sig.c new file mode 100644 index 0000000000..500e168daf --- /dev/null +++ b/none/tests/pth_2sig.c @@ -0,0 +1,45 @@ +#include +#include +#include +#include +#include +#include + +#include +void * +slavethread(void *arg) +{ + char c; + while (1) + (void)read(0, &c, 1); +} + +int main(int argc, char **argv) +{ + pthread_t slave; + int i = 0; + int pid = getpid(); + + for (i = 0; i < 10; i++) + if (pthread_create(&slave, 0, &slavethread, 0)) + { + perror("slave2"); + exit(2); + } + switch (fork()) + { + case 0: // child + sleep(2); // Should be enough to ensure (some) threads are created + for (i = 0; i < 20 && kill(pid, 2) == 0; i++) + ; + exit(0); + case -1: + perror("fork"); + exit(4); + } + + while (rand() >= 0) + i++; + fprintf(stderr, "strange, this program is supposed to be killed !!!!\n"); + return 1; +} diff --git a/none/tests/pth_2sig.stderr.exp b/none/tests/pth_2sig.stderr.exp new file mode 100644 index 0000000000..e69de29bb2 diff --git a/none/tests/pth_2sig.vgtest b/none/tests/pth_2sig.vgtest new file mode 100644 index 0000000000..b7c2b14d61 --- /dev/null +++ b/none/tests/pth_2sig.vgtest @@ -0,0 +1,2 @@ +prog: pth_2sig +vgopts: -q -- 2.47.2