vgdb must not transmit signals when gdbserver has been ptrace-invoked.

Most of the time, Valgrind masks async signals, and polls for such
signals at regular interval.
There is a very narrow range of code (around client syscall logic)
where such signals are unmasked (as they must be able to interrupt
syscalls).
This is the only range of code where Valgrind is expecting to
receive such a signal.

When gdbserver is ptraced invoked, Valgrind is artificially made
to jump out of this code portion. Signals are not masked.
When ptraceing valgrind, vgdb will get these signals but cannot
transmit them immediately, otherwise they arrive in range
of code where they are not expected (causing internal error
in syscall logic) and/or causing gdbserver syscalls to be
interrupted.

3 solutions to solve that were looked at:
1. have the gdbserver code masking signals "as quickly as possible".
 Easy to implement, but this leaves still a small window
 of code where signals are not masked and would cause a problem.
2. have vgdb setting the SIGMASK of valgrind before invoking
  gdbserver.
  This would be easy to implement, but changing the SIGMASK
  of the ptrace-d process is only available on very recent kernels.
3. have vgdb queuing signals, and only transmitting them once
   gdbserver invocation is finished.
This 3rd solution has been implemented.



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

NEWS

@@ -77,6 +77,7 @@ where XXXXXX is the bug number as listed below.
 n-i-bz  Fix KVM_CREATE_IRQCHIP ioctl handling
 n-i-bz  s390x: Fix memory corruption for multithreaded applications
 n-i-bz  vex arm->IR: allow PC as basereg in some LDRD cases
+n-i-bz  internal error in Valgrind if vgdb transmit signals when ptrace invoked
 
 Release 3.9.0 (31 October 2013)
 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

coregrind/vgdb-invoker-ptrace.c

@@ -60,8 +60,18 @@ typedef void* PTRACE_ARG3_TYPE;
 # error "unexpected wordsize"
 #endif
 
+// if > 0, pid for which registers have to be restored.
+// if == 0, means we have not yet called setregs (or have already
+// restored the registers).
+static int pid_of_save_regs = 0;
 /* True if we have continued pid_of_save_regs after PTRACE_ATTACH. */
 static Bool pid_of_save_regs_continued = False;
+// When setregs has been called to change the registers of pid_of_save_regs,
+// vgdb cannot transmit the signals intercepted during ptrace.
+// So, we queue them, and will deliver them when detaching.
+// See function waitstopped for more info.
+static int signal_queue_sz = 0;
+static siginfo_t *signal_queue;
 
 /* True when loss of connection indicating that the Valgrind
    process is dying. */
@@ -252,9 +262,44 @@ Bool waitstopped (pid_t pid, int signal_expected, const char *msg)
          break;
 
       /* pid received a signal which is not the signal we are waiting for.
-         We continue pid, transmitting this signal. */
-      DEBUG(1, "waitstopped PTRACE_CONT with signal %d\n", signal_received);
-      res = ptrace (PTRACE_CONT, pid, NULL, signal_received);
+         If we have not (yet) changed the registers of the inferior
+         or we have (already) reset them, we can transmit the signal.
+
+         If we have already set the registers of the inferior, we cannot
+         transmit the signal, as this signal would arrive when the
+         gdbserver code runs. And valgrind only expects signals to
+         arrive in a small code portion around
+         client syscall logic, where signal are unmasked (see e.g.
+         m_syswrap/syscall-x86-linux.S ML_(do_syscall_for_client_WRK).
+
+         As ptrace is forcing a call to gdbserver by jumping
+         'out of this region', signals are not masked, but
+         will arrive outside of the allowed/expected code region.
+         So, if we have changed the registers of the inferior, we
+         rather queue the signal to transmit them when detaching,
+         after having restored the registers to the initial values. */
+      if (pid_of_save_regs) {
+         siginfo_t *newsiginfo;
+
+         // realloc a bigger queue, and store new signal at the end.
+         // This is not very efficient but we assume not many sigs are queued.
+         signal_queue_sz++;
+         signal_queue = vrealloc(signal_queue, sizeof(siginfo_t) * signal_queue_sz);
+         newsiginfo = signal_queue + (signal_queue_sz - 1);
+
+         res = ptrace (PTRACE_GETSIGINFO, pid, NULL, newsiginfo);
+         if (res != 0) {
+            ERROR(errno, "PTRACE_GETSIGINFO failed: signal lost !!!!\n");
+            signal_queue_sz--;
+         } else
+            DEBUG(1, "waitstopped PTRACE_CONT, queuing signal %d"
+                  " si_signo %d si_pid %d\n",
+                  signal_received, newsiginfo->si_signo, newsiginfo->si_pid);
+         res = ptrace (PTRACE_CONT, pid, NULL, 0);
+      } else {
+         DEBUG(1, "waitstopped PTRACE_CONT with signal %d\n", signal_received);
+         res = ptrace (PTRACE_CONT, pid, NULL, signal_received);
+      }
       if (res != 0) {
          ERROR(errno, "waitstopped PTRACE_CONT\n");
          return False;
@@ -447,8 +492,6 @@ void detach_from_all_threads (pid_t pid)
    }
 }
 
