From: Alexandra Hájková Date: Wed, 16 Oct 2024 17:38:48 +0000 (-0400) Subject: Report track-fd errors for fd used which was not opened or already closed X-Git-Tag: VALGRIND_3_24_0~4 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=22971a15d62df6351ab97ea064eebd9bdcb4cf37;p=thirdparty%2Fvalgrind.git Report track-fd errors for fd used which was not opened or already closed Add (optional) pathname, description, where_closed and where_opened fields to struct FdBadUse. Print those fields when set in fd_pp_Error. Add a new function ML_(find_OpenFd) that provides a recorded OpenFd given an fd (or NULL when the fd was never recorded). In ML_(fd_allowed) when using a file descriptor use ML_(find_OpenFd) to see if the fd was ever created, if not create an "was never created" FdBadUse error. If it was created, but already closed create an "was closed already", filling in as much details as we can. Add none/tests/use_after_close.vgtest to test, already closed, never created, invalid, double (double) close and invalid close issues. Adjust error message in none/tests/fdbaduse.stderr.exp. https://bugs.kde.org/show_bug.cgi?id=493418 --- diff --git a/NEWS b/NEWS index 2857a387d..54229dc57 100644 --- a/NEWS +++ b/NEWS @@ -76,6 +76,7 @@ are not entered into bugzilla tend to get forgotten about or ignored. 492214 statx(fd, NULL, AT_EMPTY_PATH) is supported since Linux 6.11 but not supported in valgrind 492663 Valgrind ignores debug info for some binaries +493418 Add bad fd usage errors for --track-fds in ML_(fd_allowed) 493454 Missing FUSE_COMPATIBLE_MAY_BLOCK markers 493507 direct readlink syscall from PRE handler is incompatible with FUSE_COMPATIBLE_MAY_BLOCK diff --git a/coregrind/m_syswrap/syswrap-generic.c b/coregrind/m_syswrap/syswrap-generic.c index 26e14d494..920c87a8a 100644 --- a/coregrind/m_syswrap/syswrap-generic.c +++ b/coregrind/m_syswrap/syswrap-generic.c @@ -593,6 +593,11 @@ struct BadCloseExtra { struct FdBadUse { Int fd; /* The file descriptor */ + HChar *pathname; /* NULL if not a regular file or unknown */ + HChar *description; /* Description of the file descriptor + might include the pathname */ + ExeContext *where_closed; /* record the last close of fd */ + ExeContext *where_opened; /* recordwhere the fd was opened */ }; struct NotClosedExtra { @@ -1166,8 +1171,25 @@ void fd_pp_Error (const Error *err) if (xml) VG_(emit)(" FdBadUse\n"); struct FdBadUse *nce = (struct FdBadUse *) VG_(get_error_extra)(err); - if (xml) VG_(emit)(" %d\n", nce->fd); - VG_(emit)("%sInvalid file descriptor %d%s\n", whatpre, nce->fd, whatpost); + const char *error_string = VG_(get_error_string)(err); + if (xml) { + VG_(emit)(" %d\n", nce->fd); + if (nce->pathname) + VG_(emit)(" %s\n", nce->pathname); + } + VG_(emit)("%sFile descriptor %d %s%s\n", whatpre, nce->fd, + error_string, whatpost); + /* If the file descriptor was never created we won't have + where_closed and where_opened. Only print them in a + use after close case. */ + if (nce->where_closed) { + VG_(emit)("%sPreviously closed%s\n", auxpre, auxpost); + VG_(pp_ExeContext)(nce->where_closed); + } + if (nce->where_opened) { + VG_(emit)("%sOriginally opened%s\n", auxpre, auxpost); + VG_(pp_ExeContext)(nce->where_opened); + } VG_(pp_ExeContext)(where); } else { vg_assert2 (False, "Unknown error kind: %d", @@ -1577,6 +1599,19 @@ static Addr do_brk ( Addr newbrk, ThreadId tid ) return VG_(brk_limit); } +const OpenFd *ML_(find_OpenFd)(Int fd) +{ + OpenFd *i = allocated_fds; + + while (i) { + if (i->fd == fd) + return i; + i = i->next; + } + + return NULL; +} + /* --------------------------------------------------------------------- Vet file descriptors for sanity @@ -1640,14 +1675,42 @@ Bool ML_(fd_allowed)(Int fd, const HChar *syscallname, ThreadId tid, client is exactly what we don't want. */ /* croak? */ + if (VG_(clo_track_fds) && allowed + && !isNewFd && (VG_(strcmp)("close", syscallname) != 0)) { + const OpenFd *openbadfd = ML_(find_OpenFd)(fd); + if (!openbadfd) { + /* File descriptor which was never created (or inherited). */ + struct FdBadUse badfd; + badfd.fd = fd; + badfd.pathname = NULL; + badfd.description = NULL; + badfd.where_opened = NULL; + badfd.where_closed = NULL; + VG_(maybe_record_error)(tid, FdBadUse, 0, + "was never created", &badfd); + + } else if (openbadfd->fd_closed) { + /* Already closed file descriptor is being used. */ + struct FdBadUse badfd; + badfd.fd = fd; + badfd.pathname = openbadfd->pathname; + badfd.description = openbadfd->description; + badfd.where_opened = openbadfd->where; + badfd.where_closed = openbadfd->where_closed; + VG_(maybe_record_error)(tid, FdBadUse, 0, + "was closed already", &badfd); + } + } if ((!allowed) && !isNewFd) { - // XXX FdBadUse might want to add more info if we are going to include - // already closed file descriptors, then we do have a bit more info if (VG_(clo_track_fds)) { struct FdBadUse badfd; badfd.fd = fd; + badfd.pathname = NULL; + badfd.description = NULL; + badfd.where_opened = NULL; + badfd.where_closed = NULL; VG_(maybe_record_error)(tid, FdBadUse, 0, - "invalid file descriptor", &badfd); + "Invalid file descriptor", &badfd); } else if (VG_(showing_core_warnings) ()) { // XXX legacy warnings, will be removed eventually VG_(message)(Vg_UserMsg, diff --git a/none/tests/Makefile.am b/none/tests/Makefile.am index 4ec294317..59be79e57 100644 --- a/none/tests/Makefile.am +++ b/none/tests/Makefile.am @@ -256,7 +256,8 @@ EXTRA_DIST = \ double_close_range.stderr.exp double_close_range.vgtest \ log-track-fds.stderr.exp log-track-fds.vgtest \ xml-track-fds.stderr.exp xml-track-fds.vgtest \ - fdbaduse.stderr.exp fdbaduse.vgtest + fdbaduse.stderr.exp fdbaduse.vgtest \ + use_after_close.stderr.exp use_after_close.vgtest check_PROGRAMS = \ @@ -311,7 +312,8 @@ check_PROGRAMS = \ sigprocmask \ socket_close \ file_dclose \ - fdbaduse + fdbaduse \ + use_after_close if HAVE_STATIC_LIBC if ! VGCONF_OS_IS_LINUX diff --git a/none/tests/fdbaduse.stderr.exp b/none/tests/fdbaduse.stderr.exp index 15d71f99a..a4772dbd9 100644 --- a/none/tests/fdbaduse.stderr.exp +++ b/none/tests/fdbaduse.stderr.exp @@ -1,3 +1,3 @@ -Invalid file descriptor -1 +File descriptor -1 Invalid file descriptor at 0x........: close (in /...libc...) by 0x........: main diff --git a/none/tests/use_after_close.c b/none/tests/use_after_close.c new file mode 100644 index 000000000..deb4e6888 --- /dev/null +++ b/none/tests/use_after_close.c @@ -0,0 +1,33 @@ +#include +#include +#include +#include + +int main(void) +{ + char *string = "bad\n"; + int fd = dup(2); + + /* OK. */ + write(fd, string, 4); + close(fd); + + /* Already closed. */ + write(fd, string, 4); + + /* Never created. */ + write(7, string, 4); + + /* Invalid. */ + write(-7, string, 4); + + /* Double double close. */ + close(fd); + + /* Invalid close. */ + close (-7); + + return 0; +} + + diff --git a/none/tests/use_after_close.stderr.exp b/none/tests/use_after_close.stderr.exp new file mode 100644 index 000000000..1ef31c655 --- /dev/null +++ b/none/tests/use_after_close.stderr.exp @@ -0,0 +1,28 @@ +bad +File descriptor 3 was closed already + Previously closed + at 0x........: close (in /...libc...) + by 0x........: main + Originally opened + at 0x........: dup (in /...libc...) + by 0x........: main + at 0x........: write (in /...libc...) + by 0x........: main +File descriptor 7 was never created + at 0x........: write (in /...libc...) + by 0x........: main +File descriptor -7 Invalid file descriptor + at 0x........: write (in /...libc...) + by 0x........: main +File descriptor ...: ... is already closed + at 0x........: close (in /...libc...) + by 0x........: main + Previously closed + at 0x........: close (in /...libc...) + by 0x........: main + Originally opened + at 0x........: dup (in /...libc...) + by 0x........: main +File descriptor -7 Invalid file descriptor + at 0x........: close (in /...libc...) + by 0x........: main diff --git a/none/tests/use_after_close.vgtest b/none/tests/use_after_close.vgtest new file mode 100644 index 000000000..eec5b9308 --- /dev/null +++ b/none/tests/use_after_close.vgtest @@ -0,0 +1,4 @@ +prog: use_after_close +prereq: test -x use_after_close +vgopts: -q --track-fds=yes +stderr_filter: filter_fdleak