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)

Answer 1

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";
    }
};

Answer 2

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.