]> git.ipfire.org Git - thirdparty/valgrind.git/commitdiff
Provide a back trace when a function argument of a known allocation
authorFlorian Krohm <florian@eich-krohm.de>
Sun, 13 Jul 2014 14:41:55 +0000 (14:41 +0000)
committerFlorian Krohm <florian@eich-krohm.de>
Sun, 13 Jul 2014 14:41:55 +0000 (14:41 +0000)
function is presumably negative. Fixes BZ 79311.

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

NEWS
memcheck/docs/mc-manual.xml
memcheck/mc_errors.c
memcheck/mc_include.h
memcheck/mc_malloc_wrappers.c
memcheck/tests/accounting.stderr.exp
memcheck/tests/malloc3.stderr.exp

diff --git a/NEWS b/NEWS
index f898bcfdd9b7b05d34faaef9255472b80383efbc..b8f48cee27052eda0838ca37520dc5fe95d173e1 100644 (file)
--- a/NEWS
+++ b/NEWS
@@ -71,6 +71,9 @@ Release 3.10.0 (?? ?????? 201?)
 
 * The C++ demangler has been updated for better C++11 support.
 
+* Error messages about fishy arguments (formerly known as silly arguments)
+  now include a back-trace to aid debugging.
+
 * ==================== FIXED BUGS ====================
 
 The following bugs have been fixed or resolved.  Note that "n-i-bz"
@@ -1053,6 +1056,7 @@ To see details of a given bug, visit
 https://bugs.kde.org/show_bug.cgi?id=XXXXXX
 where XXXXXX is the bug number as listed below.
 
+ 79311  malloc silly arg warning does not give stack trace
 210935  port valgrind.h (not valgrind) to win32 to support client requests
 214223  valgrind SIGSEGV on startup gcc 4.4.1 ppc32 (G4) Ubuntu 9.10
 243404  Port to zSeries
index 1745d7a5b93213031ddcfc1b3f0cbe8cf49792da..66a1eb38b1d064a9bf1e5222d93d7d67a51b8df2 100644 (file)
@@ -44,6 +44,12 @@ problems that are common in C and C++ programs.</para>
     functions.</para>
   </listitem>
 
+  <listitem>
+    <para>Passing a fishy (presumably negative) value to the
+    <computeroutput>size</computeroutput> parameter of a memory
+    allocation function.</para>
+  </listitem>
+
   <listitem>
     <para>Memory leaks.</para>
   </listitem>
@@ -378,6 +384,48 @@ implementation.</para>
 </sect2>
 
 
+<sect2 id="mc-manual.fishyvalue"
+       xreflabel="Fishy argument values">
+<title>Fishy argument values</title>
+
+<para>All memory allocation functions take an argument specifying the
+size of the memory block that should be allocated. Clearly, the requested
+size should be a non-negative value and is typically not excessively large. 
+For instance, it is extremely unlikly that the size of an allocation
+request exceeds 2**63 bytes on a 64-bit machine. It is much more likely that
+such a value is the result of an erroneous size calculation and is in effect
+a negative value (that just happens to appear excessively large because
+the bit pattern is interpreted as an unsigned integer).
+Such a value is called a "fishy value".
+
+The <varname>size</varname> argument of the following allocation functions
+is checked for being fishy:
+<function>malloc</function>,
+<function>calloc</function>,
+<function>realloc</function>,
+<function>memalign</function>,
+<function>new</function>,
+<function>new []</function>. 
+<function>__builtin_new</function>,
+<function>__builtin_vec_new</function>,
+For <function>calloc</function> both arguments are being checked.
+</para>
+
+<para>For example:</para>
+<programlisting><![CDATA[
+==32233== Argument 'size' of function malloc has a fishy (possibly negative) value: -3
+==32233==    at 0x4C2CFA7: malloc (vg_replace_malloc.c:298)
+==32233==    by 0x400555: foo (fishy.c:15)
+==32233==    by 0x400583: main (fishy.c:23)
+]]></programlisting>
+
+<para>In earlier Valgrind versions those values were being referred to
+as "silly arguments" and no back-trace was included.
+</para>
+
+</sect2>
+
+
 <sect2 id="mc-manual.leaks" xreflabel="Memory leak detection">
 <title>Memory leak detection</title>
 
index cd8cd6d2945d6a9dfff68e761f6a0e5cac8b2149..18e927774da574566d53e6fae5a5d88a2a11c8f3 100644 (file)
@@ -75,6 +75,7 @@ typedef
       Err_Overlap,
       Err_Leak,
       Err_IllegalMempool,
+      Err_FishyValue,
    }
    MC_ErrorTag;
 
@@ -178,6 +179,15 @@ struct _MC_Error {
          AddrInfo ai;
       } IllegalMempool;
 
