From: Alexandra Hájková Date: Tue, 25 Jun 2024 15:27:16 +0000 (-0400) Subject: Add BadFdExtra core error. X-Git-Tag: VALGRIND_3_24_0~34 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=ff96f07bc9b97d15bbb05b470f06d46d52c190be;p=thirdparty%2Fvalgrind.git 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 76cec58be..a0b02faf3 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 4984d30ab..4bbcea024 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 09acb7cb7..66fbbfe72 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 5434a6836..26e14d494 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, " \n"); VG_(message)(Vg_UserMsg, "\n"); } + } else if (VG_(get_error_kind)(err) == FdBadUse) { + 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); + 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= 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= 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 e365125ba..d4bf2e7a8 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 924c40957..4ec294317 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 000000000..176d5bfa6 --- /dev/null +++ b/none/tests/fdbaduse.c @@ -0,0 +1,10 @@ +#include +#include + +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 000000000..c916fa7f6 --- /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 000000000..c7129a548 --- /dev/null +++ b/none/tests/fdbaduse.vgtest @@ -0,0 +1,3 @@ +prog: fdbaduse +prereq: test -x fdbaduse +vgopts: -q --track-fds=yes