Double Free inside of a destructor upon adding to a vector

Hey, I am working on a drum machine, and am having problems with vectors.

Each Sequence has a list of samples, and the samples are ordered in a vector. However, when a sample is push_back on the vector, the sample's destructor is called, and results in a double free error.

Here is the Sample creation code:

class XSample
{
  public:
    Uint8 Repeat;
    Uint8 PlayCount;
    Uint16 Beats;
    Uint16 *Beat;
    Uint16 BeatsPerMinute;

    XSample(Uint16 NewBeats,Uint16 NewBPM,Uint8 NewRepeat);
    ~XSample();

    void GenerateSample();

    void PlaySample();
};

XSample::XSample(Uint16 NewBeats,Uint16 NewBPM,Uint8 NewRepeat)
{
    Beats = NewBeats;
    BeatsPerMinute = NewBPM;
    Repeat = NewRepeat-1;
    PlayCount = 0;

    printf("XSample Construction\n");
    Beat = new Uint16[Beats];
}

XSample::~XSample()
{
    printf("XSample Destruction\n");
    delete [] Beat;
}

And the 'Dynamo' code that creates each sample in the vector:

class XDynamo
{
  public开发者_如何学Python:
    std::vector<XSample> Samples;

    void CreateSample(Uint16 NewBeats,Uint16 NewBPM,Uint8 NewRepeat);
};

void XDynamo::CreateSample(Uint16 NewBeats,Uint16 NewBPM,Uint8 NewRepeat)
{
    Samples.push_back(XSample(NewBeats,NewBPM,NewRepeat));
}

Here is main():

int main()
{
    XDynamo Dynamo;

    Dynamo.CreateSample(4,120,2);
    Dynamo.CreateSample(8,240,1);

    return 0;
}

And this is what happens when the program is run:

Starting program: /home/shawn/dynamo2/dynamo 
[Thread debugging using libthread_db enabled]
XSample Construction
XSample Destruction
XSample Construction
XSample Destruction
*** glibc detected *** /home/shawn/dynamo2/dynamo: double free or corruption (fasttop): 0x0804d008 ***

However, when the delete [] is removed from the destructor, the program runs perfectly.

What is causing this? Any help is greatly appreciated.

You need a proper copy constructor and assignment operator since you have a non-trivial destructor (more accurately because your class wraps a memory allocation). See the 'Rule of the Big 3':

• http://en.wikipedia.org/wiki/Rule_of_three_%28C%2B%2B_programming%29

Update:

As Martin York mentioned in the comments, this answer really just addresses the immediate cause of the problem but doesn't really suggest the best way to fix it, which is to use RAII class members that manage the resources automatically. On the face of it (given the example code), the Beat member might be a std::vector<> instead of a pointer to a manually allocated array. A vector<> member would allow the class to not need a special dtor, copy ctor or assignment operator - all those pieces would be automatically provided for the Beat member if it were a vector<>.

The problem is you are dynamically allocating memory in your object but not declaring a copy constructor/ assignment operator. When you allocate memory and are responsible for deleting it you need to define all FOUR methods that the compiler generates.

class XSample
{
    public:
        // Pointer inside a class.
        // This is dangerous and usually wrong.
        Uint16 *Beat;
};

XSample::XSample(Uint16 NewBeats,Uint16 NewBPM,Uint8 NewRepeat)
{
    // You allocated it here.
    // But what happens when you copy this object or assign it to another variable.
    Beat = new Uint16[NewBeats];
}

XSample::~XSample()
{
    // Delete here. Turns into double delete if you don't have
    // copy constructor or assignment operator.
    delete [] Beat;
}

What happens to the above when you do:

XSample   a(15,2,2);
XSample   b(a);  // Copy constructor called.
XSample   c(15,2,2);

c = a; // Assignment operator called.

Two ways to solve this:

1. Create the copy constructor/assignment operator.
2. Use another object that does memory management for you.

I would use solution 2 (as it is simpler).
Its also a better design. Memory management should be done by their own class and you should concentreate on your drums.

class XSample
{
  public:
    std::vector<Uint16> Beat;
};

XSample::XSample(Uint16 NewBeats,Uint16 NewBPM,Uint8 NewRepeat):
        Beat(NewBeats)
{
         // Notice the vector is constructed above in the initializer list.
}

    // Don't need this now.
XSample::~XSample()
{
}

If you want to do it the hard way:
Dynamically allocating an array of objects

If you want to see what the compiler versions look here:
C++ implicit copy constructor for a class that contains other objects

The compiler added a default copy constructor which means that XSample::Beats got aliased during samples.push_back(...). You should add a copy constructor that initializes XSample correctly, probably by copying XSample::Beats from the argument.

vector is copy-constructing your XSamples (using the compiler generated default copy constructor), and as a result, causing problems when it destructs the copy. You can store pointers to XSample in the vector or write a proper copy constructor.

What is happening is that Samples.push_back() copies its argument into the vector. Since XSample doesn't have a copy constructor defined, the compiler creates a default constructor, which does a shallow copy. This means that the Beat pointer in both the original and the copy in the vector point to the same memory. The original is then destructed at the end push_back(), deleting the Beat pointer.

At the end of main, Dynamo is destructed, calling the destructor of each of the elements. This tries to delete the already deleted Beat pointer, resulting in your double delete error.