+      // A fishy function argument value
+      // An argument value is considered fishy if the corresponding
+      // parameter has SizeT type and the value when interpreted as a
+      // signed number is negative.
+     struct {
+         const HChar *function_name;
+         const HChar *argument_name;
+         SizeT value;
+      } FishyValue;
    } Err;
 };
 
@@ -655,6 +665,27 @@ void MC_(pp_Error) ( Error* err )
          break;
       }
 
+      case Err_FishyValue:
+         if (xml) {
+            emit( "  <kind>FishyValue</kind>\n" );
+            emit( "  <what>");
+            emit( "Argument '%s' of function %s has a fishy "
+                  "(possibly negative) value: %ld\n",
+                  extra->Err.FishyValue.argument_name,
+                  extra->Err.FishyValue.function_name,
+                  (SSizeT)extra->Err.FishyValue.value);
+            emit( "</what>");
+            VG_(pp_ExeContext)( VG_(get_error_where)(err) );
+         } else {
+            emit( "Argument '%s' of function %s has a fishy "
+                  "(possibly negative) value: %ld\n",
+                  extra->Err.FishyValue.argument_name,
+                  extra->Err.FishyValue.function_name,
+                  (SSizeT)extra->Err.FishyValue.value);
+            VG_(pp_ExeContext)( VG_(get_error_where)(err) );
+         }
+         break;
+
       default: 
          VG_(printf)("Error:\n  unknown Memcheck error code %d\n",
                      VG_(get_error_kind)(err));
@@ -835,6 +866,25 @@ Bool MC_(record_leak_error) ( ThreadId tid, UInt n_this_record,
                        /*allow_GDB_attach*/False, count_error );
 }
 
+Bool MC_(record_fishy_value_error) ( ThreadId tid, const HChar *function_name,
+                                     const HChar *argument_name, SizeT value)
+{
+   MC_Error extra;
+
+   tl_assert(VG_INVALID_THREADID != tid);
+
+   if ((SSizeT)value >= 0) return False;  // not a fishy value
+
+   extra.Err.FishyValue.function_name = function_name;
+   extra.Err.FishyValue.argument_name = argument_name;
+   extra.Err.FishyValue.value = value;
+
+   VG_(maybe_record_error)( 
+      tid, Err_FishyValue, /*addr*/0, /*s*/NULL, &extra );
+
+   return True;
+}
+
 void MC_(record_user_error) ( ThreadId tid, Addr a,
                               Bool isAddrErr, UInt otag )
 {
@@ -904,6 +954,12 @@ Bool MC_(eq_Error) ( VgRes res, Error* e1, Error* e2 )
       case Err_Cond:
          return True;
 
+      case Err_FishyValue:
+         return VG_STREQ(extra1->Err.FishyValue.function_name,
+                         extra2->Err.FishyValue.function_name) &&
+                VG_STREQ(extra1->Err.FishyValue.argument_name,
+                         extra2->Err.FishyValue.argument_name);
+
       case Err_Addr:
          return ( extra1->Err.Addr.szB == extra2->Err.Addr.szB
                 ? True : False );
@@ -1033,6 +1089,7 @@ UInt MC_(update_Error_extra)( Error* err )
    //case Err_Value:
    //case Err_Cond:
    case Err_Overlap:
+   case Err_FishyValue:
    // For Err_Leaks the returned size does not matter -- they are always
    // shown with VG_(unique_error)() so they 'extra' not copied.  But
    // we make it consistent with the others.
@@ -1186,6 +1243,7 @@ typedef
       OverlapSupp,   // Overlapping blocks in memcpy(), strcpy(), etc
       LeakSupp,      // Something to be suppressed in a leak check.
       MempoolSupp,   // Memory pool suppression.
+      FishyValueSupp,// Fishy value suppression.
    } 
    MC_SuppKind;
 
@@ -1213,6 +1271,7 @@ Bool MC_(is_recognised_suppression) ( const HChar* name, Supp* su )
    else if (VG_STREQ(name, "Value4"))  skind = Value4Supp;
    else if (VG_STREQ(name, "Value8"))  skind = Value8Supp;
    else if (VG_STREQ(name, "Value16")) skind = Value16Supp;
+   else if (VG_STREQ(name, "FishyValue")) skind = FishyValueSupp;
    else 
       return False;
 
@@ -1234,6 +1293,11 @@ struct _MC_LeakSuppExtra {
    UInt  leak_search_gen;
 };
 
