From d9e71481284ed7d8c8d836a7bc5009893f91b2b4 Mon Sep 17 00:00:00 2001 From: Philippe Waroquiers Date: Thu, 23 Apr 2020 21:30:05 +0200 Subject: [PATCH] Do not fix '417075 - pwritev(vector[...]) suppression ignored' but produce a warning. - The release 3.15 introduced a backward incompatible change for some suppression entries related to preadv and pwritev syscalls. When reading a suppression entry using the unsupported 3.14 format, valgrind will now produce a warning to say the suppression entry will not work, and suggest the needed change. For example, in 3.14, the extra line was: pwritev(vector[...]) while in 3.15, it became e.g. pwritev(vector[2]) 3 possible fixes were discussed: * revert the 3.15 change to go back to 3.14 format. This is ugly because valgrind 3.16 would be incompatible with the supp entries for 3.15. * make the suppression matching logic consider that ... is a wildcard equivalent to a *. This is ugly because the suppression matching logic/functionality is already very complex, and ... would mean 2 different things in a suppression entry: wildcard in the extra line, and whatever nr of stackframes in the backtrace portion of the supp entry. * keep the 3.15 format, and accept the incompatibility with 3.14 and before. This is ugly as valgrind 3.16 and above are still incompatible with 3.14 and before. The third option was deemed the less ugly, in particular because it was possible to detect the incompatible unsupported supp entry and produce a warning. So, now, valgrind reports a warning when such an entry is detected, giving e.g. a behaviour such as: ==21717== WARNING: pwritev(vector[...]) is an obsolete suppression line not supported in valgrind 3.15 or later. ==21717== You should replace [...] by a specific index such as [0] or [1] or [2] or similar ==21717== .... ==21717== Syscall param pwritev(vector[1]) points to unaddressable byte(s) ==21717== at 0x495B65A: pwritev (pwritev64.c:30) ==21717== by 0x1096C5: main (sys-preadv_pwritev.c:69) ==21717== Address 0xffffffffffffffff is not stack'd, malloc'd or (recently) free'd So, we can hope that users having incompatible entries will easily understand the problem of the supp entry not matching anymore. In future releases of valgrind, we must take care to: * never change the extra string produced for an error, unless *really* necessary * minimise as much as possible 'variable' information generated dynamically in error extra string. Such extra information can be reported in the rest of the error message (like the address above for example). The user can use e.g. GDB + vgdb to examine in details the offending data or parameter values or uninitialised bytes or ... A comment is added in pub_tool_errormgr.h to remind tool developers of the above. --- NEWS | 9 +++++++++ coregrind/m_syswrap/syswrap-linux.c | 4 ++++ include/pub_tool_errormgr.h | 17 +++++++++++++++-- memcheck/mc_errors.c | 10 ++++++++++ 4 files changed, 38 insertions(+), 2 deletions(-) diff --git a/NEWS b/NEWS index a71e58128..a8f37d49e 100644 --- a/NEWS +++ b/NEWS @@ -56,6 +56,12 @@ support for X86/macOS 10.13, AMD64/macOS 10.13 and nanoMIPS/Linux. - Several memcheck options are now dynamically changeable. Use valgrind --help-dyn-options to list them. + - The release 3.15 introduced a backward incompatible change for + some suppression entries related to preadv and pwritev syscalls. + When reading a suppression entry using the unsupported 3.14 format, + valgrind will now produce a warning to say the suppression entry will not + work, and suggest the needed change. + * ==================== OTHER CHANGES ==================== * New and modified GDB server monitor features: @@ -123,6 +129,9 @@ where XXXXXX is the bug number as listed below. 416464 Fix false reports for uninitialized memory for PR_CAPBSET_READ/DROP 416667 gcc10 ppc64le impossible constraint in 'asm' in test_isa. 416753 new 32bit time syscalls for 2038+ +417075 pwritev(vector[...]) suppression ignored + 417075 is not fixed, but incompatible supp entries are detected + and a warning is produced for these. 417187 [MIPS] Conditional branch problem since 'grail' changes 417238 Test memcheck/tests/vbit-test fails on mips64 BE 417266 Make memcheck/tests/linux/sigqueue usable with musl diff --git a/coregrind/m_syswrap/syswrap-linux.c b/coregrind/m_syswrap/syswrap-linux.c index 1190a57d6..797d65710 100644 --- a/coregrind/m_syswrap/syswrap-linux.c +++ b/coregrind/m_syswrap/syswrap-linux.c @@ -5997,6 +5997,8 @@ void handle_pre_sys_preadv(ThreadId tid, SyscallStatus* status, count * sizeof(struct vki_iovec))) { vec = (struct vki_iovec *)(Addr)vector; for (i = 0; i < count; i++) { + /* Note: building such a dynamic error string is *not* + a pattern to follow. See bug 417075. */ VG_(snprintf) (tmp, 30, "%s(vector[%d])", str, i); PRE_MEM_WRITE( tmp, (Addr)vec[i].iov_base, vec[i].iov_len ); } @@ -6118,6 +6120,8 @@ void handle_sys_pwritev(ThreadId tid, SyscallStatus* status, count * sizeof(struct vki_iovec))) { vec = (struct vki_iovec *)(Addr)vector; for (i = 0; i < count; i++) { + /* Note: building such a dynamic error string is *not* + a pattern to follow. See bug 417075. */ VG_(snprintf) (tmp, 30, "%s(vector[%d])", str, i); PRE_MEM_READ( tmp, (Addr)vec[i].iov_base, vec[i].iov_len ); } diff --git a/include/pub_tool_errormgr.h b/include/pub_tool_errormgr.h index fd85cff58..668afa950 100644 --- a/include/pub_tool_errormgr.h +++ b/include/pub_tool_errormgr.h @@ -75,7 +75,18 @@ void* VG_(get_error_extra) ( const Error* err ); (see pub_tool_tooliface.h print_extra_suppression_info). If no 'a', 's' or 'extra' of interest needs to be recorded, just use - NULL for them. */ + NULL for them. + + ATTENTION: 's' should not contain information that is (too) specific + to an instance of an error. For example, 's' can clearly not contain + an address, as otherwise no way the user can build a suppression entry + matching such a variable error extra string. It should preferrably not + contain offset or array indexes, for similar reason. + In other words, 's' should be NULL or should be a static string. + Finally, if you change the string 's' from one release to another + for the same error, you will introduce backward incompatible changes + with the suppression files produced for previous releases. + So, don't do that ! */ extern void VG_(maybe_record_error) ( ThreadId tid, ErrorKind ekind, Addr a, const HChar* s, void* extra ); @@ -85,7 +96,9 @@ extern void VG_(maybe_record_error) ( ThreadId tid, ErrorKind ekind, 'print_error' dictates whether to print the error, which is a bit of a hack that's useful sometimes if you just want to know if the error would be suppressed without possibly printing it. 'count_error' dictates - whether to add the error in the error total count (another mild hack). */ + whether to add the error in the error total count (another mild hack). + + ATTENTION: read the 'ATTENTION' above for VG_(maybe_record_error) ! */ extern Bool VG_(unique_error) ( ThreadId tid, ErrorKind ekind, Addr a, const HChar* s, void* extra, ExeContext* where, Bool print_error, diff --git a/memcheck/mc_errors.c b/memcheck/mc_errors.c index e067686a4..d47cfa771 100644 --- a/memcheck/mc_errors.c +++ b/memcheck/mc_errors.c @@ -1380,6 +1380,16 @@ Bool MC_(read_extra_suppression_info) ( Int fd, HChar** bufpp, eof = VG_(get_line) ( fd, bufpp, nBufp, lineno ); if (eof) return False; VG_(set_supp_string)(su, VG_(strdup)("mc.resi.1", *bufpp)); + if (VG_(strcmp) (*bufpp, "preadv(vector[...])") == 0 + || VG_(strcmp) (*bufpp, "pwritev(vector[...])") == 0) { + /* Report the incompatible change introduced in 3.15 + when reading a unsupported 3.14 or before entry. + See bug 417075. */ + VG_(umsg)("WARNING: %s is an obsolete suppression line " + "not supported in valgrind 3.15 or later.\n" + "You should replace [...] by a specific index" + " such as [0] or [1] or [2] or similar\n\n", *bufpp); + } } else if (VG_(get_supp_kind)(su) == LeakSupp) { // We might have the optional match-leak-kinds line MC_LeakSuppExtra* lse;