Bug 445011: SIGCHLD is sent when valgrind uses debuginfod-find

Valgrind fork+execs debuginfod-find in order to perform debuginfod
queries. Any SIGCHLD debuginfod-find sends upon termination can
mistakenly be delivered to the client running under valgrind.

To prevent this, record in a hash table the PID of each process
valgrind forks for internal use. Do not send SIGCHLD to the client
if it is from a PID in this hash table.

https://bugs.kde.org/show_bug.cgi?id=445011

NEWS

@@ -72,6 +72,7 @@ are not entered into bugzilla tend to get forgotten about or ignored.
 445032  valgrind/memcheck crash with SIGSEGV when SIGVTALRM timer used and
         libthr.so associated
 445300  [PATCH] Fix building tests with Musl
+445011  SIGCHLD is sent when valgrind uses debuginfod-find
 445354  arm64 backend: incorrect code emitted for doubleword CAS
 445415  arm64 front end: alignment checks missing for atomic instructions
 445504  Using C++ condition_variable results in bogus "mutex is locked simultaneously by two threads" warning

coregrind/m_libcproc.c

@@ -38,6 +38,7 @@
 #include "pub_core_libcsignal.h"
 #include "pub_core_seqmatch.h"
 #include "pub_core_mallocfree.h"
+#include "pub_core_signals.h"
 #include "pub_core_syscall.h"
 #include "pub_core_xarray.h"
 #include "pub_core_clientstate.h"
@@ -888,14 +889,68 @@ Int VG_(ptrace) ( Int request, Int pid, void *addr, void *data )
    Fork
    ------------------------------------------------------------------ */
 