-// if > 0, pid for which registers have to be restored.
-static int pid_of_save_regs = 0;
 static struct user user_save;
 
 // The below indicates if ptrace_getregs (and ptrace_setregs) can be used.
@@ -588,6 +631,11 @@ Bool setregs (pid_t pid, void *regs, long regs_bsz)
 static
 void restore_and_detach (pid_t pid)
 {
+   int res;
+
+   DEBUG(1, "restore_and_detach pid %d pid_of_save_regs %d\n",
+         pid, pid_of_save_regs);
+
    if (pid_of_save_regs) {
       /* In case the 'main pid' has been continued, we need to stop it
          before resetting the registers. */
@@ -602,10 +650,32 @@ void restore_and_detach (pid_t pid)
          ERROR(errno, "setregs restore registers pid %d after cont\n",
                pid_of_save_regs);
       }
+
+      /* Now, we transmit all the signals we have queued. */
+      if (signal_queue_sz > 0) {
+         int i;
+         for (i = 0; i < signal_queue_sz; i++) {
+            DEBUG(1, "PTRACE_CONT to transmit queued signal %d\n",
+                  signal_queue[i].si_signo);
+            res = ptrace (PTRACE_CONT, pid_of_save_regs, NULL,
+                          signal_queue[i].si_signo);
+            if (res != 0)
+               ERROR(errno, "PTRACE_CONT with signal %d\n",
+                     signal_queue[i].si_signo);
+            if (!stop(pid_of_save_regs, "sigstop after transmit sig"))
+               DEBUG(0, "Could not sigstop after transmit sig");
+         }
+         free (signal_queue);
+         signal_queue = NULL;
+         signal_queue_sz = 0;
+      }
       pid_of_save_regs = 0;
    } else {
       DEBUG(1, "PTRACE_SETREGS restore registers: no pid\n");
    }
+   if (signal_queue)
+      ERROR (0, "One or more signals queued were not delivered. "
+             "First signal: %d\n", signal_queue);
    detach_from_all_threads(pid);
 }
 

coregrind/vgdb.c

@@ -87,8 +87,6 @@ VgdbShared64 *shared64;
 
 #define VS_vgdb_pid (shared32 != NULL ? shared32->vgdb_pid : shared64->vgdb_pid)
 
-/* Calls malloc (size). Exits if memory can't be allocated. */
-static
 void *vmalloc(size_t size)
 {
    void * mem = malloc(size);
@@ -97,8 +95,6 @@ void *vmalloc(size_t size)
    return mem;
 }
 
-/* Calls realloc (size). Exits if memory can't be allocated. */
-static
 void *vrealloc(void *ptr,size_t size)
 {
    void * mem = realloc(ptr, size);

coregrind/vgdb.h

@@ -64,6 +64,11 @@ extern struct timeval dbgtv;
                             fflush(stderr),                              \
                             exit(1))
 
+/* Calls malloc (size). Exits if memory can't be allocated. */
+extern void *vmalloc(size_t size);
+/* Calls realloc (size). Exits if memory can't be allocated. */
+extern void *vrealloc(void *ptr,size_t size);
+
 /* Will be set to True when any condition indicating we have to shutdown
    is encountered. */
 extern Bool shutting_down;

gdbserver_tests/Makefile.am

@@ -119,7 +119,12 @@ EXTRA_DIST = \
 	nlsigvgdb.vgtest \
 	nlsigvgdb.stderr.exp \
 	nlsigvgdb.stderrB.exp \
-	nlsigvgdb.stdinB.gdb
+	nlsigvgdb.stdinB.gdb \
+	nlvgdbsigqueue.vgtest \
+	nlvgdbsigqueue.stderrB.exp \
+	nlvgdbsigqueue.stderr.exp \
+	nlvgdbsigqueue.stdinB.gdb \
+	nlvgdbsigqueue.stdoutB.exp
 
 check_PROGRAMS = \
 	clean_after_fork \

gdbserver_tests/nlvgdbsigqueue.stderr.exp

@@ -0,0 +1,11 @@
+Nulgrind, the minimal Valgrind tool
+
+(action at startup) vgdb me ... 
+
+
+loops/sleep_ms/burn/threads_spec:  1000000000 1000000000 1 BSBSBSBS
+Brussels ready to sleep and/or burn
+London ready to sleep and/or burn
+Petaouchnok ready to sleep and/or burn
+main ready to sleep and/or burn
+Gdb request to kill this process

gdbserver_tests/nlvgdbsigqueue.stdinB.gdb

