Fix dangling ref in m_errormgr.c + report all uninit fields in a syscall param

Some syscall verification code is allocating memory to generate
the string used to build an error, e.g. syswrap-generic.c verifying fields of
e.g socket addresses (pre_mem_read_sockaddr) or sendmsg/recvmsg args
(msghdr_foreachfield)

The allocated pointer was copied in the error created by VG_(maybe_record_error).

This was wrong for 2 reasons:
1. If the error is a new error, it is stored in a list of errors,
   but the string memory was freed by pre_mem_read_sockaddr, msghdr_foreachfield, ...
   This causes a dangling reference. Was at least visible when giving -v, which
   re-prints all errors at the end of execution.
   Probably this could have some consequences during run while generating new errors,
   and comparing for equality with a recorded error having a dangling reference.
2. the same allocated string is re-used for each piece/field of the verified struct.
   The code in mc_errors.c that checks that 2 errors are identical was then wrongly
   considereing that 2 successive errors for 2 different fields for the same syscall
   arg are identical, just because the error string happened to be produced at
   the same address.
(it is believed that initially, the error string was assumed to be a static
string, which is not the case anymore, causing the above 2 problems).

Changes:
* The fix consists in duplicating in m_errormgr.c the given error string when
  the error is recorded. In other words, the error string is now duplicated similarly
  to the (optional) extra component of the error.

* memcheck/tests/linux/rfcomm.c test modified as now an error is reported
  for each uninit field.

