From: Mark Wielaard Date: Wed, 20 Mar 2024 05:44:23 +0000 (-0400) Subject: Add core errors and use them to implement file descriptor tracker X-Git-Tag: VALGRIND_3_23_0~29 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=34e9e4957ada1ba0a51266faee23f61724935fcb;p=thirdparty%2Fvalgrind.git Add core errors and use them to implement file descriptor tracker All the tool error callbacks now have a core error equivalent. core errors are negative (while tool errors are positive). There are two new ones for tracking issues with file descriptors. FdBadClose (-2) and FdNotClosed (-3). Add following core error functions with delegates to file descriptor specific functions (implemented in syswrap-generic): - core_eq_Error (fd_eq_Error) Compares core errors to detect duplicates - core_before_pp_Error (fd_before_pp_Error) Currently prints nothing for known core errors and exists with FATAL for unknown core errors - core_pp_Error (fd_pp_Error) For FdBadClose prints the backtraces for the file descriptor was opened, where it was originally closed and where it was closed again. For FdNotClosed prints where the file descriptor was opened. - core_update_extra (fd_update_extra) Returns the size of the BadCloseExtra or FdNotClosedExtra struct which data needs to be saved (the fd number, pathname/description and previous backtraces). We now accept the error (ExeContext) where to be NULL. This is necessary for reporting not closed file descriptors when the descriptor is inherited from the parent (so wasn't actually created and doesn't have a 'where' in the current process code). All the testcases still pass since the (stderr) output is the same. But now they count as "real" errors. And so --error-exitcode does now also work for file descriptor errors or leaks. https://bugs.kde.org/show_bug.cgi?id=362680 Co-authored-by: Alexandra Hájková --- diff --git a/NEWS b/NEWS index dcca47cd3..66ea47486 100644 --- a/NEWS +++ b/NEWS @@ -40,6 +40,7 @@ are not entered into bugzilla tend to get forgotten about or ignored. 283429 ARM leak checking needs CLEAR_CALLER_SAVED_REGS 281059 Cannot connect to Oracle using valgrind +362680 --error-exitcode not honored when file descriptor leaks are found 369723 __builtin_longjmp not supported in clang/llvm on Android arm64 target 390269 unhandled amd64-darwin syscall: unix:464 (openat_nocancel) 401284 False positive "Source and destination overlap in strncat" diff --git a/coregrind/m_errormgr.c b/coregrind/m_errormgr.c index bac3f712a..d3004c316 100644 --- a/coregrind/m_errormgr.c +++ b/coregrind/m_errormgr.c @@ -46,6 +46,7 @@ #include "pub_core_tooliface.h" #include "pub_core_translate.h" // for VG_(translate)() #include "pub_core_xarray.h" // VG_(xaprintf) et al +#include "pub_core_syswrap.h" // for core callbacks (Fds) #define DEBUG_ERRORMGR 0 // set to 1 for heavyweight tracing @@ -95,6 +96,11 @@ static UInt n_supp_contexts = 0; /* forwards ... */ static Supp* is_suppressible_error ( const Error* err ); +static Bool core_eq_Error (VgRes, const Error*, const Error*); +static void core_before_pp_Error (const Error*); +static void core_pp_Error (const Error*); +static UInt core_update_extra (const Error*); + static ThreadId last_tid_printed = 1; /* Stats: number of searches of the error list initiated. */ @@ -297,20 +303,17 @@ static Bool eq_Error ( VgRes res, const Error* e1, const Error* e2 ) if (!VG_(eq_ExeContext)(res, e1->where, e2->where)) return False; - switch (e1->ekind) { - //(example code, see comment on CoreSuppKind above) - //case ThreadErr: - // vg_assert(VG_(needs).core_errors); - // return - default: - if (VG_(needs).tool_errors) { - return VG_TDICT_CALL(tool_eq_Error, res, e1, e2); - } else { - VG_(printf)("\nUnhandled error type: %u. VG_(needs).tool_errors\n" - "probably needs to be set.\n", - (UInt)e1->ekind); - VG_(core_panic)("unhandled error type"); - } + if (e1->ekind >= 0) { + if (VG_(needs).tool_errors) { + return VG_TDICT_CALL(tool_eq_Error, res, e1, e2); + } else { + VG_(printf)("\nUnhandled error type: %u. VG_(needs).tool_errors\n" + "probably needs to be set.\n", + (UInt)e1->ekind); + VG_(core_panic)("unhandled error type"); + } + } else { + return core_eq_Error(res, e1, e2); } } @@ -370,13 +373,15 @@ static void gen_suppression(const Error* err) vg_assert(err); ec = VG_(get_error_where)(err); - vg_assert(ec); + vg_assert(ec); // XXX - name = VG_TDICT_CALL(tool_get_error_name, err); - if (NULL == name) { - VG_(umsg)("(%s does not allow error to be suppressed)\n", - VG_(details).name); - return; + if (err->ekind >= 0) { + name = VG_TDICT_CALL(tool_get_error_name, err); + if (NULL == name) { + VG_(umsg)("(%s does not allow error to be suppressed)\n", + VG_(details).name); + return; + } } /* In XML mode, we also need to print the plain text version of the @@ -585,7 +590,7 @@ static void pp_Error ( const Error* err, Bool allow_db_attach, Bool xml, Bool co { /* If this fails, you probably specified your tool's method dictionary incorrectly. */ - vg_assert(VG_(needs).tool_errors); + vg_assert(VG_(needs).tool_errors || err->ekind < 0 /* core errors */); if (xml) { @@ -597,7 +602,10 @@ static void pp_Error ( const Error* err, Bool allow_db_attach, Bool xml, Bool co || VG_(clo_gen_suppressions) == 2 /* for all errors */ ); /* Pre-show it to the tool */ - VG_TDICT_CALL( tool_before_pp_Error, err ); + if (err->ekind >= 0) + VG_TDICT_CALL( tool_before_pp_Error, err ); + else + core_before_pp_Error (err); /* standard preamble */ VG_(printf_xml)("\n"); @@ -609,7 +617,10 @@ static void pp_Error ( const Error* err, Bool allow_db_attach, Bool xml, Bool co } /* actually print it */ - VG_TDICT_CALL( tool_pp_Error, err ); + if (err->ekind >= 0) + VG_TDICT_CALL( tool_pp_Error, err ); + else + core_pp_Error (err); if (VG_(clo_gen_suppressions) > 0) gen_suppression(err); @@ -622,7 +633,10 @@ static void pp_Error ( const Error* err, Bool allow_db_attach, Bool xml, Bool co if (VG_(clo_error_markers)[0]) VG_(umsg)("%s\n", VG_(clo_error_markers)[0]); - VG_TDICT_CALL( tool_before_pp_Error, err ); + if (err->ekind >= 0) + VG_TDICT_CALL( tool_before_pp_Error, err ); + else + core_before_pp_Error(err); if (VG_(tdict).tool_show_ThreadIDs_for_errors && err->tid > 0 && err->tid != last_tid_printed) { @@ -634,9 +648,13 @@ static void pp_Error ( const Error* err, Bool allow_db_attach, Bool xml, Bool co } last_tid_printed = err->tid; } - - VG_TDICT_CALL( tool_pp_Error, err ); - VG_(umsg)("\n"); + + if (err->ekind >= 0) { + VG_TDICT_CALL( tool_pp_Error, err ); + VG_(umsg)("\n"); + } else { + core_pp_Error(err); + } if (VG_(clo_error_markers)[1]) VG_(umsg)("%s\n", VG_(clo_error_markers)[1]); @@ -662,7 +680,7 @@ void construct_error ( Error* err, ThreadId tid, ErrorKind ekind, Addr a, err->supp = NULL; err->count = 1; err->tid = tid; - if (NULL == where) + if (NULL == where && VG_(is_valid_tid)(tid)) err->where = VG_(record_ExeContext)( tid, 0 ); else err->where = where; @@ -813,16 +831,12 @@ void VG_(maybe_record_error) ( ThreadId tid, *p = err; /* update 'extra' */ - switch (ekind) { - //(example code, see comment on CoreSuppKind above) - //case ThreadErr: - // vg_assert(VG_(needs).core_errors); - // extra_size = - // break; - default: - vg_assert(VG_(needs).tool_errors); - extra_size = VG_TDICT_CALL(tool_update_extra, p); - break; + if (ekind < 0) { + /* core error */ + extra_size = core_update_extra (p); + } else { + vg_assert(VG_(needs).tool_errors); + extra_size = VG_TDICT_CALL(tool_update_extra, p); } /* copy the error string, if there is one. @@ -886,7 +900,10 @@ Bool VG_(unique_error) ( ThreadId tid, ErrorKind ekind, Addr a, const HChar* s, because that can have an affect on whether it's suppressed. Ignore the size return value of VG_(tdict).tool_update_extra, because we're not copying 'extra'. Similarly, 's' is also not copied. */ - (void)VG_TDICT_CALL(tool_update_extra, &err); + if (ekind >= 0) + (void)VG_TDICT_CALL(tool_update_extra, &err); + else + (void)core_update_extra(&err); su = is_suppressible_error(&err); if (NULL == su) { @@ -914,6 +931,55 @@ Bool VG_(unique_error) ( ThreadId tid, ErrorKind ekind, Addr a, const HChar* s, } +/*------------------------------------------------------------*/ +/*--- Core error fns ---*/ +/*------------------------------------------------------------*/ + +static Bool is_fd_core_error (const Error *e) +{ + return e->ekind == FdBadClose || e->ekind == FdNotClosed; +} + +static Bool core_eq_Error (VgRes res, const Error *e1, const Error *e2) +{ + if (is_fd_core_error(e1)) + return fd_eq_Error (res, e1, e2); + else { + VG_(umsg)("FATAL: unknown core error kind: %d\n", e1->ekind ); + VG_(exit)(1); + } +} + +static void core_before_pp_Error (const Error *err) +{ + if (is_fd_core_error(err)) + fd_before_pp_Error(err); + else { + VG_(umsg)("FATAL: unknown core error kind: %d\n", err->ekind ); + VG_(exit)(1); + } +} + +static void core_pp_Error (const Error *err) +{ + if (is_fd_core_error(err)) + fd_pp_Error(err); + else { + VG_(umsg)("FATAL: unknown core error kind: %d\n", err->ekind ); + VG_(exit)(1); + } +} + +static UInt core_update_extra (const Error *err) +{ + if (is_fd_core_error(err)) + return fd_update_extra(err); + else { + VG_(umsg)("FATAL: unknown core error kind: %d\n", err->ekind ); + VG_(exit)(1); + } +} + /*------------------------------------------------------------*/ /*--- Exported fns ---*/ /*------------------------------------------------------------*/ @@ -1987,6 +2053,9 @@ static Supp* is_suppressible_error ( const Error* err ) IP is needed (i.e. for the matching with the next suppr pattern), then the fun or obj name will not be searched again in the debug info. */ + if (err->where == NULL) + return NULL; + /* stats gathering */ em_supplist_searches++; diff --git a/coregrind/m_syswrap/syswrap-generic.c b/coregrind/m_syswrap/syswrap-generic.c index 7827988ca..fa218ddbd 100644 --- a/coregrind/m_syswrap/syswrap-generic.c +++ b/coregrind/m_syswrap/syswrap-generic.c @@ -40,7 +40,7 @@ #include "pub_core_xarray.h" #include "pub_core_clientstate.h" // VG_(brk_base), VG_(brk_limit) #include "pub_core_debuglog.h" -#include "pub_core_errormgr.h" +#include "pub_core_errormgr.h" // For VG_(maybe_record_error) #include "pub_core_gdbserver.h" // VG_(gdbserver) #include "pub_core_libcbase.h" #include "pub_core_libcassert.h" @@ -581,6 +581,17 @@ void ML_(record_fd_close_range)(ThreadId tid, Int from_fd) } } +struct BadCloseExtra { + Int fd; /* The file descriptor */ + HChar *pathname; /* NULL if not a regular file or unknown */ + ExeContext *where_closed; /* record the last close of fd */ + ExeContext *where_opened; /* recordwhere the fd was opened */ +}; + +struct NotClosedExtra { + Int fd; + HChar description[256]; +}; /* Note the fact that a file descriptor was just closed. */ void ML_(record_fd_close)(ThreadId tid, Int fd) @@ -593,17 +604,13 @@ void ML_(record_fd_close)(ThreadId tid, Int fd) while(i) { if (i->fd == fd) { if (i->fd_closed) { - char *path = i->pathname; - // Note we might want to turn this into a "Core error" - // Or use pub_core_execontext later. - // VG_(print_ExeContext_stats) (True ...); - VG_(emit)("File descriptor %d: %s is already closed\n", - fd, path); - VG_(pp_ExeContext)(VG_(record_ExeContext)(tid, 0)); - VG_(emit)(" Previously closed\n"); - VG_(pp_ExeContext)(i->where_closed); - VG_(emit)(" Originally opened\n"); - VG_(pp_ExeContext)(i->where); + struct BadCloseExtra bce; + bce.fd = i->fd; + bce.pathname = i->pathname; + bce.where_opened = i->where; + bce.where_closed = i->where_closed; + VG_(maybe_record_error)(tid, FdBadClose, 0, + "file descriptor already closed", &bce); } else { i->fd_closed = True; i->where_closed = ((tid == -1) @@ -621,7 +628,7 @@ void ML_(record_fd_close)(ThreadId tid, Int fd) if (VG_(getsockopt)(i->fd, VKI_SOL_SOCKET, VKI_SO_TYPE, &val, &len) == -1) { HChar *pathname = VG_(malloc)("vg.record_fd_close.fd", 30); - VG_(snprintf)(pathname, 30, "file descriptor %d\n", i->fd); + VG_(snprintf)(pathname, 30, "file descriptor %d", i->fd); i->pathname = pathname; } else { HChar *name = VG_(malloc)("vg.record_fd_close.sock", 256); @@ -920,8 +927,10 @@ void VG_(show_open_fds) (const HChar* when) && (VG_(clo_verbosity) == 0)) return; - VG_(message)(Vg_UserMsg, "FILE DESCRIPTORS: %d open (%d std) %s.\n", + if (!VG_(clo_xml)) { + VG_(umsg)("FILE DESCRIPTORS: %d open (%d std) %s.\n", fd_count, fd_count - non_std, when); + } for (i = allocated_fds; i; i = i->next) { if (i->fd_closed) @@ -930,30 +939,33 @@ void VG_(show_open_fds) (const HChar* when) if (i->fd <= 2 && VG_(clo_track_fds) < 2) continue; + struct NotClosedExtra nce; if (i->pathname) { - VG_(message)(Vg_UserMsg, "Open file descriptor %d: %s\n", i->fd, - i->pathname); + VG_(snprintf) (nce.description, 256, "file descriptor %d: %s", + i->fd, i->pathname); } else { Int val; Int len = sizeof(val); if (VG_(getsockopt)(i->fd, VKI_SOL_SOCKET, VKI_SO_TYPE, &val, &len) == -1) { - VG_(message)(Vg_UserMsg, "Open file descriptor %d:\n", i->fd); + VG_(sprintf)(nce.description, "file descriptor %d:", i->fd); } else { - HChar buf[256]; - VG_(message)(Vg_UserMsg, "Open %s\n", - getsockdetails(i->fd, 256, buf)); + getsockdetails(i->fd, 256, nce.description); } } - if(i->where) { - VG_(pp_ExeContext)(i->where); - VG_(message)(Vg_UserMsg, "\n"); - } else { - VG_(message)(Vg_UserMsg, " \n"); - VG_(message)(Vg_UserMsg, "\n"); - } + nce.fd = i->fd; + VG_(unique_error) (1 /* Fake ThreadId */, + FdNotClosed, + 0, /* Addr */ + "Still Open file descriptor", + &nce, /* extra */ + i->where, + True, /* print_error */ + False, /* allow_GDB_attach */ + True /* count_error */); + } VG_(message)(Vg_UserMsg, "\n"); @@ -1071,6 +1083,68 @@ void VG_(init_preopened_fds)(void) #endif } +Bool fd_eq_Error (VgRes res, const Error *e1, const Error *e2) +{ + // XXX should we compare the fds? + return False; +} + +void fd_before_pp_Error (const Error *err) +{ + // Nothing to do here +} + +void fd_pp_Error (const Error *err) +{ + const Bool xml = VG_(clo_xml); + const HChar* whatpre = VG_(clo_xml) ? " " : ""; + const HChar* whatpost = VG_(clo_xml) ? "" : ""; + const HChar* auxpre = VG_(clo_xml) ? " " : " "; + const HChar* auxpost = VG_(clo_xml) ? "" : ""; + ExeContext *where = VG_(get_error_where)(err); + if (VG_(get_error_kind)(err) == FdBadClose) { + if (xml) VG_(emit)(" FdBadClose\n"); + struct BadCloseExtra *bce = (struct BadCloseExtra *) + VG_(get_error_extra)(err); + VG_(emit)("%sFile descriptor %d: %s is already closed%s\n", + whatpre, bce->fd, bce->pathname, whatpost); + VG_(pp_ExeContext)( VG_(get_error_where)(err) ); + VG_(emit)("%sPreviously closed%s\n", auxpre, auxpost); + VG_(pp_ExeContext)(bce->where_closed); + VG_(emit)("%sOriginally opened%s\n", auxpre, auxpost); + VG_(pp_ExeContext)(bce->where_opened); + } else if (VG_(get_error_kind)(err) == FdNotClosed) { + if (xml) VG_(emit)(" FdNotClosed\n"); + struct NotClosedExtra *nce = (struct NotClosedExtra *) + VG_(get_error_extra)(err); + VG_(emit)("%sOpen %s%s\n", whatpre, nce->description, whatpost); + if (where != NULL) { + VG_(pp_ExeContext)(where); + if (!xml) VG_(message)(Vg_UserMsg, "\n"); + } else if (!xml) { + VG_(message)(Vg_UserMsg, " \n"); + VG_(message)(Vg_UserMsg, "\n"); + } + } else { + vg_assert2 (False, "Unknown error kind: %d", + VG_(get_error_kind)(err)); + } +} + +/* Called to see if there is any extra state to be saved with this + error. Must return the size of the extra struct. */ +UInt fd_update_extra (const Error *err) +{ + if (VG_(get_error_kind)(err) == FdBadClose) + return sizeof (struct BadCloseExtra); + else if (VG_(get_error_kind)(err) == FdNotClosed) + return sizeof (struct NotClosedExtra); + else { + vg_assert2 (False, "Unknown error kind: %d", + VG_(get_error_kind)(err)); + } +} + static void pre_mem_read_sendmsg ( ThreadId tid, Bool read, const HChar *msg, Addr base, SizeT size ) diff --git a/coregrind/pub_core_errormgr.h b/coregrind/pub_core_errormgr.h index 6dde62384..9133a119d 100644 --- a/coregrind/pub_core_errormgr.h +++ b/coregrind/pub_core_errormgr.h @@ -44,6 +44,8 @@ typedef // could detect them. This example is left as an example should new // core errors ever be added. ThreadErr = -1, + FdBadClose = -2, + FdNotClosed = -3, } CoreErrorKind; diff --git a/coregrind/pub_core_syswrap.h b/coregrind/pub_core_syswrap.h index 0b40b501d..d30dc90cf 100644 --- a/coregrind/pub_core_syswrap.h +++ b/coregrind/pub_core_syswrap.h @@ -105,6 +105,12 @@ extern void VG_(track_client_dataseg)(ThreadId tid); extern Bool VG_(get_capability_mode)(void); #endif +// For the core errors +extern Bool fd_eq_Error (VgRes, const Error*, const Error*); +extern void fd_before_pp_Error (const Error*); +extern void fd_pp_Error (const Error*); +extern UInt fd_update_extra (const Error*); + #endif // __PUB_CORE_SYSWRAP_H /*--------------------------------------------------------------------*/