Add BadFdExtra core error.

Introduce a new FdBadFd type with associated extra info struct.
Which for now just holds the fd number (no path or description).
fd_pp_Error and fd_update_extra have been updated to handle the
new type and produce xml when requested.

Rename showing_core_errors to showing_core_warning
(returns yes when the tools wants to show core errors,
-q isn't given and we aren't producing xml).

In ML_(fd_allowed) we now call VG_(maybe_record_error) to
generate a real error (that can be suppressed and shows up
in the xml output with full execution backtrace). For now
we also produce the legacy warnings when --track-fds=yes
isn't given.

Add none/tests/fdbaduse.vgtest to test the new FdBadUse
core error.

This is the first part of reporting bad fd usage errors.
We are also tracking already closed file descriptors which
should also produce errors like these. The current bad file
descriptors are just those that are negative or above the
current file limit of the process.

https://bugs.kde.org/show_bug.cgi?id=493418

diff --git a/NEWS b/NEWS
index 76cec58be9d0c0c7fc05eb5f90838b0e34507b47..a0b02faf31411d99abeb9e5968d8f56099a1a74a 100644 (file)
--- a/NEWS
+++ b/NEWS
@@ -10,6 +10,12 @@ AMD64/macOS 10.13 and nanoMIPS/Linux.
 
 * ==================== CORE CHANGES ===================
 
+* Bad file descriptor usage now generates a real error with
+  --track-fds=yes that is suppressible and shows up in the xml output
+  with full execution backtrace. The warnings shown without using the
+  option are deprecated and will be removed in a future valgrind
+  version.
+
 * ================== PLATFORM CHANGES =================
 
 * S390X added support for the DFLTCC instruction provided by the

diff --git a/coregrind/m_errormgr.c b/coregrind/m_errormgr.c
index 4984d30aba00dcd2e663abc51cc1474a47091aee..4bbcea02474cca4656a61cba92d3ddc2132adca8 100644 (file)
--- a/coregrind/m_errormgr.c
+++ b/coregrind/m_errormgr.c
@@ -297,9 +297,9 @@ void VG_(set_supp_extra)  ( Supp* su, void* extra )
 /*--- Helper fns                                           ---*/
 /*------------------------------------------------------------*/
 
-// Only show core errors if the tool wants to, we're not running with -q,
+// Only show core warnings if the tool wants to, we're not running with -q,
 // and were not outputting XML.
-Bool VG_(showing_core_errors)(void)
+Bool VG_(showing_core_warnings)(void)
 {
    return VG_(needs).core_errors && VG_(clo_verbosity) >= 1 && !VG_(clo_xml);
 }
