Copy assignment operator memory leak, on big project

Question

I have been working on a big project for the past few months. Now I'm finally done with that project. But in my copy assignment operator I have a memory leak. Valgrind shows that it's the original value of data_ that leaks.

Here is my copy assignment operator code.

Value& Value::operator=(const Value& r)
{
  data_ = copy(r.data_);
  delete data_; 
  return *this;
}

Anyone that can help me with this problem? I would really appreciate it.

what is `data_`? a pointer i suppose... but why is it being copied (is it `std::copy`?) and deleted. what is `*this` i cant see any class there — tejas, Apr 19 '15 at 18:46

score 5 · Accepted Answer · answered Apr 19 '15 at 18:46

5

I believe you wanted to write this:

delete data_;     //here it makes sense: delete the current value
data_ = copy(r.data_);  //copy now

not this one:

data_ = copy(r.data_); //LEAK, as data_ is pointing to current value
delete data_;         //here you're deleting the copied one

Make sure that data_ always points to a valid memory — else delete it conditionally.

answered Apr 19 '15 at 18:46

Nawaz

353,942
115
666
851

1

Yeaa of course! I was just code blind. Thank you for the help buddy! :) – BlommaN Apr 19 '15 at 19:05

Marco A. · Answer 2 · 2015-04-19T18:52:57.877

This doesn't make sense:

data_ = copy(r.data_);
delete data_;

because if data_ was pointing to allocated memory and you overwrite it with

data_ = copy(r.data_);

and then delete the newly copied area, you have a memory leak (you can no longer refer to the allocated memory you originally had).

Big bonus for deleting the just-copied memory: if you ever were to actually use the _data pointer you'll get undefined behavior.

You probably meant to write

template <typename T>
Value<T>& Value<T>::operator=(const Value<T>& r)
{
  delete data_; // Free this object's memory
  data_ = copy(r.data_); // And now get a copy (hopefully a deep one) of the new memory
  return *this;
}

A small caveat: the above code even if fixed has no strong exception guarantee: if memory copy fails for whatever reason, you might end up with an inconsistent object (since data_ has already been deleted).

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

The problem is that data_ is deleted right after it is copied!

  data_ = copy(r.data_);
  delete data_;          <<< PROBLEM

The best solution may be to employ copy-and-swap idiom (What is the copy-and-swap idiom?).

template <typename T>
Value<T>& Value<T>::operator=(const Value<T> rhs)  // NOTE: pass by value
{
  swap(data_, rhs.data_);  // either std::swap or a custom swap,
                           // hard to say without knowing the type of data_
  return *this;
}

Another alternative, a direct enhancement to OP's code, may be the following.

template <typename T>
Value<T>& Value<T>::operator=(const Value<T>& r)
{
  // 1) Allocate new data. If, for some reason, the allocation throws,
  // the original data_ stays intact. This offers better
  // exception safety.
  ... new_data = ...;

  // 2) Copy r.data to new_data (note: deep copy desired)
  new_data = copy(r.data_); 

  // 3) Destroy original data_
  delete data_;

  // 4) Point data_ to new_data
  data_ = new_data;

  return *this;
}

Copy assignment operator memory leak, on big project

3 Answers3