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

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

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;

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;
    }
    

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.  */

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;

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)