Bug 256525 - Should reconsider Memcheck suppression types
Summary: Should reconsider Memcheck suppression types
Status: REPORTED
Alias: None
Product: valgrind
Classification: Developer tools
Component: memcheck (show other bugs)
Version: 3.6 SVN
Platform: Unlisted Binaries Linux
: NOR normal
Target Milestone: ---
Assignee: Julian Seward
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-11-10 17:10 UTC by Timur Iskhodzhanov
Modified: 2016-04-09 12:01 UTC (History)
4 users (show)

See Also:
Latest Commit:
Version Fixed In:


Attachments
Proposed solution (2.11 KB, patch)
2011-10-27 14:47 UTC, Timur Iskhodzhanov
Details
Add support for Memcheck:Param reports (2.21 KB, patch)
2011-10-28 09:28 UTC, Timur Iskhodzhanov
Details
Re-ordered the hunks alphabetically to produce a smaller patch (1.61 KB, patch)
2011-10-28 09:32 UTC, Timur Iskhodzhanov
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Timur Iskhodzhanov 2010-11-10 17:10:33 UTC
You can have the same code built for 32-bit architecture and 64-bit architecture.

To suppress reports on machine-word-sized variables (e.g. pointers)
you need Addr4/Value4 on 32-bit machines and Addr8/Value8 on 64-bit ones.
This means we should basically double the number of suppressions for some reports...

Also, it is quite common to have multiple reports of different types (Addr2, Addr4, Value4, Leak and Param) have the same stacktrace - simply because they are caused by the same error.

As a summary, I think it would be good to have a limited support for wildcards in suppression types, like:
Memcheck:Addr*  (matches  Addr{1,2,4,8})
Memcheck:Value* (matches Value{1,2,4,8})
Memcheck:*      (matches all types of reports)
Comment 1 Timur Iskhodzhanov 2010-11-10 17:14:19 UTC
Example or 32/64 bit stuff:
====
#include <stdio.h>
int main(void) {
  int **ptr;
  printf("%p\n", *ptr);
  return 0;
}
====
<on 64-bit machine>
$ gcc -o AddrX_64 AddrX.c
$ valgrind --gen-suppressions=all ./AddrX_64
...
==27692== Use of uninitialised value of size 8
==27692==    at 0x400520: main (in AddrX_64)
==27692== 
{
   <insert_a_suppression_name_here>
   Memcheck:Value8
   fun:main
}
==27692== Invalid read of size 8
==27692==    at 0x400520: main (in AddrX_64)
==27692==  Address 0x0 is not stack'd, malloc'd or (recently) free'd
==27692== 
{
   <insert_a_suppression_name_here>
   Memcheck:Addr8
   fun:main
}
...

$ gcc -m32 -o AddrX_32 AddrX.c
$ valgrind --gen-suppressions=all ./AddrX_32
...

==27733== Use of uninitialised value of size 4
==27733==    at 0x80483D1: main (in /home/timurrrr/sandbox/valgrind-bugs/AddrX_32)
==27733== 
{
   <insert_a_suppression_name_here>
   Memcheck:Value4
   fun:main
}
...
// Interestingly, it didn't show the Addr4 report
Comment 2 Timur Iskhodzhanov 2011-10-27 14:47:38 UTC
Created attachment 64941 [details]
Proposed solution

After struggling to copy Memcheck:{Value*,Cond*} all over the suppression file just because gcc chose a different optimization; as well as supporting both 32- and 64-bit architectures (Addr*) ...
... and using Dr. Memory for a while (has only two types: Uninitialized and Unaddressable)

... I came up with simplifying the request to just support "Memcheck:Uninitialized" and "Memcheck:Unaddressable".

Attached is a proposed patch (also supports :Uninit, :Unaddr, :Uninitialised)

Tested on:
=====test.c=====
#include <malloc.h>

void unaddr_test() {
  char * mystr = (char*)malloc(16);
  mystr[-1] = 0;
  free(mystr);
}

void uninit_test() {
  char * mystr = (char*)malloc(16);
  if (mystr[0])
    mystr[1]++;
  free(mystr);
}

int main(void) {
  uninit_test();
  unaddr_test();
}
================

===test.supp====
{
  unaddr
  Memcheck:Unaddressable
  fun:unaddr_test
  fun:main
}
{
  uninit
  Memcheck:Uninitialized
  fun:uninit_test
  fun:main
}
================
Comment 3 Timur Iskhodzhanov 2011-10-27 14:48:41 UTC
> just because gcc chose a different optimization
e.g. printf("%p", uninit_ptr);
may require Cond, Value4 or Value8 on different arch/compiler/optimizations.
Comment 4 Timur Iskhodzhanov 2011-10-27 14:56:34 UTC
Re: "Memcheck:Uninitialised" - added this just because of https://bugs.kde.org/show_bug.cgi?id=285098
I don't think we really need it, should fix bug 285098 instead
Comment 5 Timur Iskhodzhanov 2011-10-28 09:28:31 UTC
Created attachment 64973 [details]
Add support for Memcheck:Param reports

Forgotten to handle :Param uninits/unaddr.
Btw, why do you use the same suppression type (Memcheck:Param) for both types of errors?

New test:
===========
#include <malloc.h>

