valgrind-developers Mailing List for Valgrind, an open-source memory debugger (Page 34)

SourceForge Headquarters 225 Broadway Suite 1600 San Diego, CA 92101 +1 (858) 422-6466

https://sourceware.org/git/gitweb.cgi?p=valgrind.git;h=ff96f07bc9b97d15bbb05b470f06d46d52c190be

commit ff96f07bc9b97d15bbb05b470f06d46d52c190be
Author: Alexandra Hájková <aha...@re...>
Date:   Tue Jun 25 11:27:16 2024 -0400

    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:
---
 NEWS                                  |  6 ++++
 coregrind/m_errormgr.c                | 11 ++++++--
 coregrind/m_signals.c                 |  6 ++--
 coregrind/m_syswrap/syswrap-generic.c | 53 ++++++++++++++++++++++++++---------
 coregrind/pub_core_errormgr.h         |  3 +-
 none/tests/Makefile.am                |  6 ++--
 none/tests/fdbaduse.c                 | 10 +++++++
 none/tests/fdbaduse.stderr.exp        |  3 ++
 none/tests/fdbaduse.vgtest            |  3 ++
 9 files changed, 78 insertions(+), 23 deletions(-)

diff --git a/NEWS b/NEWS
index 76cec58be9..a0b02faf31 100644
--- 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 4984d30aba..4bbcea0247 100644
--- 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 09acb7cb7a..66fbbfe721 100644
--- 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 5434a68369..26e14d4943 100644
--- 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 e365125ba8..d4bf2e7a8a 100644
--- 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 924c409575..4ec2943175 100644
--- 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
index 0000000000..176d5bfa67
--- /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
index 0000000000..c916fa7f60
--- /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
index 0000000000..c7129a5485
--- /dev/null
+++ b/none/tests/fdbaduse.vgtest
@@ -0,0 +1,3 @@
+prog: fdbaduse
+prereq: test -x fdbaduse
+vgopts: -q --track-fds=yes




2002	Jan	Feb	Mar	Apr	May	Jun	Jul	Aug	Sep (1)	Oct (122)	Nov (152)	Dec (69)
2003	Jan (6)	Feb (25)	Mar (73)	Apr (82)	May (24)	Jun (25)	Jul (10)	Aug (11)	Sep (10)	Oct (54)	Nov (203)	Dec (182)
2004	Jan (307)	Feb (305)	Mar (430)	Apr (312)	May (187)	Jun (342)	Jul (487)	Aug (637)	Sep (336)	Oct (373)	Nov (441)	Dec (210)
2005	Jan (385)	Feb (480)	Mar (636)	Apr (544)	May (679)	Jun (625)	Jul (810)	Aug (838)	Sep (634)	Oct (521)	Nov (965)	Dec (543)
2006	Jan (494)	Feb (431)	Mar (546)	Apr (411)	May (406)	Jun (322)	Jul (256)	Aug (401)	Sep (345)	Oct (542)	Nov (308)	Dec (481)
2007	Jan (427)	Feb (326)	Mar (367)	Apr (255)	May (244)	Jun (204)	Jul (223)	Aug (231)	Sep (354)	Oct (374)	Nov (497)	Dec (362)
2008	Jan (322)	Feb (482)	Mar (658)	Apr (422)	May (476)	Jun (396)	Jul (455)	Aug (267)	Sep (280)	Oct (253)	Nov (232)	Dec (304)
2009	Jan (486)	Feb (470)	Mar (458)	Apr (423)	May (696)	Jun (461)	Jul (551)	Aug (575)	Sep (134)	Oct (110)	Nov (157)	Dec (102)
2010	Jan (226)	Feb (86)	Mar (147)	Apr (117)	May (107)	Jun (203)	Jul (193)	Aug (238)	Sep (300)	Oct (246)	Nov (23)	Dec (75)
2011	Jan (133)	Feb (195)	Mar (315)	Apr (200)	May (267)	Jun (293)	Jul (353)	Aug (237)	Sep (278)	Oct (611)	Nov (274)	Dec (260)
2012	Jan (303)	Feb (391)	Mar (417)	Apr (441)	May (488)	Jun (655)	Jul (590)	Aug (610)	Sep (526)	Oct (478)	Nov (359)	Dec (372)
2013	Jan (467)	Feb (226)	Mar (391)	Apr (281)	May (299)	Jun (252)	Jul (311)	Aug (352)	Sep (481)	Oct (571)	Nov (222)	Dec (231)
2014	Jan (185)	Feb (329)	Mar (245)	Apr (238)	May (281)	Jun (399)	Jul (382)	Aug (500)	Sep (579)	Oct (435)	Nov (487)	Dec (256)
2015	Jan (338)	Feb (357)	Mar (330)	Apr (294)	May (191)	Jun (108)	Jul (142)	Aug (261)	Sep (190)	Oct (54)	Nov (83)	Dec (22)
2016	Jan (49)	Feb (89)	Mar (33)	Apr (50)	May (27)	Jun (34)	Jul (53)	Aug (53)	Sep (98)	Oct (206)	Nov (93)	Dec (53)
2017	Jan (65)	Feb (82)	Mar (102)	Apr (86)	May (187)	Jun (67)	Jul (23)	Aug (93)	Sep (65)	Oct (45)	Nov (35)	Dec (17)
2018	Jan (26)	Feb (35)	Mar (38)	Apr (32)	May (8)	Jun (43)	Jul (27)	Aug (30)	Sep (43)	Oct (42)	Nov (38)	Dec (67)
2019	Jan (32)	Feb (37)	Mar (53)	Apr (64)	May (49)	Jun (18)	Jul (14)	Aug (53)	Sep (25)	Oct (30)	Nov (49)	Dec (31)
2020	Jan (87)	Feb (45)	Mar (37)	Apr (51)	May (99)	Jun (36)	Jul (11)	Aug (14)	Sep (20)	Oct (24)	Nov (40)	Dec (23)
2021	Jan (14)	Feb (53)	Mar (85)	Apr (15)	May (19)	Jun (3)	Jul (14)	Aug (1)	Sep (57)	Oct (73)	Nov (56)	Dec (22)
2022	Jan (3)	Feb (22)	Mar (6)	Apr (55)	May (46)	Jun (39)	Jul (15)	Aug (9)	Sep (11)	Oct (34)	Nov (20)	Dec (36)
2023	Jan (79)	Feb (41)	Mar (99)	Apr (169)	May (48)	Jun (16)	Jul (16)	Aug (57)	Sep (83)	Oct (89)	Nov (97)	Dec (30)
2024	Jan (25)	Feb (73)	Mar (76)	Apr (122)	May (46)	Jun (44)	Jul (27)	Aug (30)	Sep (33)	Oct (67)	Nov (91)	Dec (70)
2025	Jan (44)	Feb (36)	Mar (85)	Apr (100)	May (138)	Jun (55)	Jul (107)	Aug (54)	Sep	Oct	Nov	Dec

valgrind-developers Mailing List for Valgrind, an open-source memory debugger (Page 34)

valgrind-developers — Discussion list for developers of valgrind