+typedef struct {
+   const HChar *function_name;
+   const HChar *argument_name;
+} MC_FishyValueExtra;
+
 Bool MC_(read_extra_suppression_info) ( Int fd, HChar** bufpp,
                                         SizeT* nBufp, Int* lineno, Supp *su )
 {
@@ -1265,6 +1329,26 @@ Bool MC_(read_extra_suppression_info) ( Int fd, HChar** bufpp,
       } else {
          return False; // unknown extra line.
       }
+   } else if (VG_(get_supp_kind)(su) == FishyValueSupp) {
+      MC_FishyValueExtra *extra;
+      HChar *p;
+
+      eof = VG_(get_line) ( fd, bufpp, nBufp, lineno );
+      if (eof) return True;
+
+      extra = VG_(malloc)("mc.resi.3", sizeof *extra);
+      extra->function_name = VG_(strdup)("mv.resi.4", *bufpp);
+
+      // The suppression string is: function_name(argument_name)
+      p = VG_(strchr)(extra->function_name, '(');
+      if (p == NULL) return False;  // malformed suppression string
+      *p++ = '\0';
+      extra->argument_name = p;
+      p = VG_(strchr)(p, ')');
+      if (p == NULL) return False;  // malformed suppression string
+      *p = '\0';
+
+      VG_(set_supp_extra)(su, extra);
    }
    return True;
 }
@@ -1334,6 +1418,16 @@ Bool MC_(error_matches_suppression) ( Error* err, Supp* su )
       case MempoolSupp:
          return (ekind == Err_IllegalMempool);
 
