Crash When Deleting Pointer in Destructor

Question

I've bumped up against my lack of deep understanding of pointers in C++. I've written a class called Skymap, which has the following definition:

class Skymap {
 public:
  Skymap();
  ~Skymap();
  void DrawAitoffSkymap();
 private:
  TCanvas mCanvas;

  TBox* mSkymapBorderBox;
};

with the functions defined as:

#include "Skymap.h"

Skymap::Skymap()
{
  mCanvas.SetCanvasSize(1200,800);
  mMarkerType=1;
}

Skymap::~Skymap()
{
  delete mSkymapBorderBox;
}

void Skymap::DrawAitoffSkymap()
{
  TBox* mSkymapBorderBox=new TBox(-200,-100,200,100);
  //Use the mSkymapBorderBox pointer for a while
}

(I am using the ROOT plotting package, but I think this is just a general C++ question).

Now, the following program will crash upon reaching the destructor of skymap2:

int main(){
  Skymap skymap1;
  Skymap skymap2;
  skymap1.DrawAitoffSkymap();
  skymap2.DrawAitoffSkymap();
  return(0);
}

However, the following will not crash:

int main(){
  Skymap skymap1;
  skymap1.DrawAitoffSkymap();
  return(0);
}

Also, if I initialise the pointer mSkymapBorderBox to NULL in the constructor, I no longer experience a crash following the execution of the first program (with 2 Skymap objects).

Can anyone please explain what the underlying cause of this is? It appears to be a problem with the pointer in the second Skymap object, but I do not see what it is.

It's not the cause of this problem, but always remember the [Rule of Three](http://stackoverflow.com/questions/4172722) when you're managing resources. And better still, don't manage resources yourself - use [smart pointers](http://stackoverflow.com/questions/8839943), containers and other RAII classes. — Mike Seymour, Feb 22 '12 at 18:38
Well, you *should* always set pointers to `NULL` if not allocating at that time in the constructor. — crashmstr, Feb 22 '12 at 18:38

score 16 · Accepted Answer · edited May 23 '17 at 10:30

16

TBox* mSkymapBorderBox=new TBox(-200,-100,200,100);

Here you're allocating memory to a local variable, not to the member variable. And since you're not allocating memory for the member variable, calling delete on it would invoke undefined behavior, which results in crash in your case.

What you should be doing is this:

mSkymapBorderBox=new TBox(-200,-100,200,100);

which now allocates memory for the member variable. It is one of the reason why local variables should be named differently than member variables. Naming conventions helps avoiding such bugs.

As a sidenote, or rather a very important note, since your class manages resources, consider implementing copy-semantics along with destructor properly: this rule is popularly referred to as the-rule-of-three. Or use some smart pointers, such asstd::shared_ptr, std::unique_ptr or whatever fits in your scenario.

edited May 23 '17 at 10:30

Community

1
1

answered Feb 22 '12 at 18:35

Nawaz

353,942
115
666
851

Oh wow, thanks very much for that! Somewhat embarrassing, but I appreciate the quick reply! – Wheels2050 Feb 22 '12 at 18:36
Even the most experienced developer make mistakes like that. It is never embarrassing to ask questions ;-) your welcome – Jan Koester Feb 22 '12 at 18:39
4

But if you don't check for `mSkymapBorderBox` already having been allocated (plus initializing to `NULL`), calling the DrawAitoffSkymap additional times would cause memory leaks. – crashmstr Feb 22 '12 at 18:40

score 7 · Answer 2 · answered Feb 22 '12 at 18:43

Nawaz's answer is correct. But beyond that, your code has several possible problems:

If anyone creates a SkyMap and never calls DrawAitoffSkymap with it, then you'll get undefined behavior (since mSkymapBorderBox is never initialized, it will have a random value, which you then delete).
If anyone calls DrawAitoffSkymap more than once with a given SkyMap, then you'll get a memory leak.

To fix:

(1) Initialize mSkymapBorderBox to zero in your constructor.

(2) Decide what DrawAitoffSkymap should do if it's called multiple times. If it should reuse the old mSkymapBorderBox, then you would want to say something like:

void Skymap::DrawAitoffSkymap() {
   if (!mSkymapBorderBox) mSkymapBorderBox = new TBox(...);
   ...
}

On the other hand, if a new TBox should be created each time, then you want:

void Skymap::DrawAitoffSkymap() {
   delete mSkymapBorderBox; // note: does nothing if mSkymapBorderBox == 0
   mSkymapBorderBox = new TBox(...);
   ...
}

Thanks very much for that - I haven't dealt with pointers all that much before, so I need to get in the habit of adding such checks etc. to my code. I'll keep your advice in mind! — Wheels2050, Feb 22 '12 at 18:45

score 0 · Answer 3 · answered Oct 02 '14 at 08:18

0

TBox* mSkymapBorderBox=new TBox(-200,-100,200,100); you are creating a new TBox* pointer, which is not the data member.

Consider to properly implement the delete after you implement a new, in the same logical unit/scope...

answered Oct 02 '14 at 08:18

kiriloff

25,609
37
148
229

score 0 · Answer 4 · edited Aug 14 '16 at 13:23

TBox* mSkymapBorderBox=new TBox(-200,-100,200,100);

when you declared this this create an object of the class TBox. When you exit DrawAitoffSkymap at that time this lose the reference of this allocated memory.

When the destructor is called at time it frees some garbage memory.

To avoid this use this
mSkymapBorderBox=new TBox(-200,-100,200,100);
instead of TBox* mSkymapBorderBox=new TBox(-200,-100,200,100);

Crash When Deleting Pointer in Destructor

4 Answers4