[Valgrind-developers] [valgrind/VALGRIND_3_23_BRANCH] Close both internal pipe fds after VG_(fork) in parent and child

[Mark W. <ma...@so...>]

https://sourceware.org/git/gitweb.cgi?p=valgrind.git;h=0ced5269236c527d2e288b773e3684b6b1673ec2

commit 0ced5269236c527d2e288b773e3684b6b1673ec2
Author: Mark Wielaard <ma...@kl...>
Date:   Mon Jun 17 00:27:12 2024 +0200

    Close both internal pipe fds after VG_(fork) in parent and child
    
    An VG_fork() creates a pipe between parent and child to syncronize the
    two processes. The parent wants to register the child pid before the
    child can run. This is done in register_sigchld_ignore.
    
    Make sure both the parent and the child close both the read and write
    file descriptors so none leak.
    
    https://bugs.kde.org/show_bug.cgi?id=479661
    (cherry picked from commit 1263471efdf8405cb0f1a767c6af73bf2eaf7160)

Diff:
---
 NEWS                                          |  1 +
 coregrind/m_libcproc.c                        |  7 +++++++
 none/tests/Makefile.am                        |  3 +++
 none/tests/track-fds-exec-children.c          | 13 +++++++++++++
 none/tests/track-fds-exec-children.stderr.exp |  0
 none/tests/track-fds-exec-children.vgtest     |  3 +++
 6 files changed, 27 insertions(+)

diff --git a/NEWS b/NEWS
index 10b5ae3195..94789a04ba 100644
--- a/NEWS
+++ b/NEWS
@@ -8,6 +8,7 @@ The following bugs have been fixed or resolved on this branch.
 202770  open fd at exit --log-socket=127.0.0.1:1500 with --track-fds=yes
 311655  --log-file=FILE leads to apparent fd leak
 453044  gbserver_tests failures in aarch64
+479661  Valgrind leaks file descriptors
 486180  [MIPS] 'VexGuestArchState' has no member named 'guest_IP_AT_SYSCALL'
 486293  memccpy false positives
 486569  linux inotify_init syscall wrapper missing POST entry in syscall_table
diff --git a/coregrind/m_libcproc.c b/coregrind/m_libcproc.c
index 11dabe768f..8422e9d118 100644
--- a/coregrind/m_libcproc.c
+++ b/coregrind/m_libcproc.c
@@ -905,6 +905,8 @@ static void register_sigchld_ignore ( Int pid, Int fds[2])
       return;
 
    if (pid == 0) {
+      /* We are the child, close writing fd that we don't use.  */
+      VG_(close)(fds[1]);
       /* Before proceeding, ensure parent has recorded child PID in map
          of SIGCHLD to ignore */
       while (child_wait == 1)
@@ -916,6 +918,7 @@ static void register_sigchld_ignore ( Int pid, Int fds[2])
          }
       }
 
+      /* Now close reading fd.  */
       VG_(close)(fds[0]);
       return;
    }
@@ -926,11 +929,15 @@ static void register_sigchld_ignore ( Int pid, Int fds[2])
       ht_sigchld_ignore = VG_(HT_construct)("ht.sigchld.ignore");
    VG_(HT_add_node)(ht_sigchld_ignore, n);
 
+   /* We are the parent process, close read fd that we don't use.  */
+   VG_(close)(fds[0]);
+
    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");
 
+   /* Now close writing fd.  */
    VG_(close)(fds[1]);
 }
 
diff --git a/none/tests/Makefile.am b/none/tests/Makefile.am
index 532cc7632a..1751ad3fa2 100644
--- a/none/tests/Makefile.am
+++ b/none/tests/Makefile.am
@@ -223,6 +223,7 @@ EXTRA_DIST = \
 	threadederrno.vgtest \
 	timestamp.stderr.exp timestamp.vgtest \
 	tls.vgtest tls.stderr.exp tls.stdout.exp  \
+	track-fds-exec-children.vgtest track-fds-exec-children.stderr.exp \
 	unit_debuglog.stderr.exp unit_debuglog.vgtest \
 	vgprintf.stderr.exp vgprintf.vgtest \
 	vgprintf_nvalgrind.stderr.exp vgprintf_nvalgrind.vgtest \
@@ -276,6 +277,7 @@ check_PROGRAMS = \
 	tls \
 	tls.so \
 	tls2.so \
+	track-fds-exec-children \
 	unit_debuglog \
 	valgrind_cpp_test \
 	vgprintf \
@@ -414,6 +416,7 @@ if VGCONF_OS_IS_DARWIN
 else
  tls2_so_LDFLAGS	= -shared
 endif
+track_fds_exec_children_SOURCES = track-fds-exec-children.c
 
 vgprintf_nvalgrind_SOURCES = vgprintf.c
 vgprintf_nvalgrind_CFLAGS = ${AM_CFLAGS} -DNVALGRIND
diff --git a/none/tests/track-fds-exec-children.c b/none/tests/track-fds-exec-children.c
new file mode 100644
index 0000000000..7209ee73d5
--- /dev/null
+++ b/none/tests/track-fds-exec-children.c
@@ -0,0 +1,13 @@
+#include <unistd.h>
+#include <sys/wait.h>
+
+int main()
+{
+  pid_t pid = fork ();
+  if (pid == 0)
+    execlp("true", "true", NULL);
+
+  // Wait till true succeeds
+  wait (NULL);
+  return 0;
+}
diff --git a/none/tests/track-fds-exec-children.stderr.exp b/none/tests/track-fds-exec-children.stderr.exp
new file mode 100644
index 0000000000..e69de29bb2
diff --git a/none/tests/track-fds-exec-children.vgtest b/none/tests/track-fds-exec-children.vgtest
new file mode 100644
index 0000000000..aa926a6290
--- /dev/null
+++ b/none/tests/track-fds-exec-children.vgtest
@@ -0,0 +1,3 @@
+env: DEBUGINFOD_URLS=file:/dev/null
+prog: track-fds-exec-children
+vgopts: -q --track-fds=yes --trace-children=yes