Add "yes" argument for the --modify-fds option.

Use --modify-fds=yes to restrict the option from affecting
the 0/1/2 file descriptors as they're often used for
stdin/tdout/stderr redirection.

The new possibility is named "yes" because "yes" is used
as the default in general. The default behaviour of the --modify-fds
option is then such, that highest available file descriptor is returned
execept when the lowest stdin/stdout/stderr (0, 1, 2) are available.

For example, if we want to redirect stdout to stderr by closing stdout
(file descriptor 1) and then calling dup (), file descriptor 1 will be
returned and not the highest number available. This is because the
following is a common pattern to redirect stdout to stderr:

close (1);
/* stdout becomes stderr */
ret = dup (2);

Add none/tests/track_yes.vgtest and none/tests/track_high.vgtest
tests to test --modify-fds=yes/high behave as expected.

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

diff --git a/.gitignore b/.gitignore
index 5264bdd29a5422d5b6ea1ff530788e8374bcd70f..8cabb96df49fa5068a4252aa0531d8f90368c706 100644 (file)
--- a/.gitignore
+++ b/.gitignore
@@ -1675,6 +1675,7 @@
 /none/tests/tls
 /none/tests/track-fds-exec-children
 /none/tests/track_new
+/none/tests/track_std
 /none/tests/unit_debuglog
 /none/tests/use_after_close
 /none/tests/valgrind_cpp_test

diff --git a/NEWS b/NEWS
index d9f5fa903174f06cae7f29528594a30e48c0d53a..1450dfba820250ce4293197f18ba1f45e22d262d 100644 (file)
--- a/NEWS
+++ b/NEWS
@@ -31,6 +31,7 @@ are not entered into bugzilla tend to get forgotten about or ignored.
 504101  Add a "vgstack" script
 504177  FILE DESCRIPTORS banner shows when closing some inherited fds
 501741  syscall cachestat not wrapped
+502359  Add --modify-fds=yes option
 503969  Make test results of make ltpchecks compatible with bunsen
 504265  FreeBSD: missing syscall wrappers for fchroot and setcred
 504341  Valgrind killed by LTP syscall testcase setrlimit05

