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.

Answer 1

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.

Answer 2

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

1. 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).
2. 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(...);
   ...
}