Pointer Deletion - Possible heap corruption

[Damien McAlear]

Hey there. First of, I'm not exactly sure if this should be in this forum or the Linux version. I'm doing a university assignment (due in a couple of weeks) developing a game with PS2 linux. With the PS2, I have no real debugger, just printing to the console and crashes.

I have a problem with pointers. I have a mainly virtual controller class, with two sub classes. One for AI and one for a Player.

Both the subclasses have similar (in some cases, identical) methods however, the AI one is giving me issues.

Both classes have a pawn variable (basePawn* pawn) initially set to NULL. In their respawn method, they call pawn = new basePawn. In their removePawn method, they call

if (pawn != NULL)
{
    delete pawn;
    pawn = NULL;
}

This works fine being repeatedly called on the Player method, however it crashes on the delete call within the AI class. I have followed the 'Rule of Three' to the best of my knowledge.

If I close the program as soon as it's running, both classes will call removePawn in their destructor and work fine. However, if I allow the AI to kill the player, and then close the program, I get a segmentation fault from the AI.

If the Player kills the AI, the program crashes on the delete call.

I can share more code if required, however I'm at a dead loss and would rather leave minimal information here so I don't overload anyone with code.

I'd like to thank anyone that takes the time to read this and all feedback is appreciated.

[Catfish4]

Hmm does the destructor do delete pawn; without checking if pawn == NULL?

I would advise that you check out std::unique_ptr if your compiler's library has it, or the deprecated std::auto_ptr for a similar effect otherwise. Try to avoid manually doing new and delete if you can.

http://cplusplus.com/reference/memory/unique_ptr/
http://cplusplus.com/reference/memory/auto_ptr/

This said, you should post more code. You did your best to explain things in detail, but it's useless in this case: to help fix the code we need to see it.

[Damien McAlear]

Oops, I forgot to mention, my destructors call the removePawn() method also. (I just did that to stop me forgetting null checks and setting to NULL after the delete).

Unfortunately I cannot use smart pointers or boost libraries, this is a rather old compiler that I have to use (it doesn't even support std::vector's .at() method).


I can post as much code as is necessary however I have no idea what I should post.

Here are the .h files from my controller class and sub classes, as well as the base pawn itself.


Edit: Removed code

[Damien McAlear]

Also, I'm using polymorphism in the general game loop, so both the AI and player controller are using the same functions at the highest level.

The only major difference would be the update functions themselves.

The player one checks pad input and acts accordingly. The AI version (currently) checks it's surroundings for an enemy. If it find's one in range, it attacks.

[KRAkatau]

Question: do you always instantiate basePawn? If not - if you delete it by the pointer to basePawn you will have some problems due to non-virtual destructor.

[KRAkatau]

Plus, in your place I would really write/use some shared_ptr implementation. The fact that it is not in the standard library for your compiler doesn't mean you can't make one yourself. It's a pretty simple template class so given the compiler is able to handle std::vector - it will handle your shared_ptr as well.

Edit: it wouldn't solve the problem with non-virtual destructor though - if it is indeed the problem ;)

[Damien McAlear]

basePawn is always initialised to NULL.

In the update loop there's something similar to this

