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.

diff --git a/NEWS b/NEWS
index a71e581281ba556afb5dc3a52d279ab2fbde9c64..a8f37d49e50afc07fd47a6cdb79917a07dab346b 100644 (file)
--- 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 1190a57d655906347f1b04fcab97e27b9c9924db..797d65710b57158d47d552ca27a92c6883b2564b 100644 (file)
--- 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 fd85cff58e1d6f0d1c79ff3362ee3a2eed24fd8b..668afa95081eaf504e5af2a8f2a56bd6f1481523 100644 (file)
--- 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 e067686a402f7e69b2da16b2062fecfa704cceac..d47cfa771302a4a12378ba07829e333234e37a54 100644 (file)
--- 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;