Menu
▾
▴
[Valgrind-developers] [valgrind] Add "yes" argument for the --modify-fds option.
|
From: Mark W. <ma...@so...> - 2025-05-22 14:47:38
|
https://sourceware.org/git/gitweb.cgi?p=valgrind.git;h=ebd7dd5ea9504e0d8490507fd2b894647477b085 commit ebd7dd5ea9504e0d8490507fd2b894647477b085 Author: Alexandra Hájková <aha...@re...> Date: Tue May 6 06:50:44 2025 -0400 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: --- .gitignore | 1 + NEWS | 1 + coregrind/m_main.c | 10 ++++++---- coregrind/m_options.c | 2 +- coregrind/m_syswrap/priv_syswrap-generic.h | 15 ++++++++------- docs/xml/manual-core.xml | 6 ++++-- include/pub_tool_options.h | 5 +++++ none/tests/Makefile.am | 8 +++++--- none/tests/cmdline1.stdout.exp | 2 +- none/tests/cmdline2.stdout.exp | 2 +- none/tests/track_high.stderr.exp | 8 ++++++++ none/tests/track_high.vgtest | 4 ++++ none/tests/track_std.c | 31 ++++++++++++++++++++++++++++++ none/tests/track_yes.stderr.exp | 9 +++++++++ none/tests/track_yes.vgtest | 4 ++++ 15 files changed, 89 insertions(+), 19 deletions(-) diff --git a/.gitignore b/.gitignore index 5264bdd29a..8cabb96df4 100644 --- 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 d9f5fa9031..1450dfba82 100644 --- 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 ff82e3a505..4da156fb94 100644 --- 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 6f5a4d0458..e70ba08e8f 100644 --- 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 b24b6b9035..eb815840d9 100644 --- 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 ffcb8d4bf5..7d18d46f39 100644 --- 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 fec61e30fe..021f888bec 100644 --- 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 6305044ca6..18924b34f3 100644 --- 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 d2f3e5d6a8..06a679a111 100644 --- 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 9a49757461..d7914ae010 100644 --- 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 index 0000000000..f9a605f10e --- /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 index 0000000000..c5eb8f5461 --- /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 index 0000000000..2cf01de8f0 --- /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 index 0000000000..92c790f6c0 --- /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 index 0000000000..5c55038fd5 --- /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 + |