+      case FishyValueSupp: {
+         MC_FishyValueExtra *supp_extra = VG_(get_supp_extra)(su);
+
+         return (ekind == Err_FishyValue) &&
+                VG_STREQ(extra->Err.FishyValue.function_name,
+                         supp_extra->function_name) &&
+                VG_STREQ(extra->Err.FishyValue.argument_name,
+                         supp_extra->argument_name);
+      }
+
       default:
          VG_(printf)("Error:\n"
                      "  unknown suppression type %d\n",
@@ -1357,6 +1451,7 @@ const HChar* MC_(get_error_name) ( Error* err )
    case Err_Overlap:        return "Overlap";
    case Err_Leak:           return "Leak";
    case Err_Cond:           return "Cond";
+   case Err_FishyValue:     return "FishyValue";
    case Err_Addr: {
       MC_Error* extra = VG_(get_error_extra)(err);
       switch ( extra->Err.Addr.szB ) {
@@ -1400,6 +1495,12 @@ Bool MC_(get_extra_suppression_info) ( Error* err,
          (buf, nBuf-1, "match-leak-kinds: %s",
           pp_Reachedness_for_leak_kinds(extra->Err.Leak.lr->key.state));
       return True;
+   } else if (Err_FishyValue == ekind) {
+      MC_Error* extra = VG_(get_error_extra)(err);
+      VG_(snprintf)
+         (buf, nBuf-1, "%s(%s)", extra->Err.FishyValue.function_name,
+          extra->Err.FishyValue.argument_name);
+      return True;
    } else {
       return False;
    }
index 012f53954b5dc9410c82313786f2d93565612664..425d23380b747fecf94190ec70e3b49b7e8eab50 100644 (file)
@@ -441,6 +441,9 @@ Bool MC_(record_leak_error)     ( ThreadId tid,
                                   Bool print_record,
                                   Bool count_error );
 
+Bool MC_(record_fishy_value_error)  ( ThreadId tid, const HChar* function,
+                                      const HChar *argument_name, SizeT value );
+
 /* Parses a set of leak kinds (separated by ,).
    and give the resulting set in *lks.
    If parsing is succesful, returns True and *lks contains the resulting set.
index c51ed8ffa979bbe7bdaa7d934f7bc48037bed5ee..6b152766eadf390c767513620963dd7b7685599a 100644 (file)
@@ -335,33 +335,6 @@ UInt MC_(n_where_pointers) (void)
 /*--- client_malloc(), etc                                 ---*/
 /*------------------------------------------------------------*/
 
-// XXX: should make this a proper error (bug #79311).
-static Bool complain_about_silly_args(SizeT sizeB, const HChar* fn)
-{
-   // Cast to a signed type to catch any unexpectedly negative args.  We're
-   // assuming here that the size asked for is not greater than 2^31 bytes
-   // (for 32-bit platforms) or 2^63 bytes (for 64-bit platforms).
-   if ((SSizeT)sizeB < 0) {
-      if (!VG_(clo_xml)) 
-         VG_(message)(Vg_UserMsg, "Warning: silly arg (%ld) to %s()\n",
-                      (SSizeT)sizeB, fn );
-      return True;
-   }
-   return False;
-}
-
-static Bool complain_about_silly_args2(SizeT n, SizeT sizeB)
-{
-   if ((SSizeT)n < 0 || (SSizeT)sizeB < 0) {
-      if (!VG_(clo_xml))
-         VG_(message)(Vg_UserMsg,
-                      "Warning: silly args (%ld,%ld) to calloc()\n",
-                      (SSizeT)n, (SSizeT)sizeB);
-      return True;
-   }
-   return False;
-}
-
 /* Allocate memory and note change in memory available */
 void* MC_(new_block) ( ThreadId tid,
                        Addr p, SizeT szB, SizeT alignB,
@@ -406,7 +379,7 @@ void* MC_(new_block) ( ThreadId tid,
 
 void* MC_(malloc) ( ThreadId tid, SizeT n )
 {
-   if (complain_about_silly_args(n, "malloc")) {
+   if (MC_(record_fishy_value_error)(tid, "malloc", "size", n)) {
       return NULL;
    } else {
       return MC_(new_block) ( tid, 0, n, VG_(clo_alignment), 
@@ -416,7 +389,7 @@ void* MC_(malloc) ( ThreadId tid, SizeT n )
 
 void* MC_(__builtin_new) ( ThreadId tid, SizeT n )
 {
-   if (complain_about_silly_args(n, "__builtin_new")) {
+   if (MC_(record_fishy_value_error)(tid, "__builtin_new", "size", n)) {
       return NULL;
    } else {
       return MC_(new_block) ( tid, 0, n, VG_(clo_alignment), 
@@ -426,7 +399,7 @@ void* MC_(__builtin_new) ( ThreadId tid, SizeT n )
 
 void* MC_(__builtin_vec_new) ( ThreadId tid, SizeT n )
 {
-   if (complain_about_silly_args(n, "__builtin_vec_new")) {
+   if (MC_(record_fishy_value_error)(tid, "__builtin_vec_new", "size", n)) {
       return NULL;
    } else {
       return MC_(new_block) ( tid, 0, n, VG_(clo_alignment), 
@@ -436,7 +409,7 @@ void* MC_(__builtin_vec_new) ( ThreadId tid, SizeT n )
 
 void* MC_(memalign) ( ThreadId tid, SizeT alignB, SizeT n )
 {
-   if (complain_about_silly_args(n, "memalign")) {
+   if (MC_(record_fishy_value_error)(tid, "memalign", "size", n)) {
       return NULL;
    } else {
       return MC_(new_block) ( tid, 0, n, alignB, 
@@ -446,7 +419,8 @@ void* MC_(memalign) ( ThreadId tid, SizeT alignB, SizeT n )
 
 void* MC_(calloc) ( ThreadId tid, SizeT nmemb, SizeT size1 )
 {
-   if (complain_about_silly_args2(nmemb, size1)) {
+   if (MC_(record_fishy_value_error)(tid, "calloc", "nmemb", nmemb) ||
+       MC_(record_fishy_value_error)(tid, "calloc", "size", size1)) {
       return NULL;
    } else {
       return MC_(new_block) ( tid, 0, nmemb*size1, VG_(clo_alignment),
@@ -538,7 +512,7 @@ void* MC_(realloc) ( ThreadId tid, void* p_old, SizeT new_szB )
    Addr      a_new; 
    SizeT     old_szB;
 
-   if (complain_about_silly_args(new_szB, "realloc")) 
+   if (MC_(record_fishy_value_error)(tid, "realloc", "size", new_szB))
       return NULL;
 
    cmalloc_n_frees ++;
index fb31e6d77d320b287536175343cec77d35db3112..0119b98905a6695858749dbf2aa56f855ee97579 100644 (file)
@@ -1,5 +1,8 @@
 
-Warning: silly arg (-1) to realloc()
+Argument 'size' of function realloc has a fishy (possibly negative) value: -1
+   at 0x........: realloc (vg_replace_malloc.c:...)
+   by 0x........: main (accounting.c:17)
+
 
 HEAP SUMMARY:
     in use at exit: 0 bytes in 0 blocks
@@ -8,4 +11,4 @@ HEAP SUMMARY:
 For a detailed leak analysis, rerun with: --leak-check=full
 
 For counts of detected and suppressed errors, rerun with: -v
-ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)
+ERROR SUMMARY: 1 errors from 1 contexts (suppressed: 0 from 0)
index 71d8d3da09e176a67f61b7cadfa683153a69f4d8..a25ad14792fb4c1978445124e7caa330051c9111 100644 (file)
@@ -1,2 +1,8 @@
-Warning: silly arg (-1) to malloc()
-Warning: silly args (0,-1) to calloc()
+Argument 'size' of function malloc has a fishy (possibly negative) value: -1
+   at 0x........: malloc (vg_replace_malloc.c:...)
+   by 0x........: main (malloc3.c:15)
+
+Argument 'size' of function calloc has a fishy (possibly negative) value: -1
+   at 0x........: calloc (vg_replace_malloc.c:...)
+   by 0x........: main (malloc3.c:23)
+