C++ memory leak with 2d array pointer

Question

The three functions below contain memory leaks at the lines marked with "// memory leak vvv" according to Dr. Memory. I'm relatively new to C++ and pointers and am not sure why these lines are causing leaks. "values_" is a T** and is a member variable for the UndoArray class.

template <class T> void UndoArray<T>::set(size_type i, const T& v) {
    counts_[i]++;
    if(!initialized(i)) {
        // memory leak vvv
        values_[i] = new T[1];
        values_[i][0] = v;
    } else {
        // memory leak vvv
        T* temp = new T[counts_[i]];
        for(int j = 0; j < counts_[i] - 1; j++) {
            temp[j] = values_[i][j];
        }
        temp[counts_[i] - 1] = v;
        delete [] values_[i];
        values_[i] = temp;
    }
}

template <class T> void UndoArray<T>::undo(size_type i) {
    counts_[i]--;
    if(counts_[i] == 0) {
        values_[i] = NULL;
    } else {
        T* temp = values_[i];
        // memory leak vvv
        values_[i] = new T[counts_[i]];
        for(int j = 0; j < counts_[i]; j++) {
            values_[i][j] = temp[j];
        }
        delete [] temp;
    }
}

template <class T> void UndoArray<T>::copy(const UndoArray<T>& ua) {
    size_ = ua.size_;
    counts_ = new unsigned[size_];
    for(int i = 0; i < size_; i++) {
        counts_[i] = ua.counts_[i];
    }
    values_ = new T*[size_];
    for(int i = 0; i < size_; i++) {
        if(counts_[i] == 0) {
            values_[i] = NULL;
        } else {
            // memory leak vvv
            values_[i] = new T[counts_[i]];
            for(int j = 0; j < counts_[i]; j++) {
                values_[i][j] = ua.values_[i][j];
            }
        }
    }
}

The constructor for UndoArray uses...

template <class T> void UndoArray<T>::create() {
    size_ = 0;
    counts_ = new unsigned[size_];
    values_ = new T*[size_];
}

... if the default constructor is called (no arguments) or ...

template <class T> void UndoArray<T>::create(size_type n) {
    size_ = n;
    counts_ = new unsigned[size_];
    for(int i = 0; i < size_; i++)
        counts_[i] = 0;
    values_ = new T*[size_];
    for(int i = 0; i < size_; i++)
        values_[i] = NULL;
}

... if an initial array size is specified.

The destructor looks like...

template <class T> UndoArray<T>::~UndoArray() {
    delete [] counts_;
    if(values_ != NULL) {
        for(int i = 0; i < size_; i++) {
            delete [] values_[i];
        }
    }
    delete [] values_;
}

@ZacHowland I edited my original question and added the constructors. — Jonathan Wrona
– Jonathan Wrona, Commented Sep 24, 2013 at 18:50
Not the constructors ... the Destructors. That is, the method that gets called when your class is destroyed and is suppose to clean up it's resources. — Zac Howland
– Zac Howland, Commented Sep 24, 2013 at 18:53

ds27680 · Accepted Answer · 2013-09-24 19:00:59Z

2

There are several things that are not ok in the code:

I.e.

template <class T> void UndoArray<T>::copy(const UndoArray<T>& ua) {
size_ = ua.size_;
counts_ = new unsigned[size_];
for(int i = 0; i < size_; i++) {
    counts_[i] = ua.counts_[i];
}
//What if values_ is not null here? You do not delete the old data
values_ = new T*[size_];

And there are some more situations in the code you posted where you do something similar.

Edit1: To give you another example

template <class T> void UndoArray<T>::undo(size_type i) {
counts_[i]--;
if(counts_[i] == 0) {
    //what if values_[i] != nullptr here? You will leak the old value...
    values_[i] = NULL;

Of course you should make sure that you delete each pointer in the destructor.

like:

~UndoArray()
{
  if (nullptr != values_)
  {
      for (int i = 0; i < size_; ++i)
      {
         delete [] values[i];
      }

      delete [] values;
  }
}

edited Sep 24, 2013 at 19:00

answered Sep 24, 2013 at 18:55

ds27680

1,99310 silver badges11 bronze badges

Sign up to request clarification or add additional context in comments.

5 Comments

Jonathan Wrona Over a year ago

I was missing deleting each pointer in values_ e.g. the for loop that you mentioned in your destructor. After I added that the only leak I have is in the set function where I try values_[i] = new T[1];.

ds27680 Over a year ago

Please note that you should revise your create method you posted with one of your edits. You are basically allocating a zero sized array. I will not go into the details but you should probably read this: stackoverflow.com/questions/1087042/…

Zac Howland Over a year ago

If values_i[i] has an allocated memory block attached to it, it will leak there. You need to test it, delete what is there, and then you can assign it a new memory block.

ds27680 Over a year ago

Please see the above two other examples where it may be that you should delete the allocated memory. I saw at least one more such example of missing deletes.

ds27680 Over a year ago

I think you are going to constantly get into trouble if you will have to do this type of memory management all over the UndoArray class implementation. Probably it would be better to wrap the values_ stuff in a separate class with a simple and clear interface and do the memory management in that single place... Probably you could replace values_ with a std::vector<T>* values_

rileyberton · Accepted Answer · 2013-09-24 18:52:19Z

0

template <class T> void UndoArray<T>::set(size_type i, const T& v) {
    counts_[i]++;
    if(!initialized(i)) {
        // memory leak vvv`

this is a leak because values_[i] is never deleted.

    values_[i] = new T[1];
    values_[i][0] = v;
} else {
    // memory leak vvv

same problem here. there is no delete temp; statement.

    T* temp = new T[counts_[i]];
    for(int j = 0; j < counts_[i] - 1; j++) {
        temp[j] = values_[i][j];
    }
    temp[counts_[i] - 1] = v;
    delete [] values_[i];
    values_[i] = temp;
}

You must call delete on the newed memory. Unless the destructor of the class cleans up values_.

answered Sep 24, 2013 at 18:52

rileyberton

4753 silver badges5 bronze badges

5 Comments

Zac Howland Over a year ago

He copies values_i[i] to temp and deletes temp.

rileyberton Over a year ago

Not in the ::set method. In the ::undo method, yes.

Zac Howland Over a year ago

He has a delete [] values_i[i] in the set method as well.

Jonathan Wrona Over a year ago

The destructor of the class now cleans up values_ well and my only leak now is in the set function when I try values[i] = new T[1];.

Zac Howland Over a year ago

@rileyberton: For the 2nd allocation (in the else-condition), I meant. I apparently improperly assumed initialized(i) tested to see if the values_i[i] block had been allocated.

Collectives™ on Stack Overflow

C++ memory leak with 2d array pointer

2 Answers 2

5 Comments

5 Comments

Your Answer

Linked

Hot Network Questions

Collectives™ on Stack Overflow

2 Answers 2

5 Comments

5 Comments

Your Answer

Sign up or log in

Post as a guest

Linked

Related