Bug 280271 - Valgrind reports possible memory leaks on still-reachable std::string
Summary: Valgrind reports possible memory leaks on still-reachable std::string
Status: RESOLVED FIXED
Alias: None
Product: valgrind
Classification: Developer tools
Component: memcheck (show other bugs)
Version: 3.7 SVN
Platform: Unlisted Binaries Linux
: NOR normal
Target Milestone: ---
Assignee: Julian Seward
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-08-17 12:58 UTC by Timur Iskhodzhanov
Modified: 2013-09-30 09:30 UTC (History)
2 users (show)

See Also:
Latest Commit:
Version Fixed In:
Sentry Crash Report:


Attachments
Proposed patch for std::string and new[] (1.97 KB, patch)
2011-10-18 15:41 UTC, Timur Iskhodzhanov
Details
Improved naming; add support for string16 and wstring (2.35 KB, patch)
2011-10-20 14:10 UTC, Timur Iskhodzhanov
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Timur Iskhodzhanov 2011-08-17 12:58:48 UTC
Reproducer (C++):
-------------------------------
#include <assert.h>
#include <pthread.h>
#include <stdio.h>
#include <unistd.h>

#include <string>

void* thread_func(void *) {
  std::string str = "Valgrind";
  printf("Entering!\n");
  usleep(100000);  // Make sure the thread is killed
  printf("Finished!\n");
  return 0;
}

int main() {
  pthread_attr_t attributes;
  pthread_attr_init(&attributes);
  pthread_attr_setdetachstate(&attributes, PTHREAD_CREATE_DETACHED);
  pthread_t thr;
  int rv = pthread_create(&thr, &attributes, thread_func, NULL);
  assert(rv == 0); 
  pthread_attr_destroy(&attributes);
  usleep(10000); // Make sure the thread starts
}
-------------------------------
$ valgrind --leak-check=full ./a.out
==20096== Memcheck, a memory error detector
==20096== Copyright (C) 2002-2010, and GNU GPL'd, by Julian Seward et al.
==20096== Using Valgrind-3.7.0.SVN and LibVEX; rerun with -h for copyright info
==20096== Command: ./a.out
==20096== 
Entering!
==20096== 
...
==20096== 33 bytes in 1 blocks are possibly lost in loss record 1 of 2
==20096==    at 0x4C2B2A6: operator new(unsigned long) (vg_replace_malloc.c:261)
==20096==    by 0x50F6D98: std::string::_Rep::_S_create(...) (new_allocator.h:89)
==20096==    by 0x50F78B4: char* std::string::_S_construct<...>(...) (basic_string.tcc:139)
==20096==    by 0x50F7A52: std::basic_string<...>::basic_string(char const*, std::allocator<char> const&) (basic_string.h:1546)
==20096==    by 0x400B53: thread_func(void*) (nonjoin_string.cpp:9)
==20096==    by 0x4E389C9: start_thread (pthread_create.c:300)
==20096==    by 0x58E370C: clone (clone.S:112)

AFAIK from reading the Dr. Memory paper, std::string holds a pointer to the *middle* of the allocated chunk (+4 or +8 bytes or something like that).
That tool has special heuristics to treat such allocations as reachable (e.g. if the first 4/8 bytes look like a sane std::string allocation)
Comment 1 Timur Iskhodzhanov 2011-08-17 13:19:53 UTC
The report may change slightly:
--------------------
void* thread_func(void *) {
  std::string str = "Valgrind";
  str += " rocks\n";
  printf("Entering!\n");
  usleep(100000);  // Make sure the thread is killed
  printf("Finished!\n");
  return 0;
}
--------------------
results in:
==20839== 153 bytes in 1 blocks are possibly lost in loss record 1 of 2
==20839==    at 0x4C2B2A6: operator new(unsigned long) (vg_replace_malloc.c:261)
==20839==    by 0x50F6D98: std::string::_Rep::_S_create(...) (new_allocator.h:89)
==20839==    by 0x50F776A: std::string::_Rep::_M_clone(...) (basic_string.tcc:607)
==20839==    by 0x50F829B: std::string::reserve(unsigned long) (basic_string.tcc:488)
==20839==    by 0x50F84E7: std::string::append(...) (basic_string.tcc:309)
==20839==    by 0x400BE8: thread_func(void*) (repstr.cpp:11)
==20839==    by 0x4E389C9: start_thread (pthread_create.c:300)
==20839==    by 0x58E370C: clone (clone.S:112)
Comment 2 Timur Iskhodzhanov 2011-08-17 13:20:47 UTC
Similar reports for arrays allocated new[] with non-trivial element destructors:
----------------------------
class MyClass {
  public:
    ~MyClass() { printf("BOO\n"); }
};  

