Don't leave fds created with --log-file, --xml-file or --log-socket open

prepare_sink_fd and prepare_sink_socket will create a new file
descriptor for the output sink. finalize_sink_fd then copies the fd
to the safe range, so it doesn't conflict with any application fds.

If we created the original fd ourselves, it was a VgLogTo_File or
VgLogTo_Socket, not VgLogTo_Fd, finalize_sink_fd should close it.

Also close socket when connecting fails in VG_(connect_via_socket).

Add a testcase for --log-file and --xml-file which prints output to
/dev/stderr

https://bugs.kde.org/show_bug.cgi?id=202770
https://bugs.kde.org/show_bug.cgi?id=311655
https://bugs.kde.org/show_bug.cgi?id=488379

Co-authored-by: Alexandra Hájková <ahajkova@redhat.com

diff --git a/NEWS b/NEWS
index 7a8505cb503d96e49b153e29a2e23ba05749dd73..49d055bee09a6a76eb48cc9148198e0997b479a4 100644 (file)
--- a/NEWS
+++ b/NEWS
@@ -26,9 +26,11 @@ bugzilla (https://bugs.kde.org/enter_bug.cgi?product=valgrind) rather
 than mailing the developers (or mailing lists) directly -- bugs that
 are not entered into bugzilla tend to get forgotten about or ignored.
 
+202770  open fd at exit --log-socket=127.0.0.1:1500 with --track-fds=yes
 276780  An instruction in fftw (Fast Fourier Transform) is unhandled by
         valgrind: vex x86->IR: unhandled instruction bytes:
         0x66 0xF 0x3A 0x2
+311655  --log-file=FILE leads to apparent fd leak
 377966  arm64 unhandled instruction dc zva392146  aarch64: unhandled
         instruction 0xD5380001 (MRS rT, midr_el1)
 392146  aarch64: unhandled instruction 0xD5380001 (MRS rT, midr_el1)
@@ -42,6 +44,7 @@ are not entered into bugzilla tend to get forgotten about or ignored.
 487439  SIGILL in JDK11, JDK17
 487993  Alignment error when using Eigen with Valgrind and -m32
 488026  Use of `sizeof` instead of `strlen
+488379  --track-fds=yes errors that cannot be suppressed with --xml-file=
 488441  Add tests for --track-fds=yes --xml=yes and fd suppression tests
 
 To see details of a given bug, visit

diff --git a/coregrind/m_libcfile.c b/coregrind/m_libcfile.c
index 6098bc5813a8f040221e403e9da7aa0b467f4665..9635b80a6869549ea3aba6e8518c63fc28fcea95 100644 (file)
--- a/coregrind/m_libcfile.c
+++ b/coregrind/m_libcfile.c
@@ -1333,6 +1333,7 @@ Int VG_(connect_via_socket)( const HChar* str )
    res = my_connect(sd, &servAddr, sizeof(servAddr));
    if (res < 0) {
       /* connection failed */
+      VG_(close)(sd);
       return -2;
    }
 

diff --git a/coregrind/m_libcprint.c b/coregrind/m_libcprint.c
index c802f814038b8da7f94ab919b8f25ca323664048..593889da9d1bdc6980e232f26a734e812d3d50d2 100644 (file)
--- a/coregrind/m_libcprint.c
+++ b/coregrind/m_libcprint.c
@@ -425,6 +425,12 @@ static void finalize_sink_fd(OutputSink *sink, Int new_fd, Bool is_xml)
    } else {
       VG_(fcntl)(safe_fd, VKI_F_SETFD, VKI_FD_CLOEXEC);
       sink->fd = safe_fd;
+      /* If we created the new_fd (VgLogTo_File or VgLogTo_Socket), then we
+         don't need the original file descriptor open anymore. We only need
+         to keep it open if it was an existing fd given by the user (or
+         stderr).  */
+      if (sink->type != VgLogTo_Fd)
+         VG_(close)(new_fd);
    }
 }
 

diff --git a/none/tests/Makefile.am b/none/tests/Makefile.am
index 7caa8ca32270b19a57859809f49139089dfd0d8f..553a9c8655a189f429afd84b27f1820202185abb 100644 (file)
--- a/none/tests/Makefile.am
+++ b/none/tests/Makefile.am
@@ -251,7 +251,9 @@ EXTRA_DIST = \
        file_dclose_xml.stderr.exp file_dclose_xml.vgtest \
        file_dclose_xml.stderr.exp-nomain \
        file_dclose.supp file_dclose_sup.stderr.exp file_dclose_sup.vgtest \
-       double_close_range.stderr.exp double_close_range.vgtest
+       double_close_range.stderr.exp double_close_range.vgtest \
+       log-track-fds.stderr.exp log-track-fds.vgtest \
+       xml-track-fds.stderr.exp xml-track-fds.vgtest
 
 
 check_PROGRAMS = \

diff --git a/none/tests/log-track-fds.stderr.exp b/none/tests/log-track-fds.stderr.exp
new file mode 100644 (file)
index 0000000..e69de29

diff --git a/none/tests/log-track-fds.vgtest b/none/tests/log-track-fds.vgtest
new file mode 100644 (file)
index 0000000..dfebb5b
--- /dev/null
+++ b/none/tests/log-track-fds.vgtest
@@ -0,0 +1,4 @@
+# Simple test to make sure track-fds doesn't error on (internal) log-file
+# See https://bugs.kde.org/show_bug.cgi?id=311655
+prog: ../../tests/true
+vgopts: -q --track-fds=yes --log-file=/dev/stderr

diff --git a/none/tests/xml-track-fds.stderr.exp b/none/tests/xml-track-fds.stderr.exp
new file mode 100644 (file)
index 0000000..b06da9d
--- /dev/null
+++ b/none/tests/xml-track-fds.stderr.exp
@@ -0,0 +1,47 @@
+<?xml version="1.0"?>
+
+<valgrindoutput>
+
+<protocolversion>5</protocolversion>
+<protocoltool>none</protocoltool>
+
+<preamble>
+  <line>Nulgrind, the minimal Valgrind tool</line>
+  <line>Copyright...</line>
+  <line>Using Valgrind...</line>
+  <line>Command: ./../../tests/true</line>
+</preamble>
+
+<pid>...</pid>
+<ppid>...</ppid>
+<tool>none</tool>
+
+<args>
+  <vargv>
+    <exe>...</exe>
+    <arg>--command-line-only=yes</arg>
+    <arg>--memcheck:leak-check=no</arg>
+    <arg>--tool=none</arg>
+    <arg>--track-fds=yes</arg>
+    <arg>--xml=yes</arg>
+    <arg>--xml-file=/dev/stderr</arg>
+  </vargv>
+  <argv>
+    <exe>...</exe>
+  </argv>
+</args>
+
+<status>
+  <state>RUNNING</state>
+  <time>...</time>
+</status>
+
+
+<status>
+  <state>FINISHED</state>
+  <time>...</time>
+</status>
+
+
+</valgrindoutput>
+

diff --git a/none/tests/xml-track-fds.vgtest b/none/tests/xml-track-fds.vgtest
new file mode 100644 (file)
index 0000000..50f1a55
--- /dev/null
+++ b/none/tests/xml-track-fds.vgtest
@@ -0,0 +1,5 @@
+# Simple test to make sure track-fds doesn't error on (internal) xml-file
+# See https://bugs.kde.org/show_bug.cgi?id=488379
+prog: ../../tests/true
+vgopts: --track-fds=yes --xml=yes --xml-file=/dev/stderr
+stderr_filter: filter_xml