Double free or corruption- why?

Question

class matrix{
    private:
        int n, *wsk;
        friend istream & operator>>(istream&,matrix&);
        friend ostream & operator<<(ostream&,matrix&);
    public:
        matrix(){
            wsk=0;
            n=0;        
        }
        matrix(const matrix &mat){
            this->n=mat.n;
            if (wsk!=0) delete []wsk;
            this->wsk=new int [this->n*this->n];
            for (int i=0;i<n*n;i++)
                wsk[i]=mat.wsk[i];

        }

        ~matrix(){
            if (this->wsk!=0) delete[]this->wsk;
        }

        const matrix & operator=(const matrix &mat){
            if(&mat==this) return *this;
            if (this->wsk!=0) delete [] this->wsk;
            n=mat.n;
            this->wsk=new int [n*n];
            for (int i=0;i<mat.n*mat.n;i++)
                this->wsk[i]=mat.wsk[i];
            return *this;   
    } 
};


istream & operator>>(istream &str, matrix& mat){

    str >> mat.n;
    if (mat.n>0) {
        if (mat.wsk != 0) delete[]mat.wsk;
        mat.wsk= new int [mat.n*mat.n];
        for (int i=0;i<mat.n*mat.n;i++)
            str >> mat.wsk[i];
    }

    return str;
}

ostream & operator<<(ostream &str, matrix& mat){
    if (mat.wsk!=0){
        for (int i=0;i<mat.n*mat.n;i++){
            str << mat.wsk[i] << " ";
            if ((i+1)%mat.n==0) str << endl;
        }
    }
    return str;
}

When I'm trying to make two matrices in main, where firsts dimension is lower than second, double free is happening. When both matrices have the same dimension, or dimension of the first matrix is higher than the second, there is no problem. Maybe someone can see the code and tell me what's the problem?

Edit: Main:

int main(){
    matrix mac, a, b;
    cout << "Put number of dimensions and numbers in matrix ";  
    cin >> mac;
    cout << mac;
    cin >> a;   
    cout << a;
    mac.~matrix();
    return 0;
}

Have you stepped through with the debugger? It will show you exactly why there's a double free. You also don't need to check for null before deleting something. — chris, Mar 16 '15 at 17:17
The line `if (wsk!=0) delete []wsk;` inside the copy constructor does not make sense. The object did not exist before the copy constructor started executing, so `wsk` cannot possibly have a value that's worth checking. — fredoverflow, Mar 16 '15 at 17:18
Step 1: Replace your `int*` with `std::vector` Step 2: Profit — Cory Kramer, Mar 16 '15 at 17:19
Ok, problem is solved. Then only and one thing is that I should do "wsk=0" in deconstructor. @PaulMcKenzie it was for test that program won't crash when I will do double deconstruction. — oszust002, Mar 16 '15 at 17:45
@KamilOsuch That is *not* how you solve the problem -- if comments could get a down vote, your last comment would have gotten one. You should never call the destructor explicitly, except for things such as `placement-new`, which you are not doing. Just remove that erroneous line from `main` and leave your destructor alone. — PaulMcKenzie, Mar 16 '15 at 17:48
It was only TEST for checking how it works and the thing that if someone will do it, the problem won't come. The thing that I want to know now, when the first free is coming. Maybe you know how I could check it? — oszust002, Mar 16 '15 at 17:58
@KamilOsuch But your "test" is the reason for the problem. No C++ programmer would write code that explicitly calls the destructor, except for the case for `placement-new`. When you posted your original code without the `main` function, everyone is assuming that you're writing proper C++ code to test your class, thus letting us believe there is something wrong with the class itself (besides the copy constructor having an issue), But that wasn't the case -- there was something wrong with your `main` program that was causing the problem. — PaulMcKenzie, Mar 16 '15 at 18:29
Ok, I know. But my question now is: If i use delete, "wsk" won't go to null automatically? — oszust002, Mar 16 '15 at 19:22
@KamilOsuch All `delete` is required to do is call the destructor and deallocate the memory. Setting things to NULL is not required of `delete`. Second, it makes no sense to set `wsk` to NULL in the destructor, since the object is destroyed -- you can't use `wsk` anyway. — PaulMcKenzie, Mar 16 '15 at 20:24
why not? People say that this is good habit to do this to avoid double free http://stackoverflow.com/questions/4190703/is-it-safe-to-delete-a-null-pointer — oszust002, Mar 16 '15 at 20:37
@KamilOsuch You posted a link that has nothing to do with what I am saying. *What good is setting `wsk` to NULL in the destructor if the object is going to be destroyed?* The object is going away -- you can't use `wsk` anymore, so all you're doing is wasting cycles. Setting `wsk` to NULL only makes sense if the object is still going to be used afterwards. That is the information that you linked to. — PaulMcKenzie, Mar 16 '15 at 22:01
@KamilOsuch - `~matrix() { ... }` Once that function has returned, any non-static members are gone forever in that object. Since `wsk` is a non-static member, it goes away. So again, setting `wsk` to NULL *in the destructor* makes absolutely no sense. Also, your main() program invokes undefined behavior if you explicitly call the destructor. Stick a `std::vector` in your class, and you will see that you cannot simply set a vector to "NULL" to stop errors from occurring. — PaulMcKenzie, Mar 16 '15 at 22:10

