Double free or corruption after queue::push

Asked 28/12, 2012 at 2:21 Answered 18/8, 2015 at 7:23

#include <queue>
using namespace std;

class Test{
    int *myArray;

        public:
    Test(){
        myArray = new int[10];
    }

    ~Test(){
        delete[] myArray;
    }

};


int main(){
    queue<Test> q
    Test t;
    q.push(t);
}

After I run this, I get a runtime error "double free or corruption". If I get rid of the destructor content (the delete) it works fine. What's wrong?

Screwy answered 28/12, 2012 at 2:21 Comment(14)

Read this and then read this. – Cockcroft 28/12, 2012 at 2:22

Test t(); declares a function, your code won't compile. you need to post real code. – Skiba 28/12, 2012 at 2:29

While the slideshow in the second link has some good points, I don't agree with all of them, in particular the example on page 9 showing a local huge object being returned by value. That invokes copy construction that may be costly, unless you are using C++11 move semantics. – Kaylakayle 28/12, 2012 at 2:32

@RemyLebeau, The slideshow talks about smart pointers like std::unique_ptr, which are part of the C++11 standard. I think it's safe to assume it is implying move semantics at the very least, if not RVO. – Cockcroft 28/12, 2012 at 2:33

Read Rule of Three – Burnout 28/12, 2012 at 2:33

@chris: std::unique_ptr is new in C++11, but std::shared_ptr is not. Also, the slideshow mentions std::array, which is not new in C++11, either. So I don't think move semantics can be assumed. Even with RVO used, copy assignment would be invoked without C++11 move assignment. – Kaylakayle 28/12, 2012 at 2:38

@RemyLebeau, Both std::shared_ptr and std::array are only C++11. There may be similar things in boost, but those wouldn't have a std:: prefix. – Cockcroft 28/12, 2012 at 2:39

@chris: actually, they were introduced in TR1 and then updated in C++11 to match boost functionality. – Kaylakayle 28/12, 2012 at 2:44

@RemyLebeau, Fair enough. I never did use TR1, but I see your point. Even then, though, it would be in the tr1 namespace, not std ;) – Cockcroft 28/12, 2012 at 2:45

Does this even compile? Shouldn't Test t(); be Test t? – Jamisonjammal 28/12, 2012 at 3:45

@Robᵩ Fixed. It compiles and has the same problem now. – Burnout 28/12, 2012 at 5:52

Unfortunately the answers provided below are the wrong way to fix this problem. You should make myArray a std::vector<int> rather than a int* then all the problems are solved (included the correct C++ move constructors are included in std::vector). – Burnout 28/12, 2012 at 5:54

@chris, second link is 404, do you mind send a fresh link, it looks interesting. – Remorseless 23/11, 2020 at 14:10

@SAMPro, Here you go. I should really get into the habit of archiving things like that when I link them. – Cockcroft 23/11, 2020 at 16:57

Let's talk about copying objects in C++.

Test t;, calls the default constructor, which allocates a new array of integers. This is fine, and your expected behavior.

Trouble comes when you push t into your queue using q.push(t). If you're familiar with Java, C#, or almost any other object-oriented language, you might expect the object you created earler to be added to the queue, but C++ doesn't work that way.

When we take a look at std::queue::push method, we see that the element that gets added to the queue is "initialized to a copy of x." It's actually a brand new object that uses the copy constructor to duplicate every member of your original Test object to make a new Test.

Your C++ compiler generates a copy constructor for you by default! That's pretty handy, but causes problems with pointer members. In your example, remember that int *myArray is just a memory address; when the value of myArray is copied from the old object to the new one, you'll now have two objects pointing to the same array in memory. This isn't intrinsically bad, but the destructor will then try to delete the same array twice, hence the "double free or corruption" runtime error.

How do I fix it?

The first step is to implement a copy constructor, which can safely copy the data from one object to another. For simplicity, it could look something like this:

Test(const Test& other){
    myArray = new int[10];
    memcpy( myArray, other.myArray, 10 );
}

Now when you're copying Test objects, a new array will be allocated for the new object, and the values of the array will be copied as well.

We're not completely out trouble yet, though. There's another method that the compiler generates for you that could lead to similar problems - assignment. The difference is that with assignment, we already have an existing object whose memory needs to be managed appropriately. Here's a basic assignment operator implementation:

Test& operator= (const Test& other){
    if (this != &other) {
        memcpy( myArray, other.myArray, 10 );
    }
    return *this;
}

