0

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_;
}
Brian Tompsett - 汤莱恩
  • 5,753
  • 72
  • 57
  • 129
Jonathan Wrona
  • 423
  • 2
  • 7
  • 19

2 Answers2

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;
  }
}
ds27680
  • 1,993
  • 10
  • 11
  • 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];`. – Jonathan Wrona Sep 24 '13 at 19:03
  • 2
    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: http://stackoverflow.com/questions/1087042/c-new-int0-will-it-allocate-memory – ds27680 Sep 24 '13 at 19:05
  • 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. – Zac Howland Sep 24 '13 at 19:10
  • 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 Sep 24 '13 at 19:11
  • 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* values_ – ds27680 Sep 24 '13 at 19:18
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_.

rileyberton
  • 485
  • 3
  • 5