if (pawn != NULL)
{
    game logic
}
else
{
     respawn code (which contains pawn = new basePawn and some checks to make sure it's not NULL)
} 

The only non-virtual destructors are the subclasses of Controller (shouldn't cause a problem as they don't have any further subclasses) and basePawn, which should have one, however I don't have any subclasses of that yet either (and I don't think it actually needs a destructor anyway, it's empty).


I suppose it would be fairly easy to write one, however I don't have the time to write one (I have multiple courseworks to hand in). I also feel that my pointer implementation is correct, as one class works perfectly fine and as far as the pointer is concerned, it's pretty much identical.

I'll post the implementations of the two controller classes in case I've done something immensely stupid.

[Damien McAlear]

Controller class implementations. Edited to cut out some extra functions.

Edit: Removed code.

[Damien McAlear]

I do hate to bump posts. However I'm still at a loss here.

I've checked the memory addresses and the pointer still points to the same address as the one at the time of allocation.

Although the instance of these classes are passed around (as baseController *c) removePawn (function to delete) is only ever called either when the character is killed (intended) and when the program is closed (also intended).

[MiiNiPaa]

Do your destructors virtual now?
BTW, you do not need to check for null pointer when you delete it: delete already required to do nothing to null pointers by standard.

[Damien McAlear]

The baseController destructor is virtual, and since there are no subclasses of the AIController and PlayerController their destructors shouldn't have to be virtual (to my knowledge?)

I know that now, I just never got around to changing the function. Haha.

[KRAkatau]

Yes, as any other function with the same signature they are automatically made virtual. People only write virtual keyword in derived classes as a kind of documentation (in c++11 there's an override keyword which is in fact much more useful as it will generate an error if the signature of function in base class is changed).

To the point: to be honest, it's somewhat hard to see the error. If you could provide your files - I could stick it in a MSVS project - even if it wont compile - analyzing larger code will still be way easier. I really don't like your operator= though. That "make sure the old instance doesn't troll you." comment is epic, but it doesn't make the function better at all. In fact it looks like a simple unique_ptr implemenation, but it's error prone - e.g. if you pass your AIController to some function by value - your pawn will be destroyed. Additionally there's a potential memory leak if you assign one AIController to another one (operator= should call removePawn() before assigning the rhs.pawn).

Anyway, if you post your full code related to the issue (e.g. rendering related stuff is not necessary) - I might take a look.

Also, you could really use a debugger in this case to see the state of the program upon the crash. Another option - log the addresses upon deletion - perhaps u delete the same address twice.

[Damien McAlear]

I was advised by a member of StackOverflow to write the operator= like that. (Including his comment haha).

I never pass the controller by value (at least, not to my knowledge). I'll post an example signature of a function I use them with.

void		setControllerTarget(basePawnController *c);

By the potential memory leak issue, do you mean to say I should do something to his effect.

pawn = rhs.pawn;
rhs.removePawn(); //Should delete rhs.pawn and set it to NULL in here
rhs.pawn = NULL //However, just to be safe 

Edit: If I call removePawn() here, wouldn't that delete both of them since pawn is just a pointer?

Second Edit: Ah wait, you mean something more like this, don't you?

this.removePawn(); //To delete our current pawn.
pawn = rhs.pawn;
rhs.pawn = NULL 

Unfortunately, the PS2 environment does not have any debugging facilities. It will just crash on an operation (and takes between 5-10 minutes to restart).

I have tried logging the addresses, once at allocation and again at deletion. The same address is used each time (obviously a different one for each instance of the class).

[Damien McAlear]

Also, how would I post a VS project here? Would a drop box link suffice?

[KRAkatau]

Second edit is exactly what i meant.

Question about logging was not if the address was the same - but if it is there only ONCE (or rather if number of allocations for same address equals number of deletions).

Hmmm, so PS2? Do you have a devkit? I remember using Metrowerks CodeWarrior for working with PS2, and devkits allowed debugging with breakpoints, watches and stuff :)

Aaaand ye, dropbox is fine.

[Damien McAlear]

I've implemented that second edit and I will test it tomorrow when I get back to the console.

Ah right, yes, I've logged when new is called, plus the address (some other info at that time too) and the same at delete. It seems to be perfectly 1 for 1.

My university provides PS2s with a version of Linux installed on them as well as a basic framework. Unfortunately I can't put any sort of dev kit over the top of this. We are to use the methods we have been taught really to debug our programs.

Basically, we write our code (generally using notepad++ but some people use Visual Studio) then copy it across and use the command line and a make file (controlling the console from the pc using PuTTY).


I'll stick the files together. (:

[Damien McAlear]

Here are the files that you should need access to.

I'd warn you that some of the style is rather messy as I've been rushing trying to fix this bug.

https://www.dropbox.com/s/sc1dn7e6yvgligm/PS2PointerDeletion.zip

[Damien McAlear]

I have been advised that heap corruption is a likely cause of this problem.

Would anyone be able or willing to give an example of exception safe copy constructors (the one I used was this = rhs) and assignment operators?

[Damien McAlear]

I've ran the following test on the program and it has successfully ran with no crashes or segmentation faults. This was run in place of my update function.

    testCounter++;
	
	if (testCounter == 500)
	{
	
		pController.removePawn();
		
		testAI.removePawn();
		
		pController.getNewPawn();
		
		controllerLoop(&pController);		
		
		testAI.getNewPawn();
		
		controllerLoop(&testAI);
		
		testCounter = 0;
	}

I used different variations of this, calling removePawn on both of them before giving them a new pawn. Calling remove then new one after the other etc. None of them caused any problems.

This leads me to believe that the problem is somewhere else as running the normal code still causes a segmentation fault when closing after the player has been killed and respawned, and also a crash if attempting to kill the AI (even with forced removePawn straight as the pawn is 'dead' rather than after the animation is finished and new forced the frame after rather than waiting for the respawn time).

[KRAkatau]

You marked this as solved because you solved the problem? :)

If not - this caught my eye in your code.

		damageNodes.push_back(node);
		
		delete node;
		node = NULL;

You save the pointer for future use (i assume) and then immediately delete the object you will probably use later. This is looks to be the source of your problems.