Memory leak in class Constructor of c++

Question

I have the following class

class CSample
{
   char* m_pChar;
   double* m_pDouble;

 CSample():m_pChar(new char[1000]), m_pDouble(new Double[1000000])
{
}
~CSample()
{
   if(m_pChar != NULL) delete [] m_pchar;
   if(m_pDouble != NULL) delete [] m_pDouble;
}
};

and in my main() function i'm trying to create object of CSample

int main()
{
    try
  {
    CSample objSample;
  }

catch(std::bad_alloc)
{
  cout<<"Exception is caught !!! Failed to create object";
}

}

Say while allocating the memory for m_pDouble in constructor's initializer list, it throws the exception because of insufficient available memory. But for m_pChar it is already allocated. Since object itself is not created, the destructor wouldnt be called. Then there will be memory leak for m_pChar.

How do you avoid this memory leak ?

@jxh Actually, no. `std::default_delete` has a specialization for `T[]` that does the right thing. — T.C., Jul 22 '14 at 06:08
In general, either a class manages *one* resource, or it has business related methods. Trying to do both at once is a recipe for disaster. — Matthieu M., Jul 22 '14 at 06:21
@T.C.: On ideone, I have to pass the specialization in to make `unique_ptr` work with an array allocation. — jxh, Jul 22 '14 at 06:49
@jxh Well, you do need `std::unique_ptr`, but you shouldn't have to pass `default_delete` explicitly... — T.C., Jul 22 '14 at 06:53

score 5 · Accepted Answer · edited May 23 '17 at 12:27

5

You can easily avoid this kind of problem by using a vector instead.

class CSample
{
   std::vector<char> m_pChar;
   std::vector<double> m_pDouble;

   CSample():m_pChar(1000), m_pDouble(1000000)
   {
   }
};

Generally speaking, you should aim to write classes that do not require a destructor. This makes them trivially obey the Rule of Three.

edited May 23 '17 at 12:27

Community

1
1

answered Jul 22 '14 at 05:48

jxh

69,070
8
110
193

2

Effectively making it obey the [Rule of Zero](http://flamingdangerzone.com/cxx11/2012/08/15/rule-of-zero.html). – Benjamin Gruenbaum Jul 22 '14 at 06:16

score 3 · Answer 2 · edited May 23 '17 at 10:25

There are several ways to do this safely:

Delegate memory management to another class, such as std::unique_ptr (C++11) or std::vector:
```
class CSample
{
   std::unique_ptr<char []> m_pChar;
   std::unique_ptr<double []> m_pDouble;

   CSample():m_pChar(new char[1000]), m_pDouble(new double[1000000])
   {
   }
};
```
The language guarantees that any class member already constructed will be destroyed if an exception is thrown, which will release the allocated memory.

Perform memory allocation in the constructor body instead, and use a local try block:

class CSample
{
   char* m_pChar;
   double* m_pDouble;

    CSample() : m_pChar(nullptr), m_pDouble(nullptr)
    {    
        try {
            m_pChar = new char[1000];
            m_pDouble = new double[1000000];
        }
        catch(...){
            if(m_pChar) delete [] m_pChar;
            if(m_pDouble) delete [] m_pDouble;
            throw;
        }    
    }
    CSample(const CSample &other) { /* perform deep copy */ }
    CSample &operator=(const CSample &other) { /* perform deep copy of other and release my resources */ }        
    ~CSample()
    {
       if(m_pChar) delete [] m_pchar;
       if(m_pDouble) delete [] m_pDouble;
    }

};

Use a C++11 delegating constructor. The object is deemed constructed when the target (non-delegating) constructor completes execution, so if the delegating constructor later throws, the destructor will be called.

class CSample
{
    char* m_pChar;
    double* m_pDouble;

    CSample(int) : m_pChar(nullptr), m_pDouble(nullptr) { }
    CSample() : CSample(0)
    {    
        m_pChar = new char[1000];
        m_pDouble = new double[1000000];
    }
    CSample(const CSample &other) { /* perform deep copy */ }
    CSample &operator=(const CSample &other) { /* perform deep copy of other and release my resources */ }
    ~CSample()
    {
       if(m_pChar) delete [] m_pchar;
       if(m_pDouble) delete [] m_pDouble;
    }

};

If you do not delegate resource management to another class, you would also need to supply proper copy constructors and copy assignment operators, as the default ones (memberwise copy/assignment) have the wrong semantics. It should be obvious that the first way is both the simplest by far and the least error prone.

Would you mind explaining why you'd rather do this than the other answer which uses a standard container? — Benjamin Gruenbaum, Jul 22 '14 at 06:16
@BenjaminGruenbaum To illustrate how hard it is to do it right without using a `vector` or `unique_ptr`? — T.C., Jul 22 '14 at 06:20
If you're going to describe how to do it the hard way, then at least mention the [Rule of Three](http://stackoverflow.com/questions/4172722) - the examples posted here are deathtraps. — Mike Seymour, Jul 22 '14 at 06:29

Memory leak in class Constructor of c++

2 Answers2