Add --modify-fds=[no|high] option

Normally a newly recreated file descriptor gets the lowest number
available. This might cause old file descriptor numbers to be reused
and hides bad file descriptor accesses (because the old number is
new again).

When enabled, when the program opens a new file descriptor,
the highest available file descriptor is returned instead of the
lowest one.

Add the none/tests/track_new.stderr.exp test to test this new option.

Adjust none/tests/filter_fdleak to filter the track_new.vgtest,
removing some internal glibc functions from the backtraces and remove
symbol versioning. The output of the use_after_close test also had to
be adjusted. Also adjust the none/tests/cmdline1 and
none/tests/cmdline2 output as the new --modify-fds=no|high is
displayed.

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

diff --git a/coregrind/m_main.c b/coregrind/m_main.c
index 877e6b0b68776e0643b287a4174955e2fc5a2f0b..55ffd769b35cc37bcf9001939520a4f9bdb77ec8 100644 (file)
--- a/coregrind/m_main.c
+++ b/coregrind/m_main.c
@@ -115,6 +115,7 @@ static void usage_NORETURN ( int need_help )
 "           startup exit abexit valgrindabexit all none\n"
 "    --track-fds=no|yes|all    track open file descriptors? [no]\n"
 "                              all includes reporting inherited file descriptors\n"
+"    --modify-fds=no|high      modify newly open file descriptors? [no]\n"
 "    --time-stamp=no|yes       add timestamps to log messages? [no]\n"
 "    --log-fd=<number>         log messages to file descriptor [2=stderr]\n"
 "    --log-file=<file>         log messages to <file>\n"
@@ -646,6 +647,15 @@ static void process_option (Clo_Mode mode,
          VG_(fmsg_bad_option)(arg,
             "Bad argument, should be 'yes', 'all' or 'no'\n");
    }
+   else if VG_STR_CLO(arg, "--modify-fds",         tmp_str) {
+      if (VG_(strcmp)(tmp_str, "high") == 0)
+         VG_(clo_modify_fds) = 1;
+      else if (VG_(strcmp)(tmp_str, "no") == 0)
+         VG_(clo_modify_fds) = 0;
+      else
+         VG_(fmsg_bad_option)(arg,
+            "Bad argument, should be 'high' or 'no'\n");
+   }
    else if VG_BOOL_CLOM(cloPD, arg, "--trace-children",   VG_(clo_trace_children)) {}
    else if VG_BOOL_CLOM(cloPD, arg, "--child-silent-after-fork",
                         VG_(clo_child_silent_after_fork)) {}

diff --git a/coregrind/m_options.c b/coregrind/m_options.c
index 16452f252565f5d0f359b5ccc36f8cd861848268..6f5a4d04582dd80727a773e455c962baf8cb95c4 100644 (file)
--- a/coregrind/m_options.c
+++ b/coregrind/m_options.c
@@ -182,6 +182,7 @@ XArray *VG_(clo_req_tsyms);  // array of strings
 Bool   VG_(clo_run_libc_freeres) = True;
 Bool   VG_(clo_run_cxx_freeres) = True;
 UInt   VG_(clo_track_fds)      = 0;
+UInt   VG_(clo_modify_fds)      = 0;
 Bool   VG_(clo_show_below_main)= False;
 Bool   VG_(clo_keep_debuginfo) = False;
 Bool   VG_(clo_show_emwarns)   = False;

diff --git a/coregrind/m_syswrap/priv_syswrap-generic.h b/coregrind/m_syswrap/priv_syswrap-generic.h
index b888a167ccb492ffa9cd69e677e8f76b64352dba..b24b6b9035f1b2ff5783e1c5103da43cad9c27a3 100644 (file)
--- a/coregrind/m_syswrap/priv_syswrap-generic.h
+++ b/coregrind/m_syswrap/priv_syswrap-generic.h
@@ -73,6 +73,8 @@ extern Bool ML_(fd_recorded)(Int fd);
 // Returned string must not be modified nor free'd.
 extern const HChar *ML_(find_fd_recorded_by_fd)(Int fd);
 
+extern int ML_(get_next_new_fd)(Int fd);
+
 // Used when killing threads -- we must not kill a thread if it's the thread
 // that would do Valgrind's final cleanup and output.
 extern
