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 877e6b0..55ffd76 100644 --- a/coregrind/m_main.c +++ b/coregrind/m_main.c
@@ -115,6 +115,7 @@ " 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 @@ 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 16452f2..6f5a4d0 100644 --- a/coregrind/m_options.c +++ b/coregrind/m_options.c
@@ -182,6 +182,7 @@ 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 b888a16..b24b6b9 100644 --- a/coregrind/m_syswrap/priv_syswrap-generic.h +++ b/coregrind/m_syswrap/priv_syswrap-generic.h
@@ -73,6 +73,8 @@ // 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 @@ #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 dea656a..1ab494c 100644 --- a/coregrind/m_syswrap/syswrap-generic.c +++ b/coregrind/m_syswrap/syswrap-generic.c
@@ -552,6 +552,49 @@ /* 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 @@ 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 @@ 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 @@ 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 9277157..de710c9 100644 --- a/coregrind/m_syswrap/syswrap-linux.c +++ b/coregrind/m_syswrap/syswrap-linux.c
@@ -2103,6 +2103,7 @@ 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 @@ 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 @@ } 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 @@ } 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 @@ 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 @@ 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 @@ 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 3a2ce46..ffcb8d4 100644 --- a/docs/xml/manual-core.xml +++ b/docs/xml/manual-core.xml
@@ -901,6 +901,17 @@ </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 32345dc..fec61e3 100644 --- a/include/pub_tool_options.h +++ b/include/pub_tool_options.h
@@ -419,6 +419,8 @@ /* 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 ccad463..c2b36e3 100644 --- a/none/tests/Makefile.am +++ b/none/tests/Makefile.am
@@ -267,7 +267,9 @@ 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 @@ 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 464c58f..d2f3e5d 100644 --- a/none/tests/cmdline1.stdout.exp +++ b/none/tests/cmdline1.stdout.exp
@@ -30,6 +30,7 @@ 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 fe52685..9a49757 100644 --- a/none/tests/cmdline2.stdout.exp +++ b/none/tests/cmdline2.stdout.exp
@@ -30,6 +30,7 @@ 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 18a65b4..5fbd74b 100755 --- a/none/tests/filter_fdleak +++ b/none/tests/filter_fdleak
@@ -14,6 +14,7 @@ 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 \(/: 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 @@ 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 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 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 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 index 0000000..e69de29 --- /dev/null +++ b/none/tests/track_new.stdout.exp
diff --git a/none/tests/track_new.vgtest b/none/tests/track_new.vgtest new file mode 100644 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 75a8d66..620a6b8 100644 --- 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