@@ -0,0 +1,29 @@
+# connect gdb to Valgrind gdbserver:
+target remote | ./vgdb --wait=60 --vgdb-prefix=./vgdb-prefix-nlvgdbsigqueue
+echo vgdb launched process attached\n
+monitor v.set vgdb-error 999999
+#
+#
+# simulate control-c in a 1 second
+shell ./simulate_control_c --vgdb-prefix=./vgdb-prefix-nlvgdbsigqueue 1 grep main nlvgdbsigqueue.stderr.out
+#
+# send SIGUSR1/SIGUSR1 in a few seconds, when vgdb is attached
+shell ./send_signal USR1 --vgdb-prefix=./vgdb-prefix-nlvgdbsigqueue 2
+shell ./send_signal USR1 --vgdb-prefix=./vgdb-prefix-nlvgdbsigqueue 3
+#
+echo continuing to have vgdb interrupted by simulate_control_c\n
+continue
+#
+# Now vgdb should have received the interrupt, and so has
+# attached to the sleeping process.
+# wait for the USR sig to be sent, that will be queued by vgdb.
+shell sleep 4
+# continue, so as to have vgdb sending queued signals when PTRACE_DETACHing
+echo continuing to receive first SIGUSR1\n
+continue
+# simulate a control c to afterwards stop the execution
+shell ./simulate_control_c --vgdb-prefix=./vgdb-prefix-nlvgdbsigqueue 1
+echo continuing to receive second SIGUSR1\n
+continue
+kill
+quit

gdbserver_tests/nlvgdbsigqueue.stdoutB.exp

@@ -0,0 +1,14 @@
+continuing to have vgdb interrupted by simulate_control_c
+Continuing.
+Program received signal SIGUSR1, User defined signal 1.
+0x........ in syscall ...
+continuing to receive first SIGUSR1
+Continuing.
+[New Thread ....]
+Program received signal SIGUSR1, User defined signal 1.
+0x........ in syscall ...
+continuing to receive second SIGUSR1
+Continuing.
+Program received signal SIGTRAP, Trace/breakpoint trap.
+0x........ in syscall ...
+Kill the program being debugged? (y or n) [answered Y; input not from terminal]

gdbserver_tests/nlvgdbsigqueue.vgtest

@@ -0,0 +1,14 @@
+# test :
+#   sending signal when all threads are in WaitSys
+# vgdb must queue these signals and deliver them before PTRACE_DETACHing.
+# sleepers is started with argument so that it will mostly sleep.
+prog: sleepers
+args: 1000000000 1000000000 1 BSBSBSBS
+vgopts: --tool=none --vgdb=yes --vgdb-error=0 --vgdb-prefix=./vgdb-prefix-nlvgdbsigqueue
+stderr_filter: filter_stderr
+prereq: test -e gdb -a -f vgdb.invoker
+progB: gdb
+argsB: --quiet -l 60 --nx ./sleepers
+stdinB: nlvgdbsigqueue.stdinB.gdb
+stdoutB_filter: filter_gdb
+stderrB_filter: filter_make_empty

gdbserver_tests/send_signal

@@ -0,0 +1,17 @@
+#! /bin/sh
+
+# send_signal sends signal $1 to the Valgrind process using prefix $2 in $3 seconds
+SIG=$1
+shift
+PREFIX=$1
+shift
+SLEEP=$1
+shift
+VPID=`./vgdb -l $PREFIX 2>&1 | awk '{print $2}' | sed -e 's/--pid=//'`
+if [ "$VPID" = "" ]
+then
+  echo "send_signal could not determine the valgrind pid with " $PREFIX
+  exit 1
+fi
+(sleep $SLEEP
+ kill -s $SIG $VPID) &

gdbserver_tests/sleepers.c

@@ -8,11 +8,11 @@
 #include <sys/types.h>
 #include <sys/syscall.h>
 #include <sched.h>
-
+#include <signal.h>
 static int loops = 15; // each thread+main will do this amount of loop
 static int sleepms = 1000; // in each loop, will sleep "sleepms" milliseconds
 static int burn = 0; // after each sleep, will burn cpu in a tight 'burn' loop 
-
+static void setup_sigusr_handler(void); // sigusr1 and 2 sigaction setup.
 
 static pid_t gettid()
 {
@@ -133,7 +133,7 @@ int main (int argc, char *argv[])
   struct spec b, l, p, m;
   char *some_mem __attribute__((unused)) = malloc(100);
   setaffinity();
-
+  setup_sigusr_handler();
   if (argc > 1)
      loops = atoi(argv[1]);
 
@@ -194,3 +194,24 @@ int main (int argc, char *argv[])
 
   return 0;
 }
+
+static int sigusr1_received = 0;
+static void sigusr1_handler(int signr)
+{
+   sigusr1_received++;
+}
+static void setup_sigusr_handler(void)
+{
+   struct sigaction sa;
+   sa.sa_handler = sigusr1_handler;
+   sigemptyset(&sa.sa_mask);
+   sa.sa_flags = 0;
+
+   if (sigaction (SIGUSR1, &sa, NULL) != 0)
+      perror("sigaction SIGUSR1");
+
+   sa.sa_handler = SIG_IGN;
+   if (sigaction (SIGUSR2, &sa, NULL) != 0)
+      perror("sigaction SIGUSR2");
+}
+