Pointer private atribute in a class, where this class is using in a vector, generate error double free or orruption

Question

I have implement to class in C++, std c++11, the code two test winstrument class works well, but the code for station.cpp doesn't work, it fails in

stationA.insertInstrument(instrumentC);

but before this I insert more two instruments, the internal container is a vector and I use the method push_back to store the instrument:

void WStation::insertInstrument(const WInstrument& instrument)
{
   _instruments.push_back(instrument);
}

This is the prototype of the class

class WInstrument
{
  public:
    WInstrument(int serialNumber, std::string description, Type type);
    ~WInstrument();

    bool operator==(const WInstrument& rhs) const;
    bool operator!=(const WInstrument& rhs) const;

    double read() const;

    std::string getDescription() const;
    int getSerialNumber() const;
    Type getType() const;

  private:
    int _serialNumber = -1; 
    Type _type;
    std::string _description;

  private:
    std::uniform_real_distribution<double> *_dist;
};

This is the default constructor:

WInstrument::WInstrument(int serialNumber, std::string description, Type type):
  _serialNumber(serialNumber),
  _type(type),
  _description(description)
{
  switch(_type)
  {
    case wind:
      {
    _dist = new std::uniform_real_distribution<double>(0.0, 200.0);
    break;
      }
    case temperature:
      {
    _dist = new std::uniform_real_distribution<double>(-20.0, 100.0);
    break;
      }
    case humidity:
      {
    _dist = new std::uniform_real_distribution<double>(0.0, 100.0);
    break; 
      }
  }
}

I belive the problem is *_dist, because if I don't use pointer and implement direct, where I will use the function read, the code works. In the destructor I use delete _dist. The question is that I don't understand the problem.

The complete code is avaliable in https://github.com/retiarus/test-winstrument.

The shown class clearly violates the [Rule Of Three](https://stackoverflow.com/questions/4172722/what-is-the-rule-of-three), which is likely the reason for the memory corruption; but since the shown code in the question fails to meet the requirements for a [mcve] in the question itself, it's not possible to state that authoritatively. — Sam Varshavchik, Apr 07 '18 at 03:09
*I belive the problem is *_dist,*. -- Put a breakpoint in the `WInstrument` destructor, and you should see that it is being called when you didn't expect it to be called, thus destroying `_dist` while it should still be in use. The `std::vector` made a copy of `WInstrument` objects, and in making copies, the destructor is called. Read the *Rule Of Three* link above. — PaulMcKenzie, Apr 07 '18 at 03:27

Chris Drew · Accepted Answer · 2018-04-07T04:20:48.520

The problem is that WInstrument contains an owning raw-pointer to uniform_real_distribution that is being deleted in your destructor but your class does not have a proper copy constructor or copy assignment operator (see The Rule of Three).

Because you have not defined a copy constructor or copy assignment operator, the compiler has automatically created one for you but it is just dumbly copying the owning raw-pointer.

Here:

void WStation::insertInstrument(const WInstrument& instrument)
{
  _instruments.push_back(instrument);
}

You are taking a copy of a WInstrument into a std::vector and now you have two copies of WInstrument both with a pointer to the same uniform_real_distribution. When the destructors of these two objects are called you get a double deletion and hence the error.

The easiest solution is just to not use a pointer at all and just store a uniform_real_distribution directly:

class WInstrument
{
  public:
    WInstrument(int serialNumber, std::string description, Type type);

  //...

  private:
    std::uniform_real_distribution<double> _dist;
};

WInstrument::WInstrument(int serialNumber, std::string description, Type type):
  _serialNumber(serialNumber),
  _type(type),
  _description(description)
{
  switch(_type)
  {
    case wind:
      {
    _dist = std::uniform_real_distribution<double>(0.0, 200.0);
    break;
      }
      // ...
  }
}

Then you don't need to define a destructor at all and your problem goes away.

Live demo.

Minor detail: If you store a uniform_real_distribution by value it forces you to deal with const correctness because the operator() on uniform_real_distribution is not const. One solution is to make WInstrument::read() non-const. Another solution is to make dist_ mutable so you can use it in a const member function. Although you have to be careful, because normally const member functions are thread-safe and this is not.

score 0 · Answer 2 · answered Apr 07 '18 at 03:48

When you push_back a const WInstrument &, you are copying that WInstrument into the vector. As you do not implement a copy constructor, it is using the default copy constructor. This will copy your serial number, type, and description, and, most importantly, your distribution pointers. And these pointers are copied just as that: pointers.

When the WInstrument you called insertInstrument on goes out of scope, its destructor is called, and the distribution is deallocated. But the pointer in the WInstrument in the vector is still pointing to this deallocated memory.

The simplest fix is to switch to using a smart pointer for _dist. Other solutions are creating a copy constructor that handled copying the _dist properly, or using move semantics in insertInstrument.

Pointer private atribute in a class, where this class is using in a vector, generate error double free or orruption

2 Answers2