+/* Record PID of a child process in order to avoid sending any SIGCHLD from
+   it to the client.  If PID is 0 then this is the child process and it
+   should synch with the parent to ensure it can't send any SIGCHLD before
+   the parent has registered its PID.
+
+   FDS should be initialized with VG_(pipe). This function closes both
+   file descriptors.  */
+static void register_sigchld_ignore ( Int pid, Int fds[2])
+{
+   Int child_wait = 1;
+   ht_ignore_node *n;
+
+   if (fds[0] < 0 || fds[1] < 0)
+      return;
+
+   if (pid == 0) {
+      /* Before proceeding, ensure parent has recorded child PID in map
+         of SIGCHLD to ignore */
+      while (child_wait == 1)
+      {
+         if (VG_(read)(fds[0], &child_wait, sizeof(Int)) <= 0) {
+            VG_(message)(Vg_DebugMsg,
+               "warning: Unable to record PID of internal process (read)\n");
+            child_wait = 0;
+         }
+      }
+
+      VG_(close)(fds[0]);
+      return;
+   }
+
+   n = VG_(malloc)("ht.ignore.node", sizeof(ht_ignore_node));
+   n->key = pid;
+   if (ht_sigchld_ignore == NULL)
+      ht_sigchld_ignore = VG_(HT_construct)("ht.sigchld.ignore");
+   VG_(HT_add_node)(ht_sigchld_ignore, n);
+
+   child_wait = 0;
+   if (VG_(write)(fds[1], &child_wait, sizeof(Int)) <= 0)
+      VG_(message)(Vg_DebugMsg,
+         "warning: Unable to record PID of internal process (write)\n");
+
+   VG_(close)(fds[1]);
+}
+
 Int VG_(fork) ( void )
 {
+   Int fds[2];
+
+   if (VG_(pipe)(fds) != 0) {
+      VG_(message)(Vg_DebugMsg,
+         "warning: Unable to record PID of internal process (pipe)\n");
+      fds[0] = fds[1] = -1;
+   }
+
 #  if defined(VGP_arm64_linux) || defined(VGP_nanomips_linux)
    SysRes res;
    res = VG_(do_syscall5)(__NR_clone, VKI_SIGCHLD,
                           (UWord)NULL, (UWord)NULL, (UWord)NULL, (UWord)NULL);
    if (sr_isError(res))
       return -1;
+   register_sigchld_ignore(sr_Res(res), fds);
    return sr_Res(res);
 
 #  elif defined(VGO_linux) || defined(VGO_freebsd)
@@ -903,6 +958,7 @@ Int VG_(fork) ( void )
    res = VG_(do_syscall0)(__NR_fork);
    if (sr_isError(res))
       return -1;
+   register_sigchld_ignore(sr_Res(res), fds);
    return sr_Res(res);
 
 #  elif defined(VGO_darwin)
@@ -912,8 +968,10 @@ Int VG_(fork) ( void )
       return -1;
    /* on success: wLO = child pid; wHI = 1 for child, 0 for parent */
    if (sr_ResHI(res) != 0) {
+      register_sigchld_ignore(0, fds);
       return 0;  /* this is child: return 0 instead of child pid */
    }
+   register_sigchld_ignore(sr_Res(res), fds);
    return sr_Res(res);
 
 #  elif defined(VGO_solaris)
@@ -930,8 +988,10 @@ Int VG_(fork) ( void )
               child,
         val2 = 0 in the parent process, 1 in the child process. */
    if (sr_ResHI(res) != 0) {
+      register_sigchld_ignore(0, fds);
       return 0;
    }
+   register_sigchld_ignore(sr_Res(res), fds);
    return sr_Res(res);
 
 #  else

coregrind/m_signals.c

@@ -207,6 +207,7 @@
 #include "pub_core_aspacemgr.h"
 #include "pub_core_errormgr.h"
 #include "pub_core_gdbserver.h"
+#include "pub_core_hashtable.h"
 #include "pub_core_libcbase.h"
 #include "pub_core_libcassert.h"
 #include "pub_core_libcprint.h"
@@ -247,6 +248,9 @@ typedef struct SigQueue {
    vki_siginfo_t sigs[N_QUEUED_SIGNALS];
 } SigQueue;
 
+/* Hash table of PIDs from which SIGCHLD is ignored.  */
+VgHashTable *ht_sigchld_ignore = NULL;
+
 /* ------ Macros for pulling stuff out of ucontexts ------ */
 
 /* Q: what does VG_UCONTEXT_SYSCALL_SYSRES do?  A: let's suppose the
@@ -2058,6 +2062,28 @@ static void deliver_signal ( ThreadId tid, const vki_siginfo_t *info,
    void			*handler_fn;
    ThreadState		*tst = VG_(get_ThreadState)(tid);
 
+#if defined(VGO_linux)
+   /* If this signal is SIGCHLD and it came from a process which valgrind
+      created for some internal use, then it should not be delivered to
+      the client.  */
+   if (sigNo == VKI_SIGCHLD && ht_sigchld_ignore != NULL) {
+      Int pid = info->_sifields._sigchld._pid;
+      ht_ignore_node *n = VG_(HT_lookup)(ht_sigchld_ignore, pid);
+
+      if (n != NULL) {
+         /* If the child has terminated, remove its PID from the
+            ignore list.  */
+         if (info->si_code == VKI_CLD_EXITED
+             || info->si_code == VKI_CLD_KILLED
+             || info->si_code == VKI_CLD_DUMPED) {
+            VG_(HT_remove)(ht_sigchld_ignore, pid);
+            VG_(free)(n);
+         }
+         return;
+      }
+   }
+#endif
+
    if (VG_(clo_trace_signals))
       VG_(dmsg)("delivering signal %d (%s):%d to thread %u\n", 
                 sigNo, VG_(signame)(sigNo), info->si_code, tid );

coregrind/pub_core_signals.h

@@ -35,6 +35,7 @@
 
 #include "pub_tool_signals.h"       // I want to get rid of this header...
 #include "pub_core_vki.h"           // vki_sigset_t et al.
+#include "pub_tool_hashtable.h"
 
 /* Highest signal the kernel will let us use */
 extern Int VG_(max_signal);
@@ -85,6 +86,15 @@ extern Bool VG_(extend_stack)(ThreadId tid, Addr addr);
    before using that signal to kill the process. */
 extern void VG_(set_default_handler)(Int sig);
 
+/* Hash table of PIDs from which SIGCHLD is ignored.  */
+extern VgHashTable *ht_sigchld_ignore;
+
+/* Hash table node where each key represents a PID.  */
+typedef struct _ht_ignore_node {
+   struct _ht_ignore_node *next;
+   UWord   key;
+} ht_ignore_node;
+
 #endif   // __PUB_CORE_SIGNALS_H
 
 /*--------------------------------------------------------------------*/

include/vki/vki-darwin.h

@@ -525,6 +525,12 @@ typedef
 #define VKI_BUS_ADRERR   BUS_ADRERR
 #define VKI_BUS_OBJERR   BUS_OBJERR
 #define VKI_TRAP_BRKPT   TRAP_BRKPT
+#define VKI_CLD_EXITED   CLD_EXITED
+#define VKI_CLD_KILLED   CLD_KILLED
+#define VKI_CLD_DUMPED   CLD_DUMPED
+#define VKI_CLD_TRAPPED  CLD_TRAPPED
+#define VKI_CLD_STOPPED  CLD_STOPPED
+#define VKI_CLD_CONTINUED CLD_CONTINUED
 
 /* JRS: not 100% sure, but I think these two are correct */
 #define VKI_SA_ONESHOT   SA_RESETHAND

include/vki/vki-freebsd.h

@@ -609,6 +609,16 @@ typedef struct vki_siginfo {
 #define VKI_TRAP_DTRACE 3   /* DTrace induced trap.                 */
 #define VKI_TRAP_CAP    4   /* Capabilities protective trap.        */
 
+/*
+ * SIGCHLD si_codes
+ */
+#define VKI_CLD_EXITED    1  /* child has exited */
+#define VKI_CLD_KILLED    2  /* child was killed */
+#define VKI_CLD_DUMPED    3  /* child terminated abnormally */
+#define VKI_CLD_TRAPPED   4  /* traced child has trapped */
+#define VKI_CLD_STOPPED   5  /* child has stopped */
+#define VKI_CLD_CONTINUED 6  /* stopped child has continued */
+
 #if 0 /* freebsd-6 */
 typedef struct vki_sigevent {
    int sigev_notify;

include/vki/vki-linux.h

@@ -576,6 +576,16 @@ typedef struct vki_siginfo {
 #define VKI_TRAP_BRKPT      (__VKI_SI_FAULT|1)  /* process breakpoint */
 #define VKI_TRAP_TRACE      (__VKI_SI_FAULT|2)  /* process trace trap */
 
+/*
+ * SIGCHLD si_codes
+ */
+#define VKI_CLD_EXITED    (__VKI_SI_FAULT|1)  /* child has exited */
+#define VKI_CLD_KILLED    (__VKI_SI_FAULT|2)  /* child was killed */
+#define VKI_CLD_DUMPED    (__VKI_SI_FAULT|3)  /* child terminated abnormally */
+#define VKI_CLD_TRAPPED   (__VKI_SI_FAULT|4)  /* traced child has trapped */
+#define VKI_CLD_STOPPED   (__VKI_SI_FAULT|5)  /* child has stopped */
+#define VKI_CLD_CONTINUED (__VKI_SI_FAULT|6)  /* stopped child has continued */
+
 /*
  * This works because the alignment is ok on all current architectures
  * but we leave open this being overridden in the future