Click here to Skip to main content
Click here to Skip to main content

Typical memory leak scenarios analysis: Part 1

By , 10 Apr 2007
 

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)

  1. 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.
  2. The compiler also generates a default destructor for class A, but it is not virtual.
  3. 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.
  4. 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: // Do business logic
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.

License

This article, along with any associated source code and files, is licensed under The Code Project Open License (CPOL)

About the Author

Frank _ Li
Web Developer
Canada Canada
Member
I am a core developer of RuntimeChecker,which detects memory leak for applications developped by visual c++.

Sign Up to vote   Poor Excellent
Add a reason or comment to your vote: x
Votes of 3 or less require a comment

Comments and Discussions

 
You must Sign In to use this message board.
Search this forum  
    Spacing  Noise  Layout  Per page   
QuestionA better fix?memberJun Du25 Apr '06 - 3:51 
I think it's better to delete p outside ThreadFunc():
void LeakTest(){
    A* p = new A();
    _beginthread( ThreadFunc, (void*)p );
    delete p;
}
for two reasons:
 
1) Delete it where it was created (if all possible).
2) You don't have to cast p inside ThreadFunc() (unless necessary).
 
Best,
AnswerRe: A better fix?memberFrank _ Li25 Apr '06 - 6:45 
Thanks Jun.
I think p can not be delete in function LeakTest because another thread ThreadFunc is still using it. Sometimes people create a copy of the passing object in thread function( ThreadFunc in this case) and delete the original one in function LeakTest, but delete can only be done after the copy finished, so an event need to be introduced to synchronize two threads.

GeneralRe: A better fix?memberJun Du25 Apr '06 - 7:16 
Frank,
 
You're right that the code is incomplete. The working code in this case should be something like:
void void LeakTest() {
    A* p = new A();
    HANDLE handle = (HANDLE)_beginthread( ThreadFunc, (void*)p );
    
    // Demo code - wait for thread termination.
    WaitForSingleObject(handle, INFINITE);
 
    delete p;
}
Sorry for the missing part.
 
Best,
GeneralRe: A better fix?membergri10 Apr '07 - 22:08 
But where is the sense now? You start a thread and pause the current one ... in that case you don't need to start a new one Wink | ;)
 
So long and thanks for the fish!

GeneralRe: A better fix?memberFrank _ Li11 Apr '07 - 7:55 

The new thread will notify the parent thread to continue after it copies the object.
 

GeneralRedundant NULL checkmemberJohann Gerell24 Apr '06 - 20:23 
There's no need to check for NULL before attempting a delete, like this:
if( it->second != NULL)
    delete it->second;
It's perfectly legal and recommended to simply do
delete it->second;

 
--
The Blog: Bits and Pieces
JokehimemberThe_Myth20 Mar '06 - 20:54 
I'm agree with Rick York
Laugh | :laugh:
 


GeneralTwo SuggestionsmemberRick York20 Mar '06 - 20:26 
Frank : Your article is a decent start but it needs to be filled in a bit to give it more depth. Specifically, I think you need to provide an example of the code in action and trace dumps (or other output) to illustrate how these errors manifest themselves in a real app. For example, what is going to tell a programmer that they forgot to delete an object ?
 
Secondly : the formatting needs some work. I think single-spacing would be the best here. I advocate the use of the {pre} tag for code snippettes but that's a personal preference.
 
Best of luck.

GeneralRe: Two SuggestionsmemberFrank _ Li21 Mar '06 - 10:32 
Thanks Rick for your suggestions!
I changed the format based on your idea.
Regarding your first suggestion, all of the samples are from real projects, but I can only use the simplified code for the confidential reason.
But I think you are right, I need to give more information on this topic.
 
Thanks,
Frank

General General    News News    Suggestion Suggestion    Question Question    Bug Bug    Answer Answer    Joke Joke    Rant Rant    Admin Admin   

Permalink | Advertise | Privacy | Mobile
Web02 | 2.6.130523.1 | Last Updated 10 Apr 2007
Article Copyright 2006 by Frank _ Li
Everything else Copyright © CodeProject, 1999-2013
Terms of Use
Layout: fixed | fluid