void* thread_func(void *) {
  MyClass *ptr = new MyClass[256];
  printf("Entering!\n");
  usleep(100000);  // Make sure the thread is killed
  printf("Finished!\n");
  delete [] ptr;
  return 0; 
}
----------------------------
==21510== 264 bytes in 1 blocks are possibly lost in loss record 1 of 2
==21510==    at 0x4C2AF1E: operator new[](unsigned long) (vg_replace_malloc.c:305)
==21510==    by 0x40097A: thread_func(void*) (test.cpp:14)
==21510==    by 0x4E389C9: start_thread (pthread_create.c:300)
==21510==    by 0x58E370C: clone (clone.S:112)
Comment 3 Timur Iskhodzhanov 2011-08-17 13:27:25 UTC
Same for multiple inheritance when a pointer is stored to some "middle" subclass:
----------------------------
struct A {
  virtual ~A() { printf("BOO\n"); }
};
struct B {
  virtual ~B() { printf("BOO\n"); }
};
struct C : public A, public B {
  virtual ~C() { printf("BOO\n"); }
};

void* thread_func(void *) {
  B *ptr = new C;  // no reports if "A *ptr = "
  printf("Entering!\n");
  usleep(100000);  // Make sure the thread is killed
  printf("Finished!\n");
  delete ptr;
  return 0;
}
----------------------------
==21869== 16 bytes in 1 blocks are possibly lost in loss record 1 of 2
==21869==    at 0x4C2B2A6: operator new(unsigned long) (vg_replace_malloc.c:261)
==21869==    by 0x400AFA: thread_func(void*) (multiple_inheritance.cpp:19)
==21869==    by 0x4E389C9: start_thread (pthread_create.c:300)
==21869==    by 0x58E370C: clone (clone.S:112)
Comment 4 Tom Hughes 2011-08-17 13:53:02 UTC
It's generally a good idea to stick to one issue per bug report.

That said, all these things are sort of the same issue, which isn't really an issue at all.

When valgrind reports a "possible leak" what it means is that it can't find a pointer to the start of the block, only a pointer to the middle of the block. Only you, as the end user, can know if that is a real problem or not.

Because valgrind is operating down at the machine instruction level it has no semantic knowledge of the language your code is written in, or of the standard libraries used by that language.

So basically, if you think that we should try and somehow add that semantic knowledge and hence avoid these reports, then each of these issues should be raised as a separate bug as they would need to be addressed separately.

That said, it seems unlikely to me that we would be able to add such knowledge unless you've got some brilliant ideas on how we can do it.
Comment 5 Timur Iskhodzhanov 2011-08-17 14:05:06 UTC
> Only you, as the end user, can know if that is a real problem or not.
I'd expect standard C++ types to be understandable by the tool.
Otherwise, it gives so much noise that developers just disable the "possibly lost" reports [for example, see issue 201170].

> Unless you've got some brilliant ideas on how we can do it.
You can look at the Dr. Memory publication at CGO 2011
http://www.burningcutlery.com/derek/docs/drmem-CGO11.pdf [section VI, C]
The tool has basically the same "possibly lost" vs "definitely lost" classification and it has very little "possibly lost" noise, even on Windows
Comment 6 Tom Hughes 2011-08-18 08:02:40 UTC
Well to me those Dr Memory heuristics look pretty scary. I assumed they were going to be doing something clever (but fragile) like intercepting the relevant routines but just trying guess like that scares the pants of me.

What really surprises me in a lot of ways is that, as far as I know, nobody has ever complained about any of these before.

We do occasionally get people with issues with std::string (and some of the standard library containers) but that's always to do with the library optimising things by caching "freed" space for later reuse and such like.

Now it's true that Dr Memory will probably see a lot more C++ than we do simply because it supports Windows so that might be part of it.

Did you actually run into these issues yourself in real world code? or did you set out to reproduce them based on the comments in the Dr Memory paper?
Comment 7 Timur Iskhodzhanov 2011-08-18 08:29:21 UTC
> but just trying guess like that scares the pants of me.
Is there something you're aware of except for false negatives?
btw, by doing the described trick they also don't suffer false "possible leak" reports in sqlite which has its own malloc wrapper.

