Deep copy constructor - dynamic array inside a class C++ Invalid free() / delete / delete[] / realloc()

Question

I wanted to write a simple class to represent a point A(x,y). However, I have some problems with copy constructor. It gives me invalid free() / delete / delete[] / realloc() error when trying analyze my code with valigrind. Please, take a look:

#include <iostream>
#include <cmath>
using namespace std;

class Point
{
private :
    int *coord;
public:
    Point();
    Point(int x, int y);
    Point(const Point& p);
    ~Point();
    void printpoint();

};

Point::Point()
{
    coord = new int[2];
    coord[0] = 0;
    coord[1] = 0;
}

Point::Point(int x, int y)
{
    coord = new int[2];
    coord[0] = x;
    coord[1] = y;
}

Point::Point(const Point& p)
{
    if (this != &p)
    {
        if(coord != NULL)
        {
            delete[] coord;
            coord = NULL;
        }
        coord = new int[2];
        coord[0] = p.coord[0];
        coord[1] = p.coord[1];
    }
}

Point::~Point()
{
    if(coord != NULL)
    {
        delete[] coord;
        coord = NULL;
    }
}

void Point::printpoint()
{
    std::cout << "Point (" << coord[0] << "," << coord[1] << ")\n";
}


int main()
{
    Point p1;
    p1.printpoint();

    Point *p2 = new Point();
    p2->printpoint();

    Point p3(3,4);
    p3.printpoint();

    Point *p4 = new Point(6,7);
    p4->printpoint();

    Point *p5 = &p1;
    p5->printpoint();

    Point *p6 = p4;
    p6->printpoint();

    Point *p7 = new Point(p3);
    p7->printpoint();

    Point p8 = Point(p3);
    p8.printpoint();

    Point p9 = p1;
    p9.printpoint();

    Point p10 = *p4;
    p10.printpoint();

    Point p11(p1);
    p11.printpoint();

    Point p12(*p6);
    p12.printpoint();

    Point *p13 = new Point(*p7);
    p13->printpoint();

    delete p2;
    delete p4;
    delete p5;
    delete p6;
    delete p7;
    delete p13;

    return 0;
}

And the valgrind's output:

