[Bug 280271] Valgrind reports possible memory leaks on still-reachable std::string

[Timur Iskhodzhanov]

https://bugs.kde.org/show_bug.cgi?id=280271





--- Comment #1 from Timur Iskhodzhanov <timurrrr google com> 2011-08-17 13:19:53 ---
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)

[Timur Iskhodzhanov]

https://bugs.kde.org/show_bug.cgi?id=280271





--- Comment #2 from Timur Iskhodzhanov <timurrrr google com> 2011-08-17 13:20:47 ---
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)

[Timur Iskhodzhanov]

https://bugs.kde.org/show_bug.cgi?id=280271





--- Comment #3 from Timur Iskhodzhanov <timurrrr google com> 2011-08-17 13:27:25 ---
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)

[Tom Hughes]

https://bugs.kde.org/show_bug.cgi?id=280271


Tom Hughes <tom-***@public.gmane.org> changed:

What |Removed |Added
----------------------------------------------------------------------------
CC| |tom-***@public.gmane.org




--- Comment #4 from Tom Hughes <tom compton nu> 2011-08-17 13:53:02 ---
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.

[Timur Iskhodzhanov]

https://bugs.kde.org/show_bug.cgi?id=280271





--- Comment #5 from Timur Iskhodzhanov <timurrrr google com> 2011-08-17 14:05:06 ---

Post by Tom Hughes
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].

Post by Tom Hughes
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

[Tom Hughes]

https://bugs.kde.org/show_bug.cgi?id=280271





--- Comment #6 from Tom Hughes <tom compton nu> 2011-08-18 08:02:40 ---
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?

[Timur Iskhodzhanov]

https://bugs.kde.org/show_bug.cgi?id=280271





--- Comment #7 from Timur Iskhodzhanov <timurrrr google com> 2011-08-18 08:29:21 ---

Post by Tom Hughes
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.

Post by Tom Hughes
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?

[Timur Iskhodzhanov]

https://bugs.kde.org/show_bug.cgi?id=280271





--- Comment #8 from Timur Iskhodzhanov <timurrrr google com> 2011-09-14 14:06:14 ---

Post by Tom Hughes
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?

[Timur Iskhodzhanov]

https://bugs.kde.org/show_bug.cgi?id=280271





--- Comment #9 from Timur Iskhodzhanov <timurrrr google com> 2011-10-18 15:41:31 ---
Created an attachment (id=64673)
--> (http://bugs.kde.org/attachment.cgi?id=64673)
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.

[Timur Iskhodzhanov]

https://bugs.kde.org/show_bug.cgi?id=280271


Timur Iskhodzhanov <timurrrr-hpIqsD4AKlfQT0dZR+***@public.gmane.org> changed:

What |Removed |Added
----------------------------------------------------------------------------
Attachment #64673|0 |1
is obsolete| |




--- Comment #10 from Timur Iskhodzhanov <timurrrr google com> 2011-10-20 14:10:40 ---
Created an attachment (id=64736)
--> (http://bugs.kde.org/attachment.cgi?id=64736)
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];
}

[Philippe Waroquiers]

https://bugs.kde.org/show_bug.cgi?id=280271

Philippe Waroquiers <philippe.waroquiers-AgBVmzD5pcezQB+***@public.gmane.org> changed:

What |Removed |Added
----------------------------------------------------------------------------
Status|UNCONFIRMED |RESOLVED
CC| |***@skynet.
| |be
Resolution|--- |FIXED

--- Comment #11 from Philippe Waroquiers <philippe.waroquiers-AgBVmzD5pcezQB+***@public.gmane.org> ---
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

[Timur Iskhodzhanov]

https://bugs.kde.org/show_bug.cgi?id=280271

--- Comment #12 from Timur Iskhodzhanov <timurrrr-hpIqsD4AKlfQT0dZR+***@public.gmane.org> ---
Just curious - why is it [none] by default rather than [all] ?

[Philippe Waroquiers]

https://bugs.kde.org/show_bug.cgi?id=280271

--- Comment #13 from Philippe Waroquiers <philippe.waroquiers-AgBVmzD5pcezQB+***@public.gmane.org> ---
(In reply to comment #12)

Post by Timur Iskhodzhanov
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.

[Timur Iskhodzhanov]

https://bugs.kde.org/show_bug.cgi?id=280271

--- Comment #14 from Timur Iskhodzhanov <timurrrr-hpIqsD4AKlfQT0dZR+***@public.gmane.org> ---
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