> Did you actually run into these issues yourself in real world code?
Yes I did - Chromium has an enormous number of such reports [see also bug 201170 for Dan Kegel's feature request from his Chromium-Valgrind experience]
We've just disabled printing Valgrind possible leaks just because of that.
And we still print possible leaks while running Dr. Memory!

---------------
Also, running Chromium unit_tests alone we get ~13000 suppressed reports like the comment #1 and #2:
http://build.chromium.org/p/chromium.memory/builders/Linux%20Tests%20%28valgrind%29%286%29/builds/424/steps/memory%20test%3A%20unit/logs/stdio [part 1 of 2]
Suppressions used:
  count name
    ...
    683 bug_76386a
    997 bug_76386b
http://build.chromium.org/p/chromium.memory/builders/Linux%20Tests%20%28valgrind%29%287%29/builds/403/steps/memory%20test%3A%20unit/logs/stdio [part 2 of 2]
Suppressions used:
  count name
   ...
   2207 bug_76386b
   8936 bug_76386a

Suppression at this time are:
http://src.chromium.org/viewvc/chrome/trunk/src/tools/valgrind/memcheck/suppressions.txt?revision=97188&view=markup
{
   bug_76386a
   Memcheck:Leak
   fun:_Znw*
   fun:_ZNSs4_Rep9_S_createE*RKSaIcE
   ...
   fun:_ZNSsC1*KS*
}
{
   bug_76386b
   Memcheck:Leak
   fun:_Znw*
   fun:_ZNSs4_Rep9_S_createE*RKSaIcE
   fun:_ZNSs4_Rep8_M_cloneERKSaIcE*
}

I'm not sure if bug_76386 are actually "possibly lost" leaks in Valgrind's mind as we're disabling printing those.
However, each time such a leak appears it looks very strange like a "leak" on some local string variable, so I can't support Valgrind's opinion there either.
Maybe that's "library optimising things by caching "freed" space for later reuse and such like" you're talking about.

But I'd suspect "caching "freed" space for later reuse" would still be reachable, no?
Comment 8 Timur Iskhodzhanov 2011-09-14 14:06:14 UTC
> What really surprises me in a lot of ways is that, as far as I know,
> nobody has ever complained about any of these before.
These report matter in the following cases:
a) The running process receives a SIGTERM in the middle of execution
b) There are non-joined threads
[maybe more]

a) and b) are true for Chromium with its complex process architecture and many threads. Not sure if it affects other C++ projects as much.

WDYT?
Comment 9 Timur Iskhodzhanov 2011-10-18 15:41:31 UTC
Created attachment 64673 [details]
Proposed patch for std::string and new[]

I've made a patch [attached] that implements the described heuristics for std::string and new[].
Multiple inheritance is not a priority right now since we don't use it very often in Chromium.
Comment 10 Timur Iskhodzhanov 2011-10-20 14:10:40 UTC
Created attachment 64736 [details]
Improved naming; add support for string16 and wstring

Updating the patch.

Tested on the following code:
------------------------------------------
#include <string>

typedef std::basic_string<unsigned char> string16;
std::string  *ptr8;
string16     *ptr16;
std::wstring *ptr32;

class MyClass {
 public:
  ~MyClass() { }
} *new_array;

int main(void) {
  ptr8 = new std::string;
  *ptr8 += "ASDASDASDASDASDASDASDSAD";

  ptr16 = new string16;
  for (int i = 0; i < ptr8->size(); i++)
    *ptr16 += (unsigned char)(*ptr8)[i];

  ptr32 = new std::wstring;
  *ptr32 += L"ASDASDASDASDASDASDASDSAD";

  new_array = new MyClass[42];
}
Comment 11 Philippe Waroquiers 2013-09-29 14:02:55 UTC
fixed in rev r13582, which adds the option:
    --leak-check-heuristics=heur1,heur2,... which heuristics to use for
        improving leak search false positive [none]
        where heur is one of stdstring newarray multipleinheritance all none
Comment 12 Timur Iskhodzhanov 2013-09-29 20:12:16 UTC
Just curious - why is it [none] by default rather than [all] ?
Comment 13 Philippe Waroquiers 2013-09-29 21:26:18 UTC
(In reply to comment #12)
> Just curious - why is it [none] by default rather than [all] 
It looked to me somewhat preferrable to keep the current behaviour,
as there is a chance that the heuristics create false negative leaks
and not everybody uses these c++ constructs.
On the other hand, when an heuristic is used, it appears in the leak summary e.g.
  ...
   still reachable: 111 bytes in 7 blocks
                      of which reachable via heuristic:
                        stdstring          : 56 bytes in 2 blocks
                        newarray           : 7 bytes in 1 blocks
                        multipleinheritance: 24 bytes in 2 blocks

So an [all] default would not create fully "invisible" false negative.
Comment 14 Timur Iskhodzhanov 2013-09-30 09:30:37 UTC
I'd argue we should set [all] as the default at least on 32-bit archs, see
https://code.google.com/p/valgrind-variant/wiki/LeakCheckingOn32bits