From aac23c9ce624f8dc51efeb644962d495d1c2a49b Mon Sep 17 00:00:00 2001 From: =?utf8?q?Alexandra=20H=C3=A1jkov=C3=A1?= Date: Wed, 20 Nov 2024 12:00:47 -0500 Subject: [PATCH] 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 --- coregrind/m_main.c | 10 +++++ coregrind/m_options.c | 1 + coregrind/m_syswrap/priv_syswrap-generic.h | 13 ++++++ coregrind/m_syswrap/syswrap-generic.c | 46 ++++++++++++++++++++++ coregrind/m_syswrap/syswrap-linux.c | 7 ++++ docs/xml/manual-core.xml | 11 ++++++ include/pub_tool_options.h | 2 + none/tests/Makefile.am | 7 +++- none/tests/cmdline1.stdout.exp | 1 + none/tests/cmdline2.stdout.exp | 1 + none/tests/filter_fdleak | 16 ++++++++ none/tests/track_new.c | 19 +++++++++ none/tests/track_new.stderr.exp | 10 +++++ none/tests/track_new.stderr.exp.debian32 | 10 +++++ none/tests/track_new.stdout.exp | 0 none/tests/track_new.vgtest | 4 ++ none/tests/use_after_close.stderr.exp | 2 +- 17 files changed, 157 insertions(+), 3 deletions(-) create mode 100644 none/tests/track_new.c create mode 100644 none/tests/track_new.stderr.exp create mode 100644 none/tests/track_new.stderr.exp.debian32 create mode 100644 none/tests/track_new.stdout.exp create mode 100644 none/tests/track_new.vgtest diff --git a/coregrind/m_main.c b/coregrind/m_main.c index 877e6b0b68..55ffd769b3 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 16452f2525..6f5a4d0458 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 b888a167cc..b24b6b9035 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 dea656aab2..1ab494c840 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 92771577bd..de710c97e4 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 3a2ce461cd..ffcb8d4bf5 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 32345dc6a0..fec61e30fe 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 ccad463cab..c2b36e33c2 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 464c58f42a..d2f3e5d6a8 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 fe526855d8..9a49757461 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 18a65b4568..5fbd74bcf4 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 0000000000..544ccacb3c --- /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 0000000000..23dec73bbc --- /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 0000000000..eb86871743 --- /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 0000000000..e69de29bb2 diff --git a/none/tests/track_new.vgtest b/none/tests/track_new.vgtest new file mode 100644 index 0000000000..f6f72d880d --- /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 75a8d66729..620a6b8d29 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 -- 2.39.5