memory leak

[jaapkroe]

Hi,
I am trying to write a nice class to process my data but I am making some logical error which causes memory leak, I hope somebody can help.

Here is a minimal code to reproduce the error:

"test.cpp"

class Atom
{
  public: 
    Atom() { x = new double[3]; }; // allocate some mem for Atom
    ~Atom() { delete[] x; };
    double* x;
};

class Frame
{
  public:
    Frame() {
      _atoms = new Atom[5]; // Allocate some Atoms for Frame
      _natoms = 0;
    }
    ~Frame() { delete[] _atoms; }
    void add_atom(Atom& a) { _atoms[_natoms++] = a; }

  private:
    int _natoms;
    Atom* _atoms;
};

int main () {
  Frame f;
  Atom a;
  f.add_atom(a);
}

This compiles and works fine, but a memory check with 'valgrind' gives:

bash$ g++ -Wall -g -fno-inline test.cpp
bash$ valgrind --leak-check=full ./a.out
==17968== Memcheck, a memory error detector
==17968== Copyright (C) 2002-2010, and GNU GPL'd, by Julian Seward et al.
==17968== Using Valgrind-3.6.0.SVN-Debian and LibVEX; rerun with -h for copyright info
==17968== Command: ./a.out
==17968==
==17968== Invalid free() / delete / delete[]
==17968== at 0x4025186: operator delete[](void*) (vg_replace_malloc.c:409)
==17968== by 0x80485F5: Atom::~Atom() (test.cpp:5)
==17968== by 0x80486E2: Frame::~Frame() (test.cpp:16)
==17968== by 0x8048592: main (test.cpp:27)
==17968== Address 0x42cc1d8 is 0 bytes inside a block of size 24 free'd
==17968== at 0x4025186: operator delete[](void*) (vg_replace_malloc.c:409)
==17968== by 0x80485F5: Atom::~Atom() (test.cpp:5)
==17968== by 0x8048586: main (test.cpp:27)
==17968==
==17968==
==17968== HEAP SUMMARY:
==17968== in use at exit: 24 bytes in 1 blocks
==17968== total heap usage: 7 allocs, 7 frees, 168 bytes allocated
==17968==
==17968== 24 bytes in 1 blocks are definitely lost in loss record 1 of 1
==17968== at 0x4025FE5: operator new[](unsigned int) (vg_replace_malloc.c:299)
==17968== by 0x80485CF: Atom::Atom() (test.cpp:4)
==17968== by 0x8048630: Frame::Frame() (test.cpp:13)
==17968== by 0x804855A: main (test.cpp:25)
==17968==
==17968== LEAK SUMMARY:
==17968== definitely lost: 24 bytes in 1 blocks
==17968== indirectly lost: 0 bytes in 0 blocks
==17968== possibly lost: 0 bytes in 0 blocks
==17968== still reachable: 0 bytes in 0 blocks
==17968== suppressed: 0 bytes in 0 blocks
==17968==
==17968== For counts of detected and suppressed errors, rerun with: -v
==17968== ERROR SUMMARY: 2 errors from 2 contexts (suppressed: 19 from 8)
bash$

I am messing something up with copying references I guess.
Somehow the array x is lost. Does anybody have advice?

[Galik]

Line 17: _atoms[_natoms++] = a;.

That causes your parameter a to overwrite one of the Atom objects in your class Fram's Atom array.

That means it overwrites its internal pointer double* x;. By overwriting double* x; you make the memory that it was pointing to inaccessible.

[LB]

You should define an operator= function that correctly copies data from one Atom to another. That = on line 17 is doing evil stuff...

[jaapkroe]

Thanks a lot! This fixed it:

class Atom
{
  public: 
    Atom() { x = new double[3]; }; // allocate some mem for Atom
    ~Atom() { delete[] x; };
    Atom& operator= (const Atom& other) {
      if (&other==this) return *this;
      for(int i=0; i<3; i++) x[i] = other.x[i];
      return *this;
    }
    double* x;
};

class Frame
{
  public:
    Frame() {
      _atoms = new Atom[5]; // Allocate some Atoms for Frame
      _natoms = 0;
    }
    ~Frame() { delete[] _atoms; }
    void add_atom(Atom& a) { _atoms[_natoms++] = a; }

  private:
    int _natoms;
    Atom* _atoms;
};

int main () {
  Frame f;
  Atom a;
  f.add_atom(a);
}

bash$ g++ -Wall -g -fno-inline test.cpp
bash$ valgrind --leak-check=full ./a.out
==853== Memcheck, a memory error detector
==853== Copyright (C) 2002-2010, and GNU GPL'd, by Julian Seward et al.
==853== Using Valgrind-3.6.0.SVN-Debian and LibVEX; rerun with -h for copyright info
==853== Command: ./a.out
==853==
==853==
==853== HEAP SUMMARY:
==853== in use at exit: 0 bytes in 0 blocks
==853== total heap usage: 7 allocs, 7 frees, 168 bytes allocated
==853==
==853== All heap blocks were freed -- no leaks are possible
==853==
==853== For counts of detected and suppressed errors, rerun with: -v
==853== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 19 from 8)
bash$

[TheNoobie]

Add return 0; at the end of your main function as well. I don't believe your destructor will be called otherwise if I remember right, I just started working with classes a few days ago. You could test this by adding a cout << "Destructor called." << endl; in your destructor code.

[Galik]

Adding return 0; is entirely optional in C++. Destructors will be called at the end of the main() scope regardless.

[TheNoobie]

Ya you're right, I just coded this tiny little program to test it..

#include <iostream>
using namespace std;

class TestClass
{
public:
	TestClass() { cout << "Constructor called." << endl; }
	~TestClass() { cout << "Destructor called." << endl; }
};

int main()
{
	TestClass MyClass;
	
	return 0;
}

Commenting out return 0; makes no difference on whether or not the destructor is called. I'm really not sure why I thought it did, I seem to remember reading it but I guess I was wrong.

[jaapkroe]

You could've seen that from the fact that the profiler doesn't complain. You could however expect the compiler to give a warning because I define a function int main() without returning an integer, but in this I guess main() is an exception.

[Galik]

The compiler doesn't give a warning because, as you rightly guessed, main() is an exception. Explicitly returning from main is entirely optional. If no return statement is given then return 0; is assumed.