How to prevent Dangling Pointer in C++ Legacy Code

Question

1. Is it always wise to use NULL after a delete in legacy code without any smartpointers to prevent dangling pointers? (bad design architecture of the legacy code excluded)

       int* var = new int(100);            
       delete var;
       var = NULL;
   

2. Does it also make sense in destructors?

3. In a getter, does it make sense to test for NULL in second step? Or is it undefinied behavier anyway?

      Foo* getPointer() {  
             if (m_var!=NULL) { // <-is this wise
                 return m_var;
             }  
               else {
               return nullptr;
            }
        }

4. What about this formalism as an alternative? In which cases will it crash?

    Foo* getPointer() {  
             if (m_var) { // <-
                 return m_var;
             }  
               else {
               return nullptr;
            }
        }

5. (Edit) Will the code crash in example 3./4. if A. NULL is used after delete or B. NULL is not used after delete.

Answer 1

1. Is it always wise to use NULL after a delete in legacy code without any smartpointers to prevent dangling pointers? (bad design architecture of the legacy code excluded)

    int* var = new int(100);
    // ...
    delete var;
    var = NULL;

Only useful if you test var afterward. if scope ends, or if you set other value, setting to null is unneeded.

2. Does it also make sense in destructors?

nullify members in destructor is useless as you cannot access them without UB afterward anyway. (but that might help with debugger)

3. In a getter, does it make sense to test for NULL in second step? Or is it undefinied behavier anyway? [..]
4. [..]

if (m_var != NULL) and if (m_var) are equivalent.

It is unneeded, as, if pointer is nullptr, you return nullptr, if pointer is not nullptr, you return that pointer, so your getter can simply be

return m_var;

Answer 2

Avoid writing code like this

int* var = new int(100);            
// ... do work ...
delete var;

This is prone to memory leaks if "do work" throws, returns or otherwise breaks out of current scope (it may not be the case right now but later when "do work" needs to be extended/changed). Always wrap heap-allocated objects in RAII such that the destructor always runs on scope exit, freeing the memory.

If you do have code like this, then setting var to NULL or even better a bad value like -1 in a Debug build can be helpful in catching use-after-free and double-delete errors.

In case of a destructor:

Setting the pointer to NULL in a destructor is not needed.

• In production code it's a waste of CPU time (writing a value that will never be read again).

• In debug code it makes catching double-deletes harder. Some compilers fill deleted objects with a marker like 0xDDDDDDDD such that a second delete or any other dereference of the pointer will cause a memory access exception. If the pointer is set to NULL, delete will silently ignore it, hiding the error.

Answer 3

This question is really opinion-based, so I'll offer some opinions ... but also a justification for those opinions, which will hopefully be more useful for learning than the opinions themselves.

1. Is it always wise to use NULL after a delete in legacy code without any smartpointers to prevent dangling pointers? (bad design architecture of the legacy code excluded)

Short answer: no.

It is generally recommended to avoid raw pointers whenever possible. Regardless of which C++ standard your code claims compliance with.

Even if you somehow find yourself needing to use a raw pointer, it is safer to ensure the pointer ceases to exist when no longer needed, rather than setting it to NULL. That can be achieved with scope (e.g. the pointer is local to a scope, and that scope ends immediately after delete pointer - which absolutely prevents subsequent use of the pointer at all). If a pointer cannot be used when no longer needed, it cannot be accidentally used - and does not need to be set to NULL. This also works for a pointer that is a member of a class, since the pointer ceases to exist when the containing object does i.e. after the destructor completes.

The idiom of "set a pointer to NULL when no longer needed, and check for NULL before using it" doesn't prevent stupid mistakes. As a rough rule, any idiom that requires a programmer to remember to do something - such as setting a pointer to NULL, or comparing a pointer to NULL - is vulnerable to programmer mistakes (forgetting to do what they are required to do).

2. Does it also make sense in destructors?

Generally speaking, no. Once the destructor completes, the pointer (assuming it is a member of the class) will cease to exist as well. Setting it to NULL immediately before it ceases to exist achieves nothing.

If you have a class with a destructor that, for some reason, shares the pointer with other objects (i.e. the value of the pointer remains valid, and presumably the object it points at, still exist after the destructor completes) then the answer may be different. But that is an exceedingly rare use case - and one which is usually probably better avoided, since it becomes more difficult to manage lifetime of the pointer or the object it points at - and therefore easier to introduce obscure bugs. Setting a pointer to NULL when done is generally not a solution to such bugs.

3. In a getter, does it make sense to test for NULL in second step? Or is it undefinied behavier anyway?

Obviously that depends on how the pointer was initialised. If the pointer is uninitialised, even comparing it with NULL gives undefined behaviour.

In general terms, I would not do it. There will presumably be some code that initialised the pointer. If that code cannot appropriately initialise a pointer, then that code should deal with the problem in a way that prevents your function being called. Examples may include throwing an exception, terminating program execution. That allows your function to safely ASSUME the pointer points at a valid object.

4. What about this formalism as an alternative? In which cases will it crash?

The "formalism" is identical to the previous one - practically the difference is stylistic. In both cases, if m_var is uninitialised, accessing its value gives undefined behaviour. Otherwise the behaviour of the function is well-defined.

A crash is not guaranteed in any circumstances. Undefined behaviour is not required to result in a crash.

If the caller exhibits undefined behaviour (e.g. if your function returns NULL the caller dereferences it anyway) there is nothing your function can do to prevent that.

Answer 4

The case you describe remains relatively simple, because the variable is described in a local scope.
But look for example at this scenario:

struct MyObject
{
public :
    MyObject (int i){ m_piVal = new int(i); };
    ~MyObject (){ 
        delete m_piVal;
    };

public:
    static int *m_piVal;
};
int* MyObject::m_piVal = NULL;

You may have a double free problem by writing this:

MyObject *pObj1 = new MyObject(1);
MyObject *pObj2 = new MyObject(2);
//...........
delete pObj1;
delete pObj2; // You will have double Free on static pointer (m_piVal)

Or here:

struct MyObject2
{
public :
    MyObject2 (int i){ m_piVal = new int(i); };
    ~MyObject2 (){ 
        delete m_piVal;
    };

public:
    int *m_piVal;
};  

when you write this:

MyObject2 Obj3 (3);
MyObject2 Obj4 = Obj3; 

At destruction, you will have double Free here because Obj3.m_piVal = Obj4.m_piVal

So there are some cases that need special attention (Implement : smart pointer, copy constructor, ...) to manage the pointer