"if(pointer) delete ponter" trys to delete even when pointer is not initialized

Question

I wrote a very small Observeable implementation. When an observer is registered it deletes the old observer and instates the new observer. However it tries to delete the pointer even if it has not been initialized. The code is as below:

Observable.h

class Observable
{
public:
    Observable();
    virtual void registerObserver(Observer * O);
    virtual ~Observable();

protected:
    Observer * myObserver;
};

Observable.cpp

#include "Observable.h"

Observable::Observable()
{
}


Observable::~Observable()
{
    if(myObserver)
        delete myObserver;
}

void Observable::registerObserver(Observer * O)
{
    if(myObserver)
        delete myObserver;
    myObserver=O;
}

all main does is

GUI * gui = new GUI();      // GUI extends Observer
Model * m = new Model();    //Model extends Observable
m->registerObserver(gui);   //I get a segfault inside this call

If I step through registerObserver, I see that even though I have never initialized myObserver, the statement if(myObserver) evaluates to true. This leads to the uninitialized pointer being deleted and a seg fault.

It's worth noting that if I run the release build I don't get a segfault. I only get the error in the debug build.

I was under the impression that if(myObserver) would only evaluate to true if the pointer was intact. (ie initialized and not deleted).

You're getting a segfault because of something else. `delete` **already** does a `nullptr` check. `if(ptr) delete ptr` is exactly the same as `delete ptr`. — Yuushi, Jun 06 '13 at 02:03
Right: not initialized is not the same as initialized to 0. That test is never necessary. — Pete Becker, Jun 06 '13 at 02:17

score 6 · Answer 1 · edited Jun 21 '22 at 19:01

Others have explained why you're getting the segmentation fault because of an uninitialized pointer, and how to fix that. You still have other bugs waiting to happen, because you're not following the rule of three. If you make a copy of the Observable class both instances will now contain a copy of myObserver, and both will attempt to delete the pointer in their respective destructors, resulting in undefined behavior, and possibly a crash.

A better implementation is to follow the rule of zero and not manage the pointer yourself.

#include <memory>

class Observable
{
public:
    Observable();
    virtual void registerObserver( std::unique_ptr<Observer> O );
    virtual ~Observable();

protected:
    std::unique_ptr<Observer> myObserver;
};

Observable::Observable()
// no need to initialize pointer
{}

Observable::~Observable()
{
  // no need to delete pointer manually
}

void Observable::registerObserver( std::unique_ptr<Observer> O )
{
    myObserver.reset( O.release() );
}

score 4 · Accepted Answer · answered Jun 06 '13 at 02:02

4

You are not initializing myObserver in your code so it's initial value is not knowable. You need to explicitly initialize it:

Observable::Observable() : myObserver(nullptr)
{
}

answered Jun 06 '13 at 02:02

Shafik Yaghmour

154,301
39
440
740

score 1 · Answer 3 · answered Jun 06 '13 at 02:01

That's right. You never initialised it, so it's value is undefined. That means you don't know what value it might contain because no value was ever set. It certainly isn't guaranteed to be NULL.

You should always initialise your pointers. The usual way is to do this using the initialiser-list syntax in your constructor:

Observable::Observable()
    : myObserver(NULL)
{ }

score 1 · Answer 4 · answered Jun 06 '13 at 02:01

1

C++ member variables are not initialized by default. That means your myObserver variable cannot be relied upon to be 0 in your example.

You need to add an initialization in your constructor to get out of the land of undefined behaviour.

answered Jun 06 '13 at 02:01

Carl Norum

219,201
40
422
469

score 0 · Answer 5 · answered Jun 06 '13 at 02:02

0

You need

Observable::Observable() : myObserver(0)
{
}

!mypointer will evaluate to true only if mypointer equals 0 (NULL, nullptr, etc.) but before you explicitly set this, it's simply undefined--some random value.

answered Jun 06 '13 at 02:02

Matt Phillips

9,465
8
44
75

"if(pointer) delete ponter" trys to delete even when pointer is not initialized

5 Answers5