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 2857a387d1e1bd6b21c16c9b26d528536f0f7559..54229dc57c7765336de5f12343c7e532e7b0d4bd 100644 (file)
--- 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 26e14d494353972fde125bdf834e2c361b79a8c6..920c87a8ae79d173e7cff94d4d79b6fd90e0436a 100644 (file)
--- 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)("  <kind>FdBadUse</kind>\n");
       struct FdBadUse *nce = (struct FdBadUse *)
          VG_(get_error_extra)(err);
-      if (xml) VG_(emit)("  <fd>%d</fd>\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)("  <fd>%d</fd>\n", nce->fd);
+        if (nce->pathname)
+            VG_(emit)("  <path>%s</path>\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 4ec29431752cd89c50cc06cd08a8b59c83080c24..59be79e57920e1c0a6cd58ff99e64d73f73a458b 100644 (file)
--- 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 15d71f99aa7bb0a25bd8f4be7776406343df5c34..a4772dbd99c53045d025927776593d649fd79327 100644 (file)
--- 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 (file)
index 0000000..deb4e68
--- /dev/null
+++ b/none/tests/use_after_close.c
@@ -0,0 +1,33 @@
+#include<stdio.h>
+#include <stdlib.h>
+#include <unistd.h>
+#include <fcntl.h>
+
+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 (file)
index 0000000..1ef31c6
--- /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 (file)
index 0000000..eec5b93
--- /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