Introduction
When a developer investigates memory leak issues, he/she could find the code that allocated the leaked memory with some tools. Most of the time, it is easy to fix. But sometimes, it is still not very clear where the real problem is and how to fix it.
In this article, I will demonstrate some of these scenarios. The analysis and fix will be put at the end of each scenario. All of the problems are from real projects. I have to simplify them due to confidential reasons. You can take a quick test to see whether you can figure out the errors immediately.
1. Destructor
1: class A{ };
2: class B : public A {
3: private:
4: std::string str;
5: public: B( std::string& s) {
6: str=s;
7: }
8: };
9: void LeakTest() {
10: std::str hello("hello");
11: A * pA = new B(hello);
12: if( pA ) {
13: delete pA;
14: }
15: }
Error report: You may use different tools to detect a memory leak (the diagram below is generated by Runtimechecker), but the result should be similar to this:
There are two panes in the window: the first is the call stack, the second is the source code.
Analysis (destructor is not virtual)
- The destructor of class
B
is not defined, so the compiler will generate one automatically, in which the destructor of std::string
will be called for str
defined in line 4, and the internal buffer that holds "Hello" will be freed. - The compiler also generates a default destructor for class
A
, but it is not virtual
. pA
is deleted in line 13. Because pA
is a pointer of class A
and the default destructor of class A
is not virtual
, only the destructor of class A
will be called, and thus the internal buffer allocated by str
will be leaked.- You may wonder why the leak was allocated in line 10 instead of line 11. The reason is,
std::basic_string
(the template of std::string
) has some reference counting implementation. You can see it by tracing the code.
Fix: Add a virtual destructor for class A
: virtual ~A(){}
.
2. Store pointers in container
1: class A{
2: int a;
3: };
4: class MyContainer{
5: public:
6: std::map<long,A*> <LONG,A*>leakMap;
7: ~MyContainer(){
8: std::map<long,A*>::iterator it;
9: for(it=leakMap.begin();it!=leakMap.end();it++){
10: if( it->second != NULL)
11: delete it->second;
12: }
13: }
14: };
15: MyContainer con;
16: void LeakTest() {
17: Test1();
18: Test2();
19: }
20: void Test1() {
21: A* pA1 = new A();
22: con.leakMap[1000] = pA1;
23: }
24: void Test2() {
25: A* pA2 = new A();
26: con.leakMap[1000] = pA2;
27: }
Analysis:
Sometimes it maybe necessary to store pointers in the container. When you replace an existing pointer in a container with a new pointer (in this sample, the operator [] of map in line 26 replaced pA1
with pA2
), make sure to delete the old pointer. When the sample puts pA2
into the container in line 26, it should check whether there is a pointer associated with key 1000; if yes, pA1
should be deleted first.
Fix: Add these lines between line 25 and line 26:
std::map<long,A*>::iterator it = con.leakMap.find(1000);
if( it != con.leakMap.end() )
delete it->second;
This bug looks simple in the above sample. But imagine, if you are working on a big application and Test1()
and Test2()
are from different source files, it could be hard to find it.
Note: This bug was originally from a server application with more than 150,000 lines of code.
3. void *
1: void ThreadFunc( void * p ) {
2: 3: if( p )
4: delete p;
5: }
6:void LeakTest(){
7: A* p = new A();
8: _beginthread( ThreadFunc,(void*)p );
9:
}
Analysis:
There is an obvious error; i.e., convert a pointer of class A
to void *
and delete the void *
, and the result is the destructor of class A
would not be called and the memory supposed to be freed in the destructor will be leaked. This scenario happens quite often, and the above sample is just one of the variants.
Fix: Change line 4 to: delete (A*)p;
.
More samples will be included in Part 2.