The important part here is that we're copying the data from the other array into this object's array, keeping each object's memory separate. We also have a check for self-assignment; otherwise, we'd be copying from ourselves to ourselves, which may throw an error (not sure what it's supposed to do). If we were deleting and allocating more memory, the self-assignment check prevents us from deleting memory from which we need to copy.

Meaganmeager answered 28/12, 2012 at 2:37 Comment(9)

I fixed the original question. So I fixed this as well Test t(); is not a variable declaration but a forward declaration of a function. – Burnout 28/12, 2012 at 5:49

Correct explanation of what is going wrong and how to fix it (if you were going to fix it). But it is the wrong solution to the problem. The correct solution in any real code is to change the type of the member myArray to std::vector<int>. This way you correctly get all the more efficient move components (constructor/assignment) that are defined in C++11. – Burnout 28/12, 2012 at 5:56

@LokiAstari - Good point, but I think it's more important to illustrate the actual problem and how to traditionally fix it. Once that's understood, it becomes obvious why the vector is a better solution. – Meaganmeager 28/12, 2012 at 6:58

Would you say that the compiler did a shallow copy as opposed to a deep copy, which is what the code meant to do? – Somaliland 26/3, 2014 at 0:23

@Somaliland - Yes, it is a shallow copy. The important part is how the compiler's behavior causes this kind of memory error, because deep copies aren't always the desired result. – Meaganmeager 26/3, 2014 at 0:35

@Meaganmeager Indeed. I was trying to think in terms of what the code wanted to do and what the compiler actually did. Thanks! – Somaliland 26/3, 2014 at 1:35

@derekerdmann: Why you didn't write delete[] myarray; in overloaded assignment operator? otherwise there is memory leak. Isn't it? Correct me If I am wrong – Precincts 24/6, 2015 at 7:37

@PravasiMeet - There isn't a memory leak because the object on the right side of the assignment can still be used after this assignment. This implementation copies the value of myArray into the new object's array, rather than transferring the actual array between objects. The array will be freed by other's destructor. If you want to actually move the array between the objects, then you might want to look into move semantics: https://mcmap.net/q/15702/-what-is-move-semantics – Meaganmeager 24/6, 2015 at 17:18

This is broken. memcpy copies bytes not integers. Better to use std::copy (just as efficient but safer). – Xiomaraxiong 13/5, 2019 at 0:8

The problem is that your class contains a managed RAW pointer but does not implement the rule of three (five in C++11). As a result you are getting (expectedly) a double delete because of copying.

If you are learning you should learn how to implement the rule of three (five). But that is not the correct solution to this problem. You should be using standard container objects rather than try to manage your own internal container. The exact container will depend on what you are trying to do but std::vector is a good default (and you can change afterwords if it is not opimal).

#include <queue>
#include <vector>

class Test{
    std::vector<int> myArray;

    public:
    Test(): myArray(10){
    }    
};

int main(){
    queue<Test> q
    Test t;
    q.push(t);
}

The reason you should use a standard container is the separation of concerns. Your class should be concerned with either business logic or resource management (not both). Assuming Test is some class you are using to maintain some state about your program then it is business logic and it should not be doing resource management. If on the other hand Test is supposed to manage an array then you probably need to learn more about what is available inside the standard library.

Burnout answered 28/12, 2012 at 6:2 Comment(0)

You are getting double free or corruption because first destructor is for object q in this case the memory allocated by new will be free.Next time when detructor will be called for object t at that time the memory is already free (done for q) hence when in destructor delete[] myArray; will execute it will throw double free or corruption. The reason is that both object sharing the same memory so define \copy, assignment, and equal operator as mentioned in above answer.

Immunogenetics answered 28/12, 2012 at 2:30 Comment(0)

You need to define a copy constructor, assignment, operator.

class Test {
   Test(const Test &that); //Copy constructor
   Test& operator= (const Test &rhs); //assignment operator
}

Your copy that is pushed on the queue is pointing to the same memory your original is. When the first is destructed, it deletes the memory. The second destructs and tries to delete the same memory.

Prewar answered 28/12, 2012 at 2:23 Comment(2)

The equal operator Ryan is referring to is likel the assignment operator. You need to define a default constructor, a copy constructor, and a copy assignment operator. If you are using C++11, you can also define a move contructor and a move assignment operator as well. – Kaylakayle 28/12, 2012 at 2:41

Sorry, you don't need it, but when comparing for equality you're generally after the contents of what the pointers point to, not the memory location so I include it most of the time when using pointers. – Prewar 28/12, 2012 at 2:41

You can also try to check null before delete such that

if(myArray) { delete[] myArray; myArray = NULL; }

or you can define all delete operations ina safe manner like this:

#ifndef SAFE_DELETE
#define SAFE_DELETE(p) { if(p) { delete (p); (p) = NULL; } }
#endif

#ifndef SAFE_DELETE_ARRAY
#define SAFE_DELETE_ARRAY(p) { if(p) { delete[] (p); (p) = NULL; } }
#endif

and then use

SAFE_DELETE_ARRAY(myArray);

Heaume answered 18/8, 2015 at 7:23 Comment(1)

Checking for null is redundant because calling delete with a null pointer doesn't do anything. Also setting the pointer to null after deleting it is redundant in a destructor because it will be invalidated when the function ends. – Xiomaraxiong 13/5, 2019 at 0:16

-2

Um, shouldn't the destructor be calling delete, rather than delete[]?

Mustee answered 28/12, 2012 at 2:45 Comment(3)

No, it was allocated with new[], ergo it needs a delete[]. – Cockcroft 28/12, 2012 at 2:46

Noooooooo! It is rightly calling delete [] myArray. See the new, it is not new int but new int [x]. – Chabot 28/12, 2012 at 2:46

parashift.com/c++-faq/delete-array.html Please read this if you didn't understand what the comments are talking about, and why you're getting downvoted (btw, I didn't downvote). – Chabot 28/12, 2012 at 2:48

Hot tags

Godot Unity Godot Help Programming Godot 4.X GUI GDScript 3D 2D Physics CSharp Godot 3.X VR XR Projects C++

Let's talk about copying objects in C++.

How do I fix it?

Recommended topics

Hot tags