==3978== Memcheck, a memory error detector
==3978== Copyright (C) 2002-2013, and GNU GPL'd, by Julian Seward et al.
==3978== Using Valgrind-3.10.1 and LibVEX; rerun with -h for copyright info
==3978== Command: ./Untitled2
==3978== 
Point (0,0)
Point (0,0)
Point (3,4)
Point (6,7)
Point (0,0)
Point (6,7)
==3978== Conditional jump or move depends on uninitialised value(s)
==3978==    at 0x400A92: Point::Point(Point const&) (Untitled2.cpp:36)
==3978==    by 0x400C94: main (Untitled2.cpp:82)
==3978== 
Point (3,4)
==3978== Conditional jump or move depends on uninitialised value(s)
==3978==    at 0x400A92: Point::Point(Point const&) (Untitled2.cpp:36)
==3978==    by 0x400CBD: main (Untitled2.cpp:85)
==3978== 
==3978== Conditional jump or move depends on uninitialised value(s)
==3978==    at 0x400A9E: Point::Point(Point const&) (Untitled2.cpp:38)
==3978==    by 0x400CBD: main (Untitled2.cpp:85)
==3978== 
==3978== Conditional jump or move depends on uninitialised value(s)
==3978==    at 0x4C2C7F2: operator delete[](void*) (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==3978==    by 0x400AAE: Point::Point(Point const&) (Untitled2.cpp:38)
==3978==    by 0x400CBD: main (Untitled2.cpp:85)
==3978== 
==3978== Invalid free() / delete / delete[] / realloc()
==3978==    at 0x4C2C83C: operator delete[](void*) (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==3978==    by 0x400AAE: Point::Point(Point const&) (Untitled2.cpp:38)
==3978==    by 0x400CBD: main (Untitled2.cpp:85)
==3978==  Address 0x602088 is in the Data segment of /home/kasiula/Dropbox/STH/codz/Untitled2
==3978== 
Point (3,4)
==3978== Conditional jump or move depends on uninitialised value(s)
==3978==    at 0x400A92: Point::Point(Point const&) (Untitled2.cpp:36)
==3978==    by 0x400CE2: main (Untitled2.cpp:88)
==3978== 
==3978== Conditional jump or move depends on uninitialised value(s)
==3978==    at 0x400A9E: Point::Point(Point const&) (Untitled2.cpp:38)
==3978==    by 0x400CE2: main (Untitled2.cpp:88)
==3978== 
==3978== Conditional jump or move depends on uninitialised value(s)
==3978==    at 0x4C2C7F2: operator delete[](void*) (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==3978==    by 0x400AAE: Point::Point(Point const&) (Untitled2.cpp:38)
==3978==    by 0x400CE2: main (Untitled2.cpp:88)
==3978== 
==3978== Invalid free() / delete / delete[] / realloc()
==3978==    at 0x4C2C83C: operator delete[](void*) (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==3978==    by 0x400AAE: Point::Point(Point const&) (Untitled2.cpp:38)
==3978==    by 0x400CE2: main (Untitled2.cpp:88)
==3978==  Address 0x1 is not stack'd, malloc'd or (recently) free'd
==3978== 
Point (0,0)
==3978== Conditional jump or move depends on uninitialised value(s)
==3978==    at 0x400A92: Point::Point(Point const&) (Untitled2.cpp:36)
==3978==    by 0x400D01: main (Untitled2.cpp:91)
==3978== 
==3978== Conditional jump or move depends on uninitialised value(s)
==3978==    at 0x400A9E: Point::Point(Point const&) (Untitled2.cpp:38)
==3978==    by 0x400D01: main (Untitled2.cpp:91)
==3978== 
==3978== Conditional jump or move depends on uninitialised value(s)
==3978==    at 0x4C2C7F2: operator delete[](void*) (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==3978==    by 0x400AAE: Point::Point(Point const&) (Untitled2.cpp:38)
==3978==    by 0x400D01: main (Untitled2.cpp:91)
==3978== 
==3978== Invalid free() / delete / delete[] / realloc()
==3978==    at 0x4C2C83C: operator delete[](void*) (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==3978==    by 0x400AAE: Point::Point(Point const&) (Untitled2.cpp:38)
==3978==    by 0x400D01: main (Untitled2.cpp:91)
==3978==  Address 0x601de8 is not stack'd, malloc'd or (recently) free'd
==3978== 
Point (6,7)
==3978== Conditional jump or move depends on uninitialised value(s)
==3978==    at 0x400A92: Point::Point(Point const&) (Untitled2.cpp:36)
==3978==    by 0x400D23: main (Untitled2.cpp:94)
==3978== 
==3978== Conditional jump or move depends on uninitialised value(s)
==3978==    at 0x400A9E: Point::Point(Point const&) (Untitled2.cpp:38)
==3978==    by 0x400D23: main (Untitled2.cpp:94)
==3978== 
==3978== Conditional jump or move depends on uninitialised value(s)
==3978==    at 0x4C2C7F2: operator delete[](void*) (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==3978==    by 0x400AAE: Point::Point(Point const&) (Untitled2.cpp:38)
==3978==    by 0x400D23: main (Untitled2.cpp:94)
==3978== 
==3978== Invalid free() / delete / delete[] / realloc()
==3978==    at 0x4C2C83C: operator delete[](void*) (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==3978==    by 0x400AAE: Point::Point(Point const&) (Untitled2.cpp:38)
==3978==    by 0x400D23: main (Untitled2.cpp:94)
==3978==  Address 0x53611a8 is not stack'd, malloc'd or (recently) free'd
==3978== 
Point (0,0)
==3978== Conditional jump or move depends on uninitialised value(s)
==3978==    at 0x400A92: Point::Point(Point const&) (Untitled2.cpp:36)
==3978==    by 0x400D42: main (Untitled2.cpp:97)
==3978== 
==3978== Conditional jump or move depends on uninitialised value(s)
==3978==    at 0x400A9E: Point::Point(Point const&) (Untitled2.cpp:38)
==3978==    by 0x400D42: main (Untitled2.cpp:97)
==3978== 
==3978== Conditional jump or move depends on uninitialised value(s)
==3978==    at 0x4C2C7F2: operator delete[](void*) (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==3978==    by 0x400AAE: Point::Point(Point const&) (Untitled2.cpp:38)
==3978==    by 0x400D42: main (Untitled2.cpp:97)
==3978== 
==3978== Invalid free() / delete / delete[] / realloc()
==3978==    at 0x4C2C83C: operator delete[](void*) (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==3978==    by 0x400AAE: Point::Point(Point const&) (Untitled2.cpp:38)
==3978==    by 0x400D42: main (Untitled2.cpp:97)
==3978==  Address 0xffefffb50 is on thread 1's stack
==3978==  in frame #2, created by main (Untitled2.cpp:63)
==3978== 
Point (6,7)
==3978== Conditional jump or move depends on uninitialised value(s)
==3978==    at 0x400A92: Point::Point(Point const&) (Untitled2.cpp:36)
==3978==    by 0x400D6A: main (Untitled2.cpp:100)
==3978== 
Point (3,4)
==3978== Invalid free() / delete / delete[] / realloc()
==3978==    at 0x4C2C2BC: operator delete(void*) (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==3978==    by 0x400DC5: main (Untitled2.cpp:105)
==3978==  Address 0xffefffae0 is on thread 1's stack
==3978==  in frame #1, created by main (Untitled2.cpp:63)
==3978== 
==3978== Invalid read of size 8
==3978==    at 0x400B06: Point::~Point() (Untitled2.cpp:49)
==3978==    by 0x400DD6: main (Untitled2.cpp:106)
==3978==  Address 0x5a1c180 is 0 bytes inside a block of size 8 free'd
==3978==    at 0x4C2C2BC: operator delete(void*) (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==3978==    by 0x400DAC: main (Untitled2.cpp:104)
==3978== 
==3978== Invalid free() / delete / delete[] / realloc()
==3978==    at 0x4C2C2BC: operator delete(void*) (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==3978==    by 0x400DDE: main (Untitled2.cpp:106)
==3978==  Address 0x5a1c180 is 0 bytes inside a block of size 8 free'd
==3978==    at 0x4C2C2BC: operator delete(void*) (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==3978==    by 0x400DAC: main (Untitled2.cpp:104)
==3978== 
==3978== 
==3978== HEAP SUMMARY:
==3978==     in use at exit: 0 bytes in 0 blocks
==3978==   total heap usage: 15 allocs, 22 frees, 120 bytes allocated
==3978== 
==3978== All heap blocks were freed -- no leaks are possible
==3978== 
==3978== For counts of detected and suppressed errors, rerun with: -v
==3978== Use --track-origins=yes to see where uninitialised values come from
==3978== ERROR SUMMARY: 25 errors from 25 contexts (suppressed: 0 from 0)

Why are you making `point` so complication? It only needs to store two ints! Why use dynamic memory for this? — Ed Heal, Jan 08 '16 at 20:21
Note: You do not need to check if your pointer is `NULL` before `delete`. Calling `delete` on `NULL` is OK (see [here](http://stackoverflow.com/questions/4190703/is-it-safe-to-delete-a-null-pointer)). Calling `delete` on an uninitialized pointer, though (as you do here) isn't going to work. — R_Kapp, Jan 08 '16 at 20:25
If you want a "dynamic array" you should really be using [`std::vector`](http://en.cppreference.com/w/cpp/container/vector). Then you can use [the rule of zero](http://en.cppreference.com/w/cpp/language/rule_of_three#Rule_of_zero) and don't have to worry about these things. — Some programmer dude, Jan 08 '16 at 20:35

score 3 · Accepted Answer · edited May 23 '17 at 11:59

3

Your copy constructor should not be attempting to delete the pointer array. Since you are in the copy constructor and the array was never initialized in a member initialization list you can just use

Point::Point(const Point& p)
{
    coord = new int[2];
    coord[0] = p.coord[0];
    coord[1] = p.coord[1];
}

As Ed Heal pointed out there is no reason to even have any dynamic memory allocation in your class. Your point class could simply be

class Point
{
private :
    int x;
    int y;
public:
    Point(int x_ = 0, int y_ = 0) : x(x_), y(y_) {}
    void printpoint() { std::cout << "Point (" << x << "," << y << ")\n"; }
};

Now we do not need a copy constructor or a destructor as the compiler provided ones will work just fine.

You are also deleting pointers that you never created with new. p5 points to p1 so you cannot call delete on it. Then you delete p4 which is okay but when you delete p6 which points to p4 you are doing a double delete which is a big no no.

edited May 23 '17 at 11:59

Community

1
1

answered Jan 08 '16 at 20:23

NathanOliver

171,901
28
288
402

Will it be the deep copy constructor then? Because I need one here – Brian Brown Jan 08 '16 at 20:24
There is nothing deep about it. – Ed Heal Jan 08 '16 at 20:24
It still gives errors: http://pastie.org/private/oqdbbyljmbnrchuqfeeq. I changed my copy constructor to yours. – Brian Brown Jan 08 '16 at 20:25
1

@BrianBrown: That is because you `delete` things you did not allocate. See for example `p5` and `p6`. You did not call `new` to allocate memory for them, yet you call `delete` on them. **DON'T** do this. Only call `delete` on things that have been `new`-ed. – R_Kapp Jan 08 '16 at 20:29
This is not exactly true - delete is happy to delete on a null pointer. – Ed Heal Jan 08 '16 at 20:31
@EdHeal: Yeah, I mentioned that above on the question itself; I meant don't call it on things that have been allocated on the stack (like `p5`), or on a pointer that is copied from another and the original pointer itself (like `p6` and `p4`). – R_Kapp Jan 08 '16 at 20:33
1

@BrianBrown Added more to the answer. You had some bad calls to delete in `main()` – NathanOliver Jan 08 '16 at 20:33
Thank you all for your answers – Brian Brown Jan 08 '16 at 20:44

score 3 · Answer 2 · edited Jan 08 '16 at 20:48

3

This is not an answer - but a bit too big for a comment.

Why not do this - a lot simpler:

class Point
{
private :
    int m_x, m_y;
public:
    Point() : m_x(0), m_y(0) {}
    Point(int x, int y) : m_x(x), m_y(y) {}
    Point(const Point& p) : m_x(p.m_x), m_y(p.m_y) {}
    ~Point() {}
    void printpoint() const {
        std::cout << "Point (" << m_x << "," << m_y << ")\n";
    }
};

edited Jan 08 '16 at 20:48

NathanOliver

171,901
28
288
402

answered Jan 08 '16 at 20:26

Ed Heal

59,252
17
87
127

Thank you, I know that its simpler, I know. But I want to do it with array, and have some problems with copy constructor... – Brian Brown Jan 08 '16 at 20:28
2

Why not `int coord[2]`? Why make a rod for your own back? Guess you like to make debugging harder for yourself – Ed Heal Jan 08 '16 at 20:29

Deep copy constructor - dynamic array inside a class C++ Invalid free() / delete / delete[] / realloc()

2 Answers2