From: Alexandra Hájková Date: Wed, 20 Nov 2024 17:00:47 +0000 (-0500) Subject: Add --modify-fds=[no|high] option X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=aac23c9ce624f8dc51efeb644962d495d1c2a49b;p=thirdparty%2Fvalgrind.git 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 877e6b0b6..55ffd769b 100644 --- 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= log messages to file descriptor [2=stderr]\n" " --log-file= log messages to \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 16452f252..6f5a4d045 100644 --- 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 b888a167c..b24b6b903 100644 --- 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 dea656aab..1ab494c84 100644 --- 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 92771577b..de710c97e 100644 --- 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 3a2ce461c..ffcb8d4bf 100644 --- 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. + + + + + + When enabled, when the program opens a new file descriptor, + the highest available file descriptor is returned instead of the + lowest one. + + + diff --git a/include/pub_tool_options.h b/include/pub_tool_options.h index 32345dc6a..fec61e30f 100644 --- 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 ccad463ca..c2b36e33c 100644 --- 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 464c58f42..d2f3e5d6a 100644 --- 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= log messages to file descriptor [2=stderr] --log-file= log messages to diff --git a/none/tests/cmdline2.stdout.exp b/none/tests/cmdline2.stdout.exp index fe526855d..9a4975746 100644 --- 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= log messages to file descriptor [2=stderr] --log-file= log messages to diff --git a/none/tests/filter_fdleak b/none/tests/filter_fdleak index 18a65b456..5fbd74bcf 100755 --- 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 index 000000000..544ccacb3 --- /dev/null +++ b/none/tests/track_new.c @@ -0,0 +1,19 @@ +#include +#include +#include + +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 000000000..23dec73bb --- /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 000000000..eb8687174 --- /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 000000000..e69de29bb diff --git a/none/tests/track_new.vgtest b/none/tests/track_new.vgtest new file mode 100644 index 000000000..f6f72d880 --- /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 75a8d6672..620a6b8d2 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