diff --git a/coregrind/m_main.c b/coregrind/m_main.c
index ff82e3a505d813e623d1b47c4b42aa4815c084eb..4da156fb94e6c52e665dcd345d1a2141aa98fa65 100644 (file)
--- a/coregrind/m_main.c
+++ b/coregrind/m_main.c
@@ -115,7 +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"
+"    --modify-fds=no|yes|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"
@@ -649,12 +649,14 @@ static void process_option (Clo_Mode mode,
    }
    else if VG_STR_CLO(arg, "--modify-fds",         tmp_str) {
       if (VG_(strcmp)(tmp_str, "high") == 0)
-         VG_(clo_modify_fds) = 1;
+         VG_(clo_modify_fds) = VG_MODIFY_FD_HIGH;
+      else if (VG_(strcmp)(tmp_str, "yes") == 0)
+        VG_(clo_modify_fds) = VG_MODIFY_FD_YES;
       else if (VG_(strcmp)(tmp_str, "no") == 0)
-         VG_(clo_modify_fds) = 0;
+         VG_(clo_modify_fds) = VG_MODIFY_FD_NO;
       else
          VG_(fmsg_bad_option)(arg,
-            "Bad argument, should be 'high' or 'no'\n");
+            "Bad argument, should be 'high', 'yes', 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",

diff --git a/coregrind/m_options.c b/coregrind/m_options.c
index 6f5a4d04582dd80727a773e455c962baf8cb95c4..e70ba08e8fd1ccdc2098fdd53c3d3e73439c8066 100644 (file)
--- a/coregrind/m_options.c
+++ b/coregrind/m_options.c
@@ -182,7 +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;
+UInt   VG_(clo_modify_fds)      = VG_MODIFY_FD_NO;
 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 b24b6b9035f1b2ff5783e1c5103da43cad9c27a3..eb815840d9c21b1c8c08205c22974340b0771303 100644 (file)
--- a/coregrind/m_syswrap/priv_syswrap-generic.h
+++ b/coregrind/m_syswrap/priv_syswrap-generic.h
@@ -342,13 +342,14 @@ extern SysRes ML_(generic_PRE_sys_mmap)         ( TId, UW, UW, UW, UW, UW, Off64
 /* 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);           \
-    }                                        \
+#define POST_newFd_RES                                       \
+  do {                                                       \
+    if ((VG_(clo_modify_fds) == VG_MODIFY_FD_YES && RES > 2) \
+        ||  (VG_(clo_modify_fds) == VG_MODIFY_FD_HIGH)) {    \
+       int newFd = ML_(get_next_new_fd)(RES);                \
+       if (newFd != RES)                                     \
+          SET_STATUS_Success(newFd);                         \
+    }                                                        \
   } while (0)
 
 /////////////////////////////////////////////////////////////////

diff --git a/docs/xml/manual-core.xml b/docs/xml/manual-core.xml
index ffcb8d4bf5127e67327acbfebacd775b9fdb7651..7d18d46f391a935ac7c51e5f55266cda5e4df688 100644 (file)
--- a/docs/xml/manual-core.xml
+++ b/docs/xml/manual-core.xml
@@ -903,12 +903,14 @@ in most cases.  We group the available options by rough categories.</para>
 
   <varlistentry id="opt.modify-fds" xreflabel="--modify-fds">
     <term>
-      <option><![CDATA[--modify-fds=<no|high> [default: no] ]]></option>
+      <option><![CDATA[--modify-fds=<no|yes|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>
+      lowest one. Use <option>yes</option> to restrict the feature from
+      the 0/1/2 file descriptors as they're often used for stdout/stderr
+      redirection.</para>
     </listitem>
   </varlistentry>
 

diff --git a/include/pub_tool_options.h b/include/pub_tool_options.h
index fec61e30fed71d4d00b5abd16ed65510d2366a71..021f888bec18f9dffe2036574d43527416448b2e 100644 (file)
--- a/include/pub_tool_options.h
+++ b/include/pub_tool_options.h
@@ -419,6 +419,11 @@ extern Bool VG_(clo_keep_debuginfo);
 /* Track open file descriptors? 0 = No, 1 = Yes, 2 = All (including std)  */
 extern UInt  VG_(clo_track_fds);
 
+/* Whether to adjust file descriptor numbers. Yes does for all nonstd file
+   descriptors. High does for all file descriptors.  */
+#define VG_MODIFY_FD_NO 0
+#define VG_MODIFY_FD_YES 1
+#define VG_MODIFY_FD_HIGH 2
 extern UInt  VG_(clo_modify_fds);
 
 

diff --git a/none/tests/Makefile.am b/none/tests/Makefile.am
index 6305044ca60e987d76cbec71fbef009857a99d98..18924b34f3ee381eec2db822a15ab2820cb24d6f 100644 (file)
--- a/none/tests/Makefile.am
+++ b/none/tests/Makefile.am
@@ -272,8 +272,9 @@ EXTRA_DIST = \
        fdbaduse.stderr.exp fdbaduse.vgtest \
        use_after_close.stderr.exp use_after_close.vgtest \
        track_new.stderr.exp track_new.stdout.exp \
-       track_new.vgtest track_new.stderr.exp-illumos
-
+       track_new.vgtest track_new.stderr.exp-illumos \
+       track_yes.vgtest track_high.vgtest \
+       track_yes.stderr.exp track_high.stderr.exp
 
 check_PROGRAMS = \
        args \
@@ -331,7 +332,8 @@ check_PROGRAMS = \
        file_dclose \
        fdbaduse \
         use_after_close \
-       track_new
+       track_new \
+       track_std
 
 if HAVE_STATIC_LIBC
 if ! VGCONF_OS_IS_LINUX

diff --git a/none/tests/cmdline1.stdout.exp b/none/tests/cmdline1.stdout.exp
index d2f3e5d6a8df377423dfd5baa8af0e6ea3e863f2..06a679a11112dd6b57302a4e656201da5c6498ad 100644 (file)
--- a/none/tests/cmdline1.stdout.exp
+++ b/none/tests/cmdline1.stdout.exp
@@ -30,7 +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]
+    --modify-fds=no|yes|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 9a497574613e8a8629b9cb5ccb5bbf1729bb9349..d7914ae0101bbc53f2a262e4ca21b0aff5756917 100644 (file)
--- a/none/tests/cmdline2.stdout.exp
+++ b/none/tests/cmdline2.stdout.exp
@@ -30,7 +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]
+    --modify-fds=no|yes|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/track_high.stderr.exp b/none/tests/track_high.stderr.exp
new file mode 100644 (file)
index 0000000..f9a605f
--- /dev/null
+++ b/none/tests/track_high.stderr.exp
@@ -0,0 +1,8 @@
+FILE DESCRIPTORS: 3 open (1 inherited) at exit.
+Open file descriptor ...: ...
+   ...
+
+Open file descriptor ...: /dev/null
+   ...
+
+

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

diff --git a/none/tests/track_std.c b/none/tests/track_std.c
new file mode 100644 (file)
index 0000000..2cf01de
--- /dev/null
+++ b/none/tests/track_std.c
@@ -0,0 +1,31 @@
+#include <fcntl.h>
+#include <unistd.h>
+#include <sys/stat.h>
+#include <string.h>
+
+int
+main (void)
+{
+  char buf[20];
+  size_t nbytes;
+  int ret;
+
+  /* close stdin */
+  close (0);
+  /* open /dev/null as new stdin */
+  (void)open ("/dev/null", O_RDONLY);
+  /* redirect stdout as stderr */
+  close (1);
+  /* stdout becomes stderr */
+  ret = dup (2);
+
+  if (ret == 1) {
+    strcpy(buf, "hello world\n");
+    nbytes = strlen(buf);
+
+    /* should come out on stderr */
+    write (1, buf, nbytes);
+  }
+
+  return 0;
+}

diff --git a/none/tests/track_yes.stderr.exp b/none/tests/track_yes.stderr.exp
new file mode 100644 (file)
index 0000000..92c790f
--- /dev/null
+++ b/none/tests/track_yes.stderr.exp
@@ -0,0 +1,9 @@
+hello world
+FILE DESCRIPTORS: 3 open (1 inherited) at exit.
+Open file descriptor ...: ...
+   ...
+
+Open file descriptor ...: /dev/null
+   ...
+
+

diff --git a/none/tests/track_yes.vgtest b/none/tests/track_yes.vgtest
new file mode 100644 (file)
index 0000000..5c55038
--- /dev/null
+++ b/none/tests/track_yes.vgtest
@@ -0,0 +1,4 @@
+prog: track_std
+vgopts: -q --track-fds=yes --modify-fds=yes
+stderr_filter: filter_fdleak
+