@@ -967,7 +967,8 @@ Bool VG_(unique_error) ( ThreadId tid, ErrorKind ekind, Addr a, const HChar* s,
 
 static Bool is_fd_core_error (const Error *e)
 {
-   return e->ekind == FdBadClose || e->ekind == FdNotClosed;
+   return e->ekind == FdBadClose || e->ekind == FdNotClosed ||
+          e->ekind == FdBadUse;
 }
 
 static Bool core_eq_Error (VgRes res, const Error *e1, const Error *e2)
@@ -1017,6 +1018,8 @@ static const HChar *core_get_error_name(const Error *err)
       return "FdBadClose";
    case FdNotClosed:
       return "FdNotClosed";
+   case FdBadUse:
+      return "FdBadUse";
    default:
       VG_(umsg)("FATAL: unknown core error kind: %d\n", err->ekind );
       VG_(exit)(1);
@@ -1030,6 +1033,8 @@ static Bool core_error_matches_suppression(const Error* err, const Supp* su)
       return err->ekind == FdBadClose;
    case FdNotClosedSupp:
       return err->ekind == FdNotClosed;
+   case FdBadUse:
+      return err->ekind == FdBadUse;
    default:
       VG_(umsg)("FATAL: unknown core suppression kind: %d\n", su->skind );
       VG_(exit)(1);

diff --git a/coregrind/m_signals.c b/coregrind/m_signals.c
index 09acb7cb7ab9f6e70446e467a37a58d9bb928f71..66fbbfe721bb782fc2a3fc9c46efeed0af0c38d2 100644 (file)
--- a/coregrind/m_signals.c
+++ b/coregrind/m_signals.c
@@ -1336,13 +1336,13 @@ SysRes VG_(do_sys_sigaction) ( Int signo,
    return VG_(mk_SysRes_Success)( 0 );
 
   bad_signo:
-   if (VG_(showing_core_errors)() && !VG_(clo_xml)) {
+   if (VG_(showing_core_warnings)()) {
       VG_(umsg)("Warning: bad signal number %d in sigaction()\n", signo);
    }
    return VG_(mk_SysRes_Error)( VKI_EINVAL );
 
   bad_signo_reserved:
-   if (VG_(showing_core_errors)() && !VG_(clo_xml)) {
+   if (VG_(showing_core_warnings)()) {
       VG_(umsg)("Warning: ignored attempt to set %s handler in sigaction();\n",
                 VG_(signame)(signo));
       VG_(umsg)("         the %s signal is used internally by Valgrind\n", 
@@ -1351,7 +1351,7 @@ SysRes VG_(do_sys_sigaction) ( Int signo,
    return VG_(mk_SysRes_Error)( VKI_EINVAL );
 
   bad_sigkill_or_sigstop:
-   if (VG_(showing_core_errors)() && !VG_(clo_xml)) {
+   if (VG_(showing_core_warnings)()) {
       VG_(umsg)("Warning: ignored attempt to set %s handler in sigaction();\n",
                 VG_(signame)(signo));
       VG_(umsg)("         the %s signal is uncatchable\n", 

diff --git a/coregrind/m_syswrap/syswrap-generic.c b/coregrind/m_syswrap/syswrap-generic.c
index 5434a6836978a4c0ce73546bc04812f566bd1015..26e14d494353972fde125bdf834e2c361b79a8c6 100644 (file)
--- a/coregrind/m_syswrap/syswrap-generic.c
+++ b/coregrind/m_syswrap/syswrap-generic.c
@@ -591,6 +591,10 @@ struct BadCloseExtra {
    ExeContext *where_opened;      /* recordwhere the fd  was opened */
 };
 
+struct FdBadUse {
+  Int fd;                        /* The file descriptor */
+};
+
 struct NotClosedExtra {
    Int fd;
    HChar *pathname;
@@ -1136,7 +1140,7 @@ void fd_pp_Error (const Error *err)
       }
       VG_(emit)("%sFile descriptor %d: %s is already closed%s\n",
                 whatpre, bce->fd, bce->description, whatpost);
-      VG_(pp_ExeContext)( VG_(get_error_where)(err) );
+      VG_(pp_ExeContext)( where );
       VG_(emit)("%sPreviously closed%s\n", auxpre, auxpost);
       VG_(pp_ExeContext)(bce->where_closed);
       VG_(emit)("%sOriginally opened%s\n", auxpre, auxpost);
@@ -1158,6 +1162,13 @@ void fd_pp_Error (const Error *err)
          VG_(message)(Vg_UserMsg, "   <inherited from parent>\n");
          VG_(message)(Vg_UserMsg, "\n");
       }
+   } else if (VG_(get_error_kind)(err) == FdBadUse) {
+      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);
+      VG_(pp_ExeContext)(where);
    } else {
       vg_assert2 (False, "Unknown error kind: %d",
                   VG_(get_error_kind)(err));
@@ -1172,6 +1183,8 @@ UInt fd_update_extra (const Error *err)
       return sizeof (struct BadCloseExtra);
    else if (VG_(get_error_kind)(err) == FdNotClosed)
       return sizeof (struct NotClosedExtra);
+   else if (VG_(get_error_kind)(err) == FdBadUse)
+      return sizeof (struct FdBadUse);
    else {
       vg_assert2 (False, "Unknown error kind: %d",
                   VG_(get_error_kind)(err));
@@ -1627,22 +1640,34 @@ Bool ML_(fd_allowed)(Int fd, const HChar *syscallname, ThreadId tid,
       client is exactly what we don't want.  */
 
    /* croak? */
-   if ((!allowed) && VG_(showing_core_errors)() ) {
-      VG_(message)(Vg_UserMsg,
-         "Warning: invalid file descriptor %d in syscall %s()\n",
-         fd, syscallname);
-      if (fd == VG_(log_output_sink).fd && VG_(log_output_sink).fd >= 0)
-        VG_(message)(Vg_UserMsg, 
+   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;
+         VG_(maybe_record_error)(tid, FdBadUse, 0,
+                                 "invalid file descriptor", &badfd);
+      } else if (VG_(showing_core_warnings) ()) {
+         // XXX legacy warnings, will be removed eventually
+         VG_(message)(Vg_UserMsg,
+                      "Warning: invalid file descriptor %d in syscall %s()\n",
+                      fd, syscallname);
+      }
+
+      if (VG_(showing_core_warnings) ()) {
+         if (fd == VG_(log_output_sink).fd && VG_(log_output_sink).fd >= 0)
+            VG_(message)(Vg_UserMsg, 
             "   Use --log-fd=<number> to select an alternative log fd.\n");
-      if (fd == VG_(xml_output_sink).fd && VG_(xml_output_sink).fd >= 0)
-        VG_(message)(Vg_UserMsg, 
+         if (fd == VG_(xml_output_sink).fd && VG_(xml_output_sink).fd >= 0)
+            VG_(message)(Vg_UserMsg, 
             "   Use --xml-fd=<number> to select an alternative XML "
             "output fd.\n");
-      // DDD: consider always printing this stack trace, it's useful.
-      // Also consider also making this a proper core error, ie.
-      // suppressible and all that.
-      if (VG_(clo_verbosity) > 1) {
-         VG_(get_and_pp_StackTrace)(tid, VG_(clo_backtrace_size));
+
+         // XXX This is the legacy warning, will be removed eventually
+         if (VG_(clo_verbosity) > 1 && !VG_(clo_track_fds)) {
+            VG_(get_and_pp_StackTrace)(tid, VG_(clo_backtrace_size));
+         }
       }
    }
 

diff --git a/coregrind/pub_core_errormgr.h b/coregrind/pub_core_errormgr.h
index e365125ba85dbe46acd05bdc8afdd0e4e5751b31..d4bf2e7a8adfdda289731e7118133ce75c32028a 100644 (file)
--- a/coregrind/pub_core_errormgr.h
+++ b/coregrind/pub_core_errormgr.h
@@ -46,6 +46,7 @@ typedef
       ThreadErr = -1,
       FdBadClose = -2,
       FdNotClosed = -3,
+      FdBadUse = -4,
    }
    CoreErrorKind;
 
@@ -77,7 +78,7 @@ extern void VG_(show_error_counts_as_XML) ( void );
 
 extern Bool VG_(is_action_requested)      ( const HChar* action, Bool* clo );
 
-extern Bool VG_(showing_core_errors)      ( void );
+extern Bool VG_(showing_core_warnings)    ( void );
 
 extern UInt VG_(get_n_errs_found)         ( void );
 extern UInt VG_(get_n_errs_shown)         ( void );

diff --git a/none/tests/Makefile.am b/none/tests/Makefile.am
index 924c409575b10e4acb5087cabca5b838f95c7a95..4ec29431752cd89c50cc06cd08a8b59c83080c24 100644 (file)
--- a/none/tests/Makefile.am
+++ b/none/tests/Makefile.am
@@ -255,7 +255,8 @@ EXTRA_DIST = \
        file_dclose.supp file_dclose_sup.stderr.exp file_dclose_sup.vgtest \
        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
+       xml-track-fds.stderr.exp xml-track-fds.vgtest \
+       fdbaduse.stderr.exp fdbaduse.vgtest
 
 
 check_PROGRAMS = \
@@ -309,7 +310,8 @@ check_PROGRAMS = \
        process_vm_readv_writev \
        sigprocmask \
        socket_close \
-       file_dclose
+       file_dclose \
+       fdbaduse
 
 if HAVE_STATIC_LIBC
 if ! VGCONF_OS_IS_LINUX

diff --git a/none/tests/fdbaduse.c b/none/tests/fdbaduse.c
new file mode 100644 (file)
index 0000000..176d5bf
--- /dev/null
+++ b/none/tests/fdbaduse.c
@@ -0,0 +1,10 @@
+#include <stdio.h>
+#include <unistd.h>
+
+int main (int argc, char **argv)
+{
+  close(-1);
+
+  return 0;
+}
+

diff --git a/none/tests/fdbaduse.stderr.exp b/none/tests/fdbaduse.stderr.exp
new file mode 100644 (file)
index 0000000..c916fa7
--- /dev/null
+++ b/none/tests/fdbaduse.stderr.exp
@@ -0,0 +1,3 @@
+Invalid file descriptor -1
+   at 0x........: close (in /...libc...)
+   by 0x........: main (fdbaduse.c:6)

diff --git a/none/tests/fdbaduse.vgtest b/none/tests/fdbaduse.vgtest
new file mode 100644 (file)
index 0000000..c7129a5
--- /dev/null
+++ b/none/tests/fdbaduse.vgtest
@@ -0,0 +1,3 @@
+prog: fdbaduse
+prereq: test -x fdbaduse
+vgopts: -q --track-fds=yes