Fix Bug 372600 - process loops forever when fatal signals are arriving quickly

git-svn-id: svn://svn.valgrind.org/valgrind/trunk@16140

diff --git a/NEWS b/NEWS
index 33699ae961260c79e07518456672355db7aef047..d22fd403d7d738e2cde7510c235632b7a6f7eb93 100644 (file)
--- 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 168b681e97447791a9c38bdf30ce54b4ab2ab503..b398882424ad736cabc62ac0d55719dff420c54a 100644 (file)
--- 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 ffe55d4e9402372e702de96c6be1fc0d02c4007a..987218a7d699280c0b0d152590dd85692226b753 100644 (file)
--- 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 (file)
index 0000000..500e168
--- /dev/null
+++ b/none/tests/pth_2sig.c
@@ -0,0 +1,45 @@
+#include <stdio.h>
+#include <unistd.h>
+#include <signal.h>
+#include <pthread.h>
+#include <assert.h>
+#include <stdlib.h>
+
+#include <sys/wait.h>
+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 (file)
index 0000000..e69de29

diff --git a/none/tests/pth_2sig.vgtest b/none/tests/pth_2sig.vgtest
new file mode 100644 (file)
index 0000000..b7c2b14
--- /dev/null
+++ b/none/tests/pth_2sig.vgtest
@@ -0,0 +1,2 @@
+prog: pth_2sig
+vgopts: -q