C++ destructor won't free properly

Question

i'm trying to free an apartment class object properly, but valgrind says "invalid free, address...is on thread 1's stack" Here's the code: I'll be very grateful if you could point to my mistakes.

class Apartment{
public:
enum SquareType {EMPTY, WALL, NUM_SQUARE_TYPES};
class ApartmentException : public std::exception {};
class IllegalArgException : public ApartmentException {};
class OutOfApartmentBoundsException : public ApartmentException {};

int length;
int width;
int price;
SquareType** squares;

Apartment (SquareType** squares, int length, int width, int price);
Apartment (const Apartment& apartment);
Apartment& operator=(const Apartment& apartment);
~Apartment();
};

Apartment::Apartment (SquareType** squares=NULL, int length=0, int width=0, int price=0){
    this->price=price;
    this->length=length;
    this->width=width;

    this->squares = new SquareType*[length];
    for(int i=0; i<length ; i++){
        this->squares[i]= new SquareType[width];
    }
    this->squares = squares;
    for(int i=0; i<length; i++){
        for(int j=0; j<width; j++){

            this->squares[i][j] = squares[i][j];
        }
    }
}
Apartment::Apartment (const Apartment& apartment):length(apartment.length),
                    width(apartment.width),price(apartment.price),squares(apartment.squares){

    for(int i=0; i<apartment.length; i++){
        for(int j=0; j<apartment.width; j++){
            squares[i][j] = apartment.squares[i][j];
        }
    }

}
Apartment& Apartment::operator=(const Apartment& apartment){

    if(this == &apartment){
        return *this;
    }
    for(int i=0;i<length;i++){
        delete [] squares[i];
    }
    delete [] squares;

    squares = new SquareType*[apartment.length];

    for(int i=0; i<apartment.length ; i++){
        squares[i]= new SquareType[apartment.width];
    }

    for(int i=0; i<apartment.length; i++){
        for(int j=0; j<apartment.width; j++){
            squares[i][j] = apartment.squares[i][j];
        }
    }
    price=apartment.price;
    length=apartment.length;
    width=apartment.width;
    return *this;
}
Apartment::~Apartment(){
    for(int i=0;i<length;i++){
        delete [] squares[i];
    }
    delete [] squares;
}

that's the main:

int main(){

    Apartment::SquareType square1[5]={Apartment::WALL};
    Apartment::SquareType square2[5]={Apartment::WALL};
    Apartment::SquareType square3[5]={Apartment::WALL};
    Apartment::SquareType square4[5]={Apartment::WALL};

    Apartment::SquareType* squares[4]={square1,square2,square3,square4};
    Apartment::SquareType* Squares[3]={square1,square2,square3};

Apartment ap(squares,4,5,0);
Apartment ap2(Squares,3,5,50);

return 0;

}

and that's the valgrind output:valg

Hi, why not vector of vectors? In c-tor you first create whole 2D areay and then: this->squares = squares;. This causes memory leaks for sure — paweldac, Jun 05 '16 at 07:11

score 0 · Answer 1 · answered Jun 05 '16 at 07:08

0

Your copy constructor must copy the contents of squares as you did in the assignment operator instead of just assigning the address in apartment.squares.

answered Jun 05 '16 at 07:08

MikeCAT

73,922
11
45
70

I did, i've copied the whole content, in that nested for loop – CodeHoarder Jun 05 '16 at 07:12
No, you didn't. You just performed assignment between the same memory region. Create new buffer for new object first. – MikeCAT Jun 05 '16 at 07:14

score 0 · Answer 2 · edited May 23 '17 at 11:58

0

The mistake is in the copy constructor. You (correctly) have this:

    this->squares = new SquareType*[length];

but then you shortly thereafter have this:

    this->squares = squares;

causing both a memory leak and potentially a dangling pointer.

More broadly:

You shouldn't be dealing so heavily in new. Rather, you should use the standard library's abstractions; std::vector seems well-suited to your case, and in fact would let you completely eliminate the destructor.
You should take a look at "What is the copy-and-swap idiom?"; it's a way to make these core functions simpler and more robust.

edited May 23 '17 at 11:58

Community

1
1

answered Jun 05 '16 at 07:11

ruakh

175,680
26
273
307

Where `this->squares = new SquareType*[length];` is not a copy constructor, but the constructor also seems wrong... – MikeCAT Jun 05 '16 at 07:15
Thanks , but i'd like to learn how to do this right with simple arrays first. What's the solution to the memory leaks you've mentioned? Lose this assignment and just copy the contents ? – CodeHoarder Jun 05 '16 at 07:41

PaulMcKenzie · Answer 3 · 2016-06-05T08:16:54.933

In addition to the issue with your Apartment constructor overwriting the pointer returned from new[], your copy constructor should do what's in your assignment operator. Instead, you're just assigning the pointer and not copying the data (you're doing a shallow copy, and not a deep copy).

Apartment::Apartment(const Apartment& rhs) : price(rhs.price), length(rhs.length), width(rhs.width), squares(new SquareType*[rhs.length])
{
    for(int i=0; i<rhs.length ; i++)
       squares[i]= new SquareType[rhs.width];

    for(int i=0; i<rhs.length; i++)
    {
        for(int j=0; j<rhs.width; j++)
            squares[i][j] = rhs.squares[i][j];
    }
}

Once you have this, there is no need for your assignment operator to duplicate this same code. All your assignment operator could do is this:

#include <algorithm>
//...
Apartment& Apartment::operator=(const Apartment& rhs)
{
   Apartment temp(rhs);
   std::swap(temp.price, price);
   std::swap(temp.length, length);
   std::swap(temp.width, width);
   std::swap(temp.squares, squares);
   return *this;
}

This uses the copy / swap idiom.

But overall, it is easy to break your code even with these changes. What if a NULL is passed as squares in the Apartment constructor, and length and/or width are greater than 0? You will be assigning to your squares from the passed in NULL pointer, causing undefined behavior.

It would be better if you resorted to using containers such as std::vector that knows their size automatically without the client having to give you this information explicitly.

As I'm certain there is a cunning reason for this, why did you not pass `rhs` by value and use it instead of `temp`? — user4581301, Jun 05 '16 at 08:04
You could do it that way -- just wanted the assignment operator signature to look more like the OP's. — PaulMcKenzie, Jun 05 '16 at 08:17
Rats. I was hoping there was some sneaky compiler optimization I'd been missing out on. — user4581301, Jun 05 '16 at 19:32

C++ destructor won't free properly

3 Answers3