@@ -337,6 +339,17 @@ extern SysRes ML_(generic_PRE_sys_mmap)         ( TId, UW, UW, UW, UW, UW, Off64
 #undef UW
 #undef SR
 
+/* Helper macro for POST handlers that return a new file in RES.
+   If possible sets RES (through SET_STATUS_Success) to a new
+   (not yet seem before) file descriptor.  */
+#define POST_newFd_RES                       \
+  do {                                       \
+    if (VG_(clo_modify_fds) == 1) {           \
+      int newFd = ML_(get_next_new_fd)(RES); \
+      if (newFd != RES)                      \
+        SET_STATUS_Success(newFd);           \
+    }                                        \
+  } while (0)
 
 /////////////////////////////////////////////////////////////////
 

diff --git a/coregrind/m_syswrap/syswrap-generic.c b/coregrind/m_syswrap/syswrap-generic.c
index dea656aab2701ba5254680d30b6129943f504b86..1ab494c84007841660d90c85e74c162a73438009 100644 (file)
--- a/coregrind/m_syswrap/syswrap-generic.c
+++ b/coregrind/m_syswrap/syswrap-generic.c
@@ -552,6 +552,49 @@ static OpenFd *allocated_fds = NULL;
 
 /* Count of open file descriptors. */
 static Int fd_count = 0;
+/* Last used file descriptor for "new" fds.
+   Used (and updated) in get_next_new_fd to find a new (not yet used)
+   fd number to return in syscall wrappers.  */
+static Int last_new_fd = 0;
+
+/* Replace (dup2) the fd with the highest file descriptor available.
+   Close the fd and return the newly created file descriptor on  success.
+   Keep track of the last_new_fd and return the initial fd on failure. */
+int ML_(get_next_new_fd)(int fd)
+{
+   int next_new_fd;
+
+   /* Check if last_new needs to wrap around.  */
+   if (last_new_fd == 0)
+     last_new_fd = VG_(fd_hard_limit);
+
+   next_new_fd = last_new_fd - 1;
+
+   /* Find the next fd number not in use. If we have to wrap around,
+      just use fd itself.  */
+   while (next_new_fd >= 0 && ML_(fd_recorded)(next_new_fd))
+      next_new_fd--;
+   if (next_new_fd < 0)
+     next_new_fd = fd;
+
+   /* Duplicate and close the existing fd if needed.  */
+   if (next_new_fd != fd) {
+      SysRes res = VG_(dup2)(fd, next_new_fd);
+      if (!sr_isError(res))
+         VG_(close)(fd);
+      else
+        next_new_fd = fd;
+
+      /* Record what the last new fd was we returned.  */
+      last_new_fd = next_new_fd;
+   } else {
+      /* There was no lower "new" fd found. Lets wrap around for
+         the next round.  */
+      last_new_fd = VG_(fd_hard_limit);
+   }
+
+   return next_new_fd;
+}
 
 
 Int ML_(get_fd_count)(void)
@@ -3693,6 +3736,7 @@ PRE(sys_dup)
 POST(sys_dup)
 {
    vg_assert(SUCCESS);
+   POST_newFd_RES;
    if (!ML_(fd_allowed)(RES, "dup", tid, True)) {
       VG_(close)(RES);
       SET_STATUS_Failure( VKI_EMFILE );
@@ -4561,6 +4605,7 @@ PRE(sys_open)
 POST(sys_open)
 {
    vg_assert(SUCCESS);
+   POST_newFd_RES;
    if (!ML_(fd_allowed)(RES, "open", tid, True)) {
       VG_(close)(RES);
       SET_STATUS_Failure( VKI_EMFILE );
@@ -4627,6 +4672,7 @@ PRE(sys_creat)
 POST(sys_creat)
 {
    vg_assert(SUCCESS);
+   POST_newFd_RES;
    if (!ML_(fd_allowed)(RES, "creat", tid, True)) {
       VG_(close)(RES);
       SET_STATUS_Failure( VKI_EMFILE );

diff --git a/coregrind/m_syswrap/syswrap-linux.c b/coregrind/m_syswrap/syswrap-linux.c
index 92771577bd34fa16e84705b41edad71155b84d2e..de710c97e43551d5e39983369f2b79d311081a19 100644 (file)
--- a/coregrind/m_syswrap/syswrap-linux.c
+++ b/coregrind/m_syswrap/syswrap-linux.c
@@ -2103,6 +2103,7 @@ PRE(sys_epoll_create)
 POST(sys_epoll_create)
 {
    vg_assert(SUCCESS);
+   POST_newFd_RES;
    if (!ML_(fd_allowed)(RES, "epoll_create", tid, True)) {
       VG_(close)(RES);
       SET_STATUS_Failure( VKI_EMFILE );
@@ -2120,6 +2121,7 @@ PRE(sys_epoll_create1)
 POST(sys_epoll_create1)
 {
    vg_assert(SUCCESS);
+   POST_newFd_RES;
    if (!ML_(fd_allowed)(RES, "epoll_create1", tid, True)) {
       VG_(close)(RES);
       SET_STATUS_Failure( VKI_EMFILE );
@@ -2234,6 +2236,7 @@ PRE(sys_eventfd)
 }
 POST(sys_eventfd)
 {
+   POST_newFd_RES;
    if (!ML_(fd_allowed)(RES, "eventfd", tid, True)) {
       VG_(close)(RES);
       SET_STATUS_Failure( VKI_EMFILE );
@@ -2250,6 +2253,7 @@ PRE(sys_eventfd2)
 }
 POST(sys_eventfd2)
 {
+   POST_newFd_RES;
    if (!ML_(fd_allowed)(RES, "eventfd2", tid, True)) {
       VG_(close)(RES);
       SET_STATUS_Failure( VKI_EMFILE );
@@ -2799,6 +2803,7 @@ PRE(sys_fanotify_init)
 
 POST(sys_fanotify_init)
 {
+   POST_newFd_RES;
    vg_assert(SUCCESS);
    if (!ML_(fd_allowed)(RES, "fanotify_init", tid, True)) {
       VG_(close)(RES);
@@ -2847,6 +2852,7 @@ PRE(sys_inotify_init)
 POST(sys_inotify_init)
 {
    vg_assert(SUCCESS);
+   POST_newFd_RES;
    if (!ML_(fd_allowed)(RES, "inotify_init", tid, True)) {
       VG_(close)(RES);
       SET_STATUS_Failure( VKI_EMFILE );
@@ -5945,6 +5951,7 @@ PRE(sys_openat)
 POST(sys_openat)
 {
    vg_assert(SUCCESS);
+   POST_newFd_RES;
    if (!ML_(fd_allowed)(RES, "openat", tid, True)) {
       VG_(close)(RES);
       SET_STATUS_Failure( VKI_EMFILE );

diff --git a/docs/xml/manual-core.xml b/docs/xml/manual-core.xml
index 3a2ce461cdcca02e86c9ec4cf88e1c041916858b..ffcb8d4bf5127e67327acbfebacd775b9fdb7651 100644 (file)
--- a/docs/xml/manual-core.xml
+++ b/docs/xml/manual-core.xml
@@ -901,6 +901,17 @@ in most cases.  We group the available options by rough categories.</para>
     </listitem>
   </varlistentry>
 
+  <varlistentry id="opt.modify-fds" xreflabel="--modify-fds">
+    <term>
+      <option><![CDATA[--modify-fds=<no|high> [default: no] ]]></option>
+    </term>
+    <listitem>
+      <para>When enabled, when the program opens a new file descriptor,
+      the highest available file descriptor is returned instead of the
+      lowest one.</para>
+    </listitem>
+  </varlistentry>
+
   <varlistentry id="opt.time-stamp" xreflabel="--time-stamp">
     <term>
       <option><![CDATA[--time-stamp=<yes|no> [default: no] ]]></option>

diff --git a/include/pub_tool_options.h b/include/pub_tool_options.h
index 32345dc6a059ae790b421c65619e8393c8e90eed..fec61e30fed71d4d00b5abd16ed65510d2366a71 100644 (file)
--- a/include/pub_tool_options.h
+++ b/include/pub_tool_options.h
@@ -419,6 +419,8 @@ extern Bool VG_(clo_keep_debuginfo);
 /* Track open file descriptors? 0 = No, 1 = Yes, 2 = All (including std)  */
 extern UInt  VG_(clo_track_fds);
 
+extern UInt  VG_(clo_modify_fds);
+
 
 /* Used to expand file names.  "option_name" is the option name, eg.
    "--log-file".  'format' is what follows, eg. "cachegrind.out.%p".  In

diff --git a/none/tests/Makefile.am b/none/tests/Makefile.am
index ccad463cab07184cd9ea1a52911d05fb87a6a974..c2b36e33c2e12de2156bbd41d5a2776848f1c26f 100644 (file)
--- a/none/tests/Makefile.am
+++ b/none/tests/Makefile.am
@@ -267,7 +267,9 @@ EXTRA_DIST = \
        log-track-fds.stderr.exp log-track-fds.vgtest \
        xml-track-fds.stderr.exp xml-track-fds.vgtest \
        fdbaduse.stderr.exp fdbaduse.vgtest \
-       use_after_close.stderr.exp use_after_close.vgtest
+       use_after_close.stderr.exp use_after_close.vgtest \
+       track_new.stderr.exp track_new.stdout.exp \
+       track_new.stderr.exp.debian32 track_new.vgtest
 
 
 check_PROGRAMS = \
@@ -325,7 +327,8 @@ check_PROGRAMS = \
        socket_close \
        file_dclose \
        fdbaduse \
-        use_after_close
+        use_after_close \
+       track_new
 
 if HAVE_STATIC_LIBC
 if ! VGCONF_OS_IS_LINUX

diff --git a/none/tests/cmdline1.stdout.exp b/none/tests/cmdline1.stdout.exp
index 464c58f42aa7f8f8bfaaecadad65b381b00ce3ea..d2f3e5d6a8df377423dfd5baa8af0e6ea3e863f2 100644 (file)
--- a/none/tests/cmdline1.stdout.exp
+++ b/none/tests/cmdline1.stdout.exp
@@ -30,6 +30,7 @@ usage: valgrind [options] prog-and-args
            startup exit abexit valgrindabexit all none
     --track-fds=no|yes|all    track open file descriptors? [no]
                               all includes reporting inherited file descriptors
+    --modify-fds=no|high      modify newly open file descriptors? [no]
     --time-stamp=no|yes       add timestamps to log messages? [no]
     --log-fd=<number>         log messages to file descriptor [2=stderr]
     --log-file=<file>         log messages to <file>

diff --git a/none/tests/cmdline2.stdout.exp b/none/tests/cmdline2.stdout.exp
index fe526855d8f8615e1328b417335d903c41d75f53..9a497574613e8a8629b9cb5ccb5bbf1729bb9349 100644 (file)
--- a/none/tests/cmdline2.stdout.exp
+++ b/none/tests/cmdline2.stdout.exp
@@ -30,6 +30,7 @@ usage: valgrind [options] prog-and-args
            startup exit abexit valgrindabexit all none
     --track-fds=no|yes|all    track open file descriptors? [no]
                               all includes reporting inherited file descriptors
+    --modify-fds=no|high      modify newly open file descriptors? [no]
     --time-stamp=no|yes       add timestamps to log messages? [no]
     --log-fd=<number>         log messages to file descriptor [2=stderr]
     --log-file=<file>         log messages to <file>

diff --git a/none/tests/filter_fdleak b/none/tests/filter_fdleak
index 18a65b4568cff1776412ff4a16c6204d2635430c..5fbd74bcf4b0a2084a445681059e559430ca9f76 100755 (executable)
--- a/none/tests/filter_fdleak
+++ b/none/tests/filter_fdleak
@@ -14,6 +14,7 @@ perl -p -e 's/^Open (AF_UNIX socket|file descriptor) [0-9]*: (\/private)?\/tmp\/
 perl -p -e 's/^Open file descriptor [0-9]*: .*/Open file descriptor ...: .../' |
 perl -p -e 's/^Open file descriptor [0-9]*:$/Open file descriptor ...:/' |
 perl -p -e 's/File descriptor [0-9]*: .* is already closed/File descriptor ...: ... is already closed/' |
+perl -p -e 's/File descriptor [0-9]* was closed already/File descriptor was closed already/' |
 perl -p -e 's/127.0.0.1:[0-9]*/127.0.0.1:.../g' |
 perl -p -e 's/socket\.c:[1-9][0-9]*/in \/...libc.../' |
 
@@ -32,6 +33,18 @@ perl -p -e 's/open \(open64\.c:[1-9][0-9]*\)/creat (in \/...libc...)/' |
 perl -p -e "s/: open \(/: creat (/" |
 # arm64 write resolved to file:line with debuginfo
 perl -p -e "s/write\.c:[1-9][0-9]*/in \/...libc.../" |
+#ppc64le
+sed 's/__dprintf.*/dprintf/' |
+
+# Remove "internal" _functions
+sed '/by 0x........: _/d' |
+
+# Remove symbol versioning
+sed -E 's/ ([a-zA-Z0-9_]+)@@?[A-Z0-9._]+/ \1/' |
+
+perl -p -e "s/\(dprintf.c:[0-9]*\)/(in \/...libc...)/" |
+perl -p -e "s/\(open.c:[0-9]*\)/(in \/...libc...)/" |
+perl -p -e "s/\(lseek(?:64)?.c:[0-9]*\)/(in \/...libc...)/" |
 
 # FreeBSD specific fdleak filters
 perl -p -e 's/ _close / close /;s/ _openat / creat /;s/ _write/ write/;s/internet/AF_INET socket 4: 127.0.0.1:... <-> 127.0.0.1:.../' |
@@ -50,6 +63,9 @@ perl -p -0 -e 's/(Open[^\n]*\n)(   (at|by)[^\n]*\n)+/$1   ...\n/gs' |
 
 sed "s/by 0x........: (below main)/by 0x........: main/" |
 sed "s/by 0x........: main (.*)/by 0x........: main/" |
+sed "s/by 0x........: open (.*)/by 0x........: open/" |
+sed "s/by 0x........: write (.*)/by 0x........: write/" |
+sed "s/by 0x........: close (.*)/by 0x........: close/" |
 
 # With glibc debuginfo installed we might see syscall-template.S,
 # dup2.c close.c or creat64.c

diff --git a/none/tests/track_new.c b/none/tests/track_new.c
new file mode 100644 (file)
index 0000000..544ccac
--- /dev/null
+++ b/none/tests/track_new.c
@@ -0,0 +1,19 @@
+#include <fcntl.h>
+#include <stdio.h>
+#include <unistd.h>
+
+int
+main ()
+{
+  int oldfd = open ("foobar.txt", O_RDWR|O_CREAT, S_IRUSR | S_IWUSR);
+  /*... do something with oldfd ...*/
+  close (oldfd);
+
+  /* Lets open another file... */
+  int newfd = open ("foobad.txt", O_RDWR|O_CREAT, S_IRUSR | S_IWUSR);
+  /* ... oops we are using the wrong fd (but same number...) */
+  dprintf (oldfd, "some new text\n");
+
+  close (newfd);
+  return 0;
+}

diff --git a/none/tests/track_new.stderr.exp b/none/tests/track_new.stderr.exp
new file mode 100644 (file)
index 0000000..23dec73
--- /dev/null
+++ b/none/tests/track_new.stderr.exp
@@ -0,0 +1,10 @@
+File descriptor was closed already
+   at 0x........: write (in /...libc...)
+   by 0x........: dprintf (in /...libc...)
+   by 0x........: main
+ Previously closed
+   at 0x........: close (in /...libc...)
+   by 0x........: main
+ Originally opened
+   at 0x........: creat (in /...libc...)
+   by 0x........: main

diff --git a/none/tests/track_new.stderr.exp.debian32 b/none/tests/track_new.stderr.exp.debian32
new file mode 100644 (file)
index 0000000..eb86871
--- /dev/null
+++ b/none/tests/track_new.stderr.exp.debian32
@@ -0,0 +1,10 @@
+File descriptor was closed already
+   at 0x........: llseek (in /...libc...)
+   by 0x........: dprintf (in /...libc...)
+   by 0x........: main
+ Previously closed
+   at 0x........: close (in /...libc...)
+   by 0x........: main
+ Originally opened
+   at 0x........: creat (in /...libc...)
+   by 0x........: main

diff --git a/none/tests/track_new.stdout.exp b/none/tests/track_new.stdout.exp
new file mode 100644 (file)
index 0000000..e69de29

diff --git a/none/tests/track_new.vgtest b/none/tests/track_new.vgtest
new file mode 100644 (file)
index 0000000..f6f72d8
--- /dev/null
+++ b/none/tests/track_new.vgtest
@@ -0,0 +1,4 @@
+prog: track_new
+prereq: test -x track_new
+vgopts: -q --track-fds=yes --modify-fds=high
+stderr_filter: filter_fdleak

diff --git a/none/tests/use_after_close.stderr.exp b/none/tests/use_after_close.stderr.exp
index 75a8d6672949d6aec88fd5d3167aad6fb5a405ce..620a6b8d292e6df5ea53d18bf924bd2312461e90 100644 (file)
--- a/none/tests/use_after_close.stderr.exp
+++ b/none/tests/use_after_close.stderr.exp
@@ -1,5 +1,5 @@
 bad
-File descriptor 3 was closed already
+File descriptor was closed already
    at 0x........: write (in /...libc...)
    by 0x........: main
  Previously closed