PaulMcKenzie · Answer 1 · 2015-03-16T17:44:06.993

One error that I see is that in your copy constructor, you are deleting memory that was never allocated:

 this->n=mat.n;
 if (wsk!=0) delete []wsk;

Checking for non-NULL doesn't help you. That pointer may have a non-null garbage value, and you're calling delete[] using a garbage pointer. Just remove that line altogether from your copy constructor.

Second, your assignment operator has problems:

  const matrix & operator=(const matrix &mat){
            if(&mat==this) return *this;

            // you've destroyed your data here
            if (this->wsk!=0) delete [] this->wsk;

            // you've changed one of your members here
            n=mat.n;

            // what if the line below throws a `std::bad_alloc` exception?
            this->wsk=new int [n*n];

The comments explain the issue. You deleted your data, and you have no way to recover if new[] fails later on.

You also return const. This is unorthodoxed for an assignment operator to return a const object.

The better way to write your assignment operator would be this:

  #include <algorithm>
  //...
  matrix & operator=(matrix mat)
  {
     std::swap(n, mat.n);
     std::swap(wsk, mat.wsk);
     return *this;
  }

This is guaranteed to work, given a working copy constructor and destructor. The copy/swap idiom is used here.

Also, there is no need to check for a null pointer when issuing a delete or delete[]. So your destructor can simply be this:

~matrix(){ delete[]this->wsk; }

Edit: You are doing this in your main function:

mac.~matrix();

You are explicitly calling the destructor. So what happens when the mac object goes out of scope? The destructor will be called again, automatically, thus you get the double-delete error.

Remove this line from main. The destructor for the object will be called automatically.

I didn't ask about operator=. My only and one problem is that I have double free from nowhere. I had deleted the line in copy constructor and deleted operator= because it isn't important now. The problem is why after "cin >> matrix1" where i put for example n=2 and numbers in it, and then "cin >> matrix2" and put the higher dimension fo example n=3 why i get double free? — oszust002, Mar 16 '15 at 17:37
@KamilOsuch Please post a main() program that duplicates the error. You never posted how you're testing this class. As a matter of fact, you didn't post code showing how you actually create one of these `matrix` objects. All you posted was a default constructor and a copy constructor. — PaulMcKenzie, Mar 16 '15 at 17:37
@KamilOsuch - `I didn't ask about operator=. ` You have a *runtime* issue. We don't know how you're using this class, therefore we don't know if `operator =` plays a role in the crash or not. Thus, we fix what we see, and your `operator=` needed fixing. — PaulMcKenzie, Mar 16 '15 at 17:39

score 0 · Answer 2 · answered Mar 16 '15 at 17:57

To me seems delete[] tries to call the destructor for every element of the array, then it destroys the pointer. It could bring to double free error.

Have you tried to replace

int *foo=new int[n*m]

with old C malloc?

int *foo;
foo=(int*)malloc(n*m*sizeof(int));

This way you could use delete instead of delete[]. I hope this works.

Have fun and let me know

gf

Double free or corruption- why?

2 Answers2