* socketaddr unknown family is also better reported (using sa_data field name,
  rather than an empty field name.

* minor reformatting in m_errormgr.c, to be below 80 characters.

Some notes:
1. the string is only duplicated if the error is recorded
   (ie. printed or the first time an error matches a suppression).
   The string is not duplicated for duplicated errors or following errors
   matching the first (suppressed) error.
   The string is also not duplicated for 'unique errors' (that are printed
   and then not recorded).
2. duplicating the string for each recorded error is not deemed to
   use a lot of memory:
     * error strings are usually NULL or short (often 10 bytes or so).
     * we expect no program has a huge number of errors
   If ever this string duplicate would be significant, having a DedupPoolAlloc
   in m_errormgr.c for these strings would reduce this memory (as we expect to
   have very few different strings, even with millions of errors).

git-svn-id: svn://svn.valgrind.org/valgrind/trunk@14214

diff --git a/NEWS b/NEWS
index 645bbf5773cc452e4fcf405193b0dd2e1458c375..1231937c8c16e03617e65d6b1c7f868108ce5942 100644 (file)
--- a/NEWS
+++ b/NEWS
@@ -16,6 +16,10 @@ Release 3.10.0 (?? ?????? 201?)
   - new leak check heuristic 'length64' to detect interior pointers
     pointing at offset 64bit of a block, when the first 8 bytes contains
     the block size - 8. This is e.g. used by sqlite3MemMalloc.
+  - if a syscall param (e.g. bind struct sockaddr, sendmsg struct msghdr,
+    ...) has several fields not initialised, an error is now reported for
+    each field. Previously, an error was reported only for the first wrong
+    field.
 
 * Helgrind:
   - Race condition error message with allocated blocks also show

diff --git a/coregrind/m_errormgr.c b/coregrind/m_errormgr.c
index e7de4eb2e9e81490324e4fbaff1072d16740c9f0..3143d781173c8fdf6bbe3a426fa64e1feb0d6c4f 100644 (file)
--- a/coregrind/m_errormgr.c
+++ b/coregrind/m_errormgr.c
@@ -677,7 +677,8 @@ void construct_error ( Error* err, ThreadId tid, ErrorKind ekind, Addr a,
    All detected errors are notified here; this routine decides if/when the
    user should see the error. */
 void VG_(maybe_record_error) ( ThreadId tid, 
-                               ErrorKind ekind, Addr a, const HChar* s, void* extra )
+                               ErrorKind ekind, Addr a, 
+                               const HChar* s, void* extra )
 {
           Error  err;
           Error* p;
@@ -818,9 +819,17 @@ void VG_(maybe_record_error) ( ThreadId tid,
          break;
    }
 
+   /* copy the error string, if there is one.
+      note: if we would have many errors with big strings, using a
+      DedupPoolAlloc for these strings will avoid duplicating
+      such string in each error using it. */
+   if (NULL != p->string) {
+      p->string = VG_(arena_strdup)(VG_AR_CORE, "errormgr.mre.2", p->string);
+   }
+
    /* copy block pointed to by 'extra', if there is one */
    if (NULL != p->extra && 0 != extra_size) { 
-      void* new_extra = VG_(malloc)("errormgr.mre.2", extra_size);
+      void* new_extra = VG_(malloc)("errormgr.mre.3", extra_size);
       VG_(memcpy)(new_extra, p->extra, extra_size);
       p->extra = new_extra;
    }
@@ -870,7 +879,7 @@ Bool VG_(unique_error) ( ThreadId tid, ErrorKind ekind, Addr a, const HChar* s,
       Then update the 'extra' part with VG_(tdict).tool_update_extra),
       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'. */
+      not copying 'extra'. Similarly, 's' is also not copied. */
    (void)VG_TDICT_CALL(tool_update_extra, &err);
 
    su = is_suppressible_error(&err);
@@ -1514,10 +1523,11 @@ typedef
       // n_expanded >= n_ips_expanded.
 
       Int* n_offsets_per_ip;
-      // n_offsets_per_ip[i] gives the nr of offsets in fun_offsets and obj_offsets
-      // resulting of the expansion of ips[i].
+      // n_offsets_per_ip[i] gives the nr of offsets in fun_offsets and
+      // obj_offsets resulting of the expansion of ips[i].
       // The sum of all n_expanded_per_ip must be equal to n_expanded.
-      // This array allows to retrieve the position in ips corresponding to an ixInput.
+      // This array allows to retrieve the position in ips corresponding to 
+      // an ixInput.
 
       // size (in elements) of fun_offsets and obj_offsets.
       // (fun|obj)_offsets are reallocated if more space is needed
@@ -1570,9 +1580,10 @@ static void pp_ip2fo (IPtoFunOrObjCompleter* ip2fo)
 }
 
 /* free the memory in ip2fo.
-   At debuglog 4, su (or NULL) will be used to show the matching (or non matching)
-   with ip2fo. */
-static void clearIPtoFunOrObjCompleter ( Supp  *su, IPtoFunOrObjCompleter* ip2fo)
+   At debuglog 4, su (or NULL) will be used to show the matching
+   (or non matching) with ip2fo. */
+static void clearIPtoFunOrObjCompleter ( Supp  *su, 
+                                         IPtoFunOrObjCompleter* ip2fo)
 {
    if (DEBUG_ERRORMGR || VG_(debugLog_getLevel)() >= 4) {
       if (su)
@@ -1672,7 +1683,8 @@ static HChar* foComplete(IPtoFunOrObjCompleter* ip2fo,
               i++) {
             ip2fo->obj_offsets[i] = ip2fo->names_free;
             if (DEBUG_ERRORMGR) 
-               VG_(printf) ("   set obj_offset %lu to %d\n", i, ip2fo->names_free);
+               VG_(printf) ("   set obj_offset %lu to %d\n", 
+                            i, ip2fo->names_free);
          }
       }
       ip2fo->names_free += VG_(strlen)(caller_name) + 1;

diff --git a/coregrind/m_syswrap/syswrap-generic.c b/coregrind/m_syswrap/syswrap-generic.c
index 39ee199c521868a0671f7655d50e4708c179ccc5..35a2b4099f05ba8c1f69f7a4a03bf39da7081ee8 100644 (file)
--- a/coregrind/m_syswrap/syswrap-generic.c
+++ b/coregrind/m_syswrap/syswrap-generic.c
@@ -1074,8 +1074,13 @@ void pre_mem_read_sockaddr ( ThreadId tid,
 #endif
 
       default:
-         VG_(sprintf) ( outmsg, description, "" );
-         PRE_MEM_READ( outmsg, (Addr) sa, salen );
+         /* No specific information about this address family.
+            Let's just check the full data following the family.
+            Note that this can give false positive if this (unknown)
+            struct sockaddr_???? has padding bytes between its elements. */
+         VG_(sprintf) ( outmsg, description, "sa_data" );
+         PRE_MEM_READ( outmsg, (Addr)&sa->sa_family + sizeof(sa->sa_family),
+                       salen );
          break;
    }
    

diff --git a/include/pub_tool_errormgr.h b/include/pub_tool_errormgr.h
index 26339ab49b5c9ecc2db2afcca6a4608264daa20b..3ec673c8b67867109d3ac69ebcc7b29f3691f566 100644 (file)
--- a/include/pub_tool_errormgr.h
+++ b/include/pub_tool_errormgr.h
@@ -66,9 +66,15 @@ void*        VG_(get_error_extra)   ( Error* err );
    seen before.  If it has, the existing error record will have its count
    incremented.
 
-   'tid' can be found as for VG_(record_ExeContext)().  The `extra' field can
-   be stack-allocated;  it will be copied by the core if needed (but it
-   won't be copied if it's NULL).
+   'tid' can be found as for VG_(record_ExeContext)().  The `s' string
+   and `extra' field can be stack-allocated;  they will be copied by the core
+   if needed (but it won't be copied if it's NULL).
+   Note that `ekind' and `s' are also used to generate a suppression.
+   `s' should therefore not contain data depending on the specific
+   execution (such as addresses, values) but should rather contain
+   e.g. a system call parameter symbolic name.
+   `extra' is also (optionally) used for generating a suppression
+   (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.  */

diff --git a/memcheck/tests/linux/rfcomm.c b/memcheck/tests/linux/rfcomm.c
index 02dcd7eaa4e0846ca19bfa8a145b45fd723b7d17..dbda1f5b8833b18e438a1d153afb8e817193c34f 100644 (file)
--- a/memcheck/tests/linux/rfcomm.c
+++ b/memcheck/tests/linux/rfcomm.c
@@ -3,6 +3,7 @@
 #include <stdlib.h>
 #include <sys/types.h>
 #include <sys/socket.h>
+#include "../../memcheck.h"
 
 /* user space headers might not be there, define things ourselves. */
 typedef struct {
@@ -34,20 +35,39 @@ main (int argc, char **argv)
     }
 
   struct vui_sockaddr_rc aAddr;
+
+  // Store correct values in aAddr but marking it undefined
+  // so as to generate errors. We need to have deterministic
+  // undefined values to have a reproducible test.
+  aAddr.rc_family = VUI_AF_BLUETOOTH;
+  aAddr.rc_bdaddr = *VUI_BDADDR_ANY;
+  aAddr.rc_channel = 5;
+  VALGRIND_MAKE_MEM_UNDEFINED(&aAddr, sizeof(aAddr));
+  // We re-assign below each piece one by one, so as to
+  // have the piece marked initialised.
+
+
   // Ignore return values.
 
-  // Missing family
+  // Everything uninit (family, ...)
+  bind(nSocket, (struct sockaddr *) &aAddr, sizeof(aAddr));
+
+  // Same but with an unknown family (we hope :)
+  aAddr.rc_family = 12345;
+  // (reset everything to uninit)
+  VALGRIND_MAKE_MEM_UNDEFINED(&aAddr, sizeof(aAddr));
   bind(nSocket, (struct sockaddr *) &aAddr, sizeof(aAddr));
 
   aAddr.rc_family = VUI_AF_BLUETOOTH;
-  // Missing bdaddr.
+  // uninit bdaddr and channel.
   bind(nSocket, (struct sockaddr *) &aAddr, sizeof(aAddr));
 
   aAddr.rc_bdaddr = *VUI_BDADDR_ANY;
-  // Missing channel.
+  // uninit channel.
   bind(nSocket, (struct sockaddr *) &aAddr, sizeof(aAddr));
 
   aAddr.rc_channel = 5;
+  // Everything correctly init.
   bind(nSocket, (struct sockaddr *) &aAddr, sizeof(aAddr));
 
   return 0;

diff --git a/memcheck/tests/linux/rfcomm.stderr.exp b/memcheck/tests/linux/rfcomm.stderr.exp
index 293ae2ba6ef2284fc896712f30f761df743d0e07..0c16048241bd4e4dcccbcda136220270a4dd48c7 100644 (file)
--- a/memcheck/tests/linux/rfcomm.stderr.exp
+++ b/memcheck/tests/linux/rfcomm.stderr.exp
@@ -1,24 +1,64 @@
 Syscall param socketcall.bind(my_addr.sa_family) points to uninitialised byte(s)
    ...
-   by 0x........: main (rfcomm.c:40)
+   by 0x........: main (rfcomm.c:53)
  Address 0x........ is on thread 1's stack
- in frame #1, created by main (rfcomm.c:25)
- Uninitialised value was created by a stack allocation
-   at 0x........: main (rfcomm.c:25)
+ in frame #1, created by main (rfcomm.c:26)
+ Uninitialised value was created by a client request
+   at 0x........: main (rfcomm.c:45)
 
 Syscall param socketcall.bind(my_addr.rc_bdaddr) points to uninitialised byte(s)
    ...
-   by 0x........: main (rfcomm.c:44)
+   by 0x........: main (rfcomm.c:53)
  Address 0x........ is on thread 1's stack
- in frame #1, created by main (rfcomm.c:25)
- Uninitialised value was created by a stack allocation
-   at 0x........: main (rfcomm.c:25)
+ in frame #1, created by main (rfcomm.c:26)
+ Uninitialised value was created by a client request
+   at 0x........: main (rfcomm.c:45)
 
 Syscall param socketcall.bind(my_addr.rc_channel) points to uninitialised byte(s)
    ...
-   by 0x........: main (rfcomm.c:48)
+   by 0x........: main (rfcomm.c:53)
  Address 0x........ is on thread 1's stack
- in frame #1, created by main (rfcomm.c:25)
- Uninitialised value was created by a stack allocation
-   at 0x........: main (rfcomm.c:25)
+ in frame #1, created by main (rfcomm.c:26)
+ Uninitialised value was created by a client request
+   at 0x........: main (rfcomm.c:45)
+
+Syscall param socketcall.bind(my_addr.sa_family) points to uninitialised byte(s)
+   ...
+   by 0x........: main (rfcomm.c:59)
+ Address 0x........ is on thread 1's stack
+ in frame #1, created by main (rfcomm.c:26)
+ Uninitialised value was created by a client request
+   at 0x........: main (rfcomm.c:58)
+
+Syscall param socketcall.bind(my_addr.sa_data) points to uninitialised byte(s)
+   ...
+   by 0x........: main (rfcomm.c:59)
+ Address 0x........ is on thread 1's stack
+ in frame #1, created by main (rfcomm.c:26)
+ Uninitialised value was created by a client request
+   at 0x........: main (rfcomm.c:58)
+
+Syscall param socketcall.bind(my_addr.rc_bdaddr) points to uninitialised byte(s)
+   ...
+   by 0x........: main (rfcomm.c:63)
+ Address 0x........ is on thread 1's stack
+ in frame #1, created by main (rfcomm.c:26)
+ Uninitialised value was created by a client request
+   at 0x........: main (rfcomm.c:58)
+
+Syscall param socketcall.bind(my_addr.rc_channel) points to uninitialised byte(s)
+   ...
+   by 0x........: main (rfcomm.c:63)
+ Address 0x........ is on thread 1's stack
+ in frame #1, created by main (rfcomm.c:26)
+ Uninitialised value was created by a client request
+   at 0x........: main (rfcomm.c:58)
+
+Syscall param socketcall.bind(my_addr.rc_channel) points to uninitialised byte(s)
+   ...
+   by 0x........: main (rfcomm.c:67)
+ Address 0x........ is on thread 1's stack
+ in frame #1, created by main (rfcomm.c:26)
+ Uninitialised value was created by a client request
+   at 0x........: main (rfcomm.c:58)