Error freeing memory

Question

I am getting a weird error when I add a custom class with dynamic memory into a vector.

The error is

error for object 0x7fee9ac000e0: pointer being freed was not allocated *** set a breakpoint in malloc_error_break to debug

Trying to add a Symbol with dynamic memory produces the error, and removing the delete[] parameters in the destructor removes the error but I think incurs in memory leaking.

The code is below

//test.cpp
#include <iostream>
#include <cstdio>
#include <cstdlib>
#include "symbol.h"

int main(int argc, char const *argv[])
{
    Symbol a('A');
    std::cout << a << std::endl;
    Symbol b('B',2);
    std::cout << b << std::endl;
    double p[3] = {1.2, 2.4, 4.8};
    Symbol c('C',3,p);
    std::cout << c << std::endl;
    Symbol* d = new Symbol('D',3,p);

    ////
    std::cout << *d << std::endl;
    std::vector<Symbol> axiom;
    axiom.push_back(a)
    axiom.push_back(b); // This lines produces the error

    delete d;
    return 0;
}

//symbol.h
#ifndef SYMBOL_H
#define SYMBOL_H

#include <iostream>

class Symbol{
    friend std::ostream& operator<<(std::ostream& output, const Symbol& s);

    private:
        char character;
        int numpar;
        double* parameters;

    public:
        Symbol();
        Symbol(const char c);
        Symbol(const char c, const int n);
        Symbol(const char c, const int n, const double p[]);
        ~Symbol();

        bool operator == (const Symbol &other) const;

};

#endif


//symbol.cpp
#include "symbol.h"

Symbol::Symbol()
{
    character = 0;
    numpar = 0;
}

Symbol::Symbol(const char c)
{
    character = c;
    numpar = 0;
}

Symbol::Symbol(const char c, const int n)
{
    character = c;
    if(n > 0)
    {
        numpar = n;
        parameters = new double[numpar];
        std::fill(parameters, parameters+numpar, 0.0);
    }
    else
    {
        numpar = 0;
    }
}

Symbol::Symbol(const char c, const int n, const double p[])
{
    character = c;
    if(n > 0)
    {
        numpar = n;
        parameters = new double[numpar];
        std::copy(p,p+numpar,parameters);
    }else{
        numpar = 0;
    }

}

Symbol::~Symbol()
{
    if(this->numpar > 0)
    {
        delete[] parameters; //If I comment this line the code runs smoothly but I think it produces memory leaks
    }
}

bool Symbol::operator==(const Symbol &other) const {
    if (character == other.character)
        return true;
    return false;
}

std::ostream& operator<<(std::ostream& output, const Symbol &s){
    output << s.character;
    if(s.numpar > 0)
    {
        output << '(';
        for(int i = 0 ; i < s.numpar-1 ; i++)
        {
            output << s.parameters[i] << ", ";
        }
        output << s.parameters[s.numpar-1] << ')';
    }
    return output;
}

Answer 1

When you store your Symbol instance holding dynamic memory into the vector:

axiom.push_back(b);

It is copied. Since you haven't declared your own copy constructor, the default copy ctor will copy all members, so now two instances own the same double* parameters pointer, and both of them will eventually try to delete[] it.

If you use a std::vector<double> parameters instead, the default copy constructor will rely on vector's copy constructor, which will do The Right Thing ™.

In general it's desirable to rely on standard library types to deal with ownership concerns for you.

Answer 2

You're using a vector< Symbol > so when you push_back a Symbol, it actually create a new Symbol and since you didn't defined a copy constructor it simply copies the fields without questionning anything.

So you actually have two Symbol instances with a pointer to the same parameters array, when one get destroyed, the other try to free something which is already freed.

See : http://www.tutorialspoint.com/cplusplus/cpp_copy_constructor.htm