void unaddr_test() { char * mystr = (char*)malloc(16); mystr[-1] = 0; free(mystr); }

void uninit_test() { char * mystr = (char*)malloc(16); if (mystr[0]) mystr[1]++; free(mystr); }

void param_uninit_test() { char * mystr = (char*)malloc(16); write(1, mystr, 1); free(mystr); }

void param_unaddr_test() { char * mystr = (char*)malloc(16); write(1, mystr-1, 1); free(mystr); }

int main(void) {
  uninit_test();
  unaddr_test();
  param_uninit_test();
  param_unaddr_test();
}
===========

Two suppression files:
1) Should report 4 errors
===========
{
  param_wrong_uninit
  Memcheck:Uninitialized
  ... 
  fun:param_unaddr_test
}
{
  param_wrong_unaddr
  Memcheck:Unaddressable
  ... 
  fun:param_uninit_test
}
===========

2) Should report 0 errors
===========
{
  unaddr
  Memcheck:Unaddressable
  fun:unaddr_test
}
{
  uninit
  Memcheck:Uninitialized
  fun:uninit_test
}
{
  param_uninit
  Memcheck:Uninitialized
  ... 
  fun:param_uninit_test
}
{
  param_unaddr
  Memcheck:Unaddressable
  ... 
  fun:param_unaddr_test
}
===========
Comment 6 Timur Iskhodzhanov 2011-10-28 09:32:38 UTC
Created attachment 64974 [details]
Re-ordered the hunks alphabetically to produce a smaller patch
Comment 7 Tom Hughes 2011-10-28 09:41:32 UTC
Would it perhaps make more sense to support AddrPointer and ValuePointer or something, as suppressions that would be precise yet portable across architectures with different pointer sizes?

It just seems to me that it would be better to do that then to encourage people to use ever vaguer types of suppression, which will come with an increased risk of false positive matches.
Comment 8 Julian Seward 2011-10-28 10:13:18 UTC
(In reply to comment #7)
> Would it perhaps make more sense to support AddrPointer and ValuePointer or
> something, as suppressions that would be precise yet portable across
> architectures with different pointer sizes?

That might also be useful, but it's kind-of orthogonal to Timur's
suggestion.  (To which I am somewhat sympathetic I should add.)

Where do we draw the line in stopping people shooting themselves in the
foot with stupid suppressions?  We already provide pretty effective facilities
for foot-shooting using with this kind of thing

  {
     blah
     Memcheck:Cond
     obj:*
  }

so I'm not sure that adding Memcheck:Addr*, Memcheck:Value* 
and Memcheck:* will add much to the potential danger.

Timur's patch is shows this can be done pretty straightforwardly.

We'd need to figure out how this plays with the core/tool interface.
Is this a Memcheck-only change, or does it have implications for the
core and other tools too?
Comment 9 Timur Iskhodzhanov 2011-10-28 10:28:04 UTC
Forget "Memcheck:*", I don't need it anymore.

I don't need ":Value*" as "Value" reports may be "Cond" on different arch and vice versa.
-> I'd prefer "Memcheck:Uninitialized" or simply "Memcheck:Uninit" (or both, as it is in my patch)

I'd prefer ":Unaddressable or ":Unaddr" over ":Addr*" as we're suppressing "unaddressable access", not some particular address. Not very strong opinion here.

> Is this a Memcheck-only change
Yes, it is.
Comment 10 Timur Iskhodzhanov 2011-10-28 10:39:54 UTC
> support AddrPointer and ValuePointer
I like this idea too.
Maybe AddrPtrSize, ValuePtrSize to be more clear what they mean.

But it's still not sufficient in some cases, e.g.
a) see Value vs Cond transitions mentioned above
b) if someone does memcpy-style copying of different sizes in a loop in one function. This leads to reports of different size in case of passing a free'd buffer to such a function.
Comment 11 Timur Iskhodzhanov 2013-09-10 10:01:15 UTC
More thoughts on the topic based on my recent experience with Memcheck and other bug detectors.

It's often the case that some libraries have strlen()-like optimizations like reading past the end of the buffer within the word boundary ("near oob"). It's not always actionable (think proprietary or system libraries) but is still annoying. What Memcheck users usually do is they just suppress the annoying function.
The problem here is that the "Addr8" or "Unaddressable" suppression covers too much and we'll miss "far oob" or use-after-free bugs in this function should they occur.

Example: there are quite a lot of "/ld-@GLIBC_VERSION@*.so*" Memcheck:AddrX suppressions in the default suppression file.
On the other hand, I was able to get full Chromium binary working with combined compiler+dynamic instrumentation of Dr.ASan [1,2] without any suppressions in "ld-*" by just ignoring buffer overreads within a word boundary.
That means the default Memcheck suppressions are way too aggressive.

You might also consider differentiating unaddressable reads with writes (e.g. you might know about an unimportant unaddressable read bug, but want to be alarmed if any unaddressable write happens around).
I'm not sure about differentiating left oobs with right oobs with use-after-frees as this might not always be possible with enough precision.

Thoughts?

[1] https://code.google.com/p/address-sanitizer/source/browse/trunk/dynamorio
[2] http://swsys.ru/index.php?page=article&id=3593