Bug 256525

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

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

> just because gcc chose a different optimization
e.g. printf("%p", uninit_ptr);
may require Cond, Value4 or Value8 on different arch/compiler/optimizations.

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

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

Created attachment 64974 [details]
Re-ordered the hunks alphabetically to produce a smaller patch

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.

(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?

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.

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

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