Pointer Deletion Crash

Question

Edit:

I've implemented copy constructors (both from the suggested answer). I have done this in my controller class, as well as equivalent versions in it's sub classes. However, this has not resolved the issue. Also, a counter and console comment in the removePawn() function (the only place in the program that delete is called) shows that it is only called once.

In more detail, there is one instance of each subclass (not pointers). These are declared in my world class. Both are used as parameters in the same world classes methods via the baseController class pointers. The problem is, while both are going through the same processes in the same order, if one class has removePawn() called on it, the program is fine and will run continually. However if the second class has removePawn() (delete specifically) called on it, it crashes the program at that instruction.

I've also checked the addresses. The address of the pointer just after allocation is identical to the address at the point of deletion.

More info: I will get a segmentation error when closing the program ONLY if the player is killed (deleted then given a new pawn). However, if the program is started then closed without any more than the first new and last delete, then it runs perfectly fine.

Original:

I'm having a bit of trouble with pointers. I understand them and believe my code is fairly robust however I seem to get a complete crash while calling this section of code.

Pawn is a basePawn* initialized to NULL.

if (pawn != NULL)
{
    cout << "Calling delete.\n";
    delete pawn;
    pawn = NULL;
}

This is a university assignment for a PS2 program so my debugging is limited to basic printing to the console.

Removing the delete line allows the main new/delete section to run a few times however it eventually crashes too (I assume this is because the memory limit is reached however I can't be sure)

I have checked all the usual culprits, the pointer is initialized to null and only deleted once (new is always called also).

I may be making a fairly obvious mistake however I have no clue. Any suggestions would be great. (I can post up more code if needed).

Edit:

Here's how the structure of the code works.

basePawn is a class with some fairly basic methods representing the character.

Controller is a class with a pointer to a basePawn (initially set to NULL) used as the brain of the character (AI or player controlled). It contains a removePawn method.

void controller::removePawn()
{
    if (pawn != NULL)
    {
        cout << "Calling delete.\n";
        delete pawn;
        pawn = NULL;
    }
}

This method gets called in the destructor. It also gets called when the pawn is removed from the level.

It also has a respawn method.

if (pawn == NULL)
{
    respawnCounter++;

    if (respawnCounter >= respawnTime)
    {
        //Switch block to change class
        pawn = new basePawn;

        if (pawn !=NULL)
        {
            pawn->boardY = 4; //Will be random
            pawn->boardX = 5; //Will be random

            respawnCounter = 0;

            pawn->setIdle();

            return true;
        }
    }
}

Edit:

baseController header file

#ifndef _BASEPAWNCONTROLLER_H
#define _BASEPAWNCONTROLLER_H

#include "basePawn.h"
#include "textureManager.h"
#include "direction.h"
#include "vector2f.h"

//Used to control pawns
//Allows the same commands to be used to control different pawns
class basePawnController
{
private:
protected:
basePawn *pawn;

int respawnCounter,
    respawnTime;

vector2f    targetDest;

bool        bMoving,
            bTarget;

void        removePawn();

public:

bool    bFirstFrameDead;

basePawnController();
virtual ~basePawnController();

virtual void update();

basePawn *getPawn();
void setPawn(basePawn *p);  
void setTarget(float x, float y);
direction getDir();
bool isMoving();
bool hasTarget();

virtual bool respawn();
virtual void render(textureManager &tManager);

virtual bool wantsPawn();
virtual void giveTargetInfo(direction d, int n);
};

#endif

Answer 1

Given that it is the PS2, I will assume you have no access to C++11 and with it -- smart pointers (std::unique_ptr and the sort) etc. If Boost or C++11 is not an option, you should really take the time and study how to reimplement basic smart pointer functionality -- this would relax the mental strain considerably. But, let's assume that's not an option.

Do note this is the most likely scenario what's going on, due to a scarce description of the problem, gathered from the question and the comments.

You're trying to utilize RAII by deallocating resources like the pawn when the controller instance goes out of scope, since you call removePawn in the destructor -- that's good. However, there are many ways an instance of a class can lose data / blow up when you don't provide non-trivial copy assignment operator and copy constructor implementations when coupled with pointers and heap allocations.

You should really implement additional functionality which manages copying which can happen in ways you don't expect. If you don't manage it yourself, the code perhaps runs a few times and lets the controller get copied somehow by passing it to a function or the like -- it will construct a new controller with the old one's data. But it's a shallow copy, the address of the basePawn which we assume is valid will be copied, but it will not set the dying instance's one to 0 / NULL.

After the old one runs out of scope, its destructor gets called. And since you didn't invalidate the old pointer, it will delete the same pawn that is now referred by two different objects. And when you state delete pawn; some time later -- boom.

You need these two properly implemented:

basePawnController(basePawnController& that);
basePawnController& operator=(basePawnController& that);

Don't get into the trap of declaring it as const, that is not an option when you're playing around with raw pointers. What needs to happen is along the lines of this, you can implement one in terms of the other:

// Copy constructor
basePawnController::basePawnController(basePawnController& rhs) {

    *this = rhs; // invokes basePawnController::operator=()

}

// Copy assignment operator
basePawnController& basePawnController::operator=(basePawnController& rhs) {

    // copy your stuff and now the basePawn
    pawn = rhs.pawn; // Copy the address...
    rhs.pawn = 0; // ...but make sure the old instance doesn't troll you.

    return *this;

}

My best advice to you would be to investigate ownership transparency through smart pointers by utilizing the RAII facilities. Programming should be a pleasure.

Answer 2

Well, you guys were pretty much right.

After adding the copy constructor and assignment operator, I performed more checks to see where my problems were actually occurring. Turns out it wasn't anything to do with my controller classes directly, but my pawn and world classes.

My pawn has a vector of damageNode* which I wasn't deleting. Not a big issue as I had a pointer to them in the world. But I never deleted them in the world class either, hence I was getting seg faults when removing the pawn and the world.