Tracking down a Memory Access Exception

[S3rgeus]

I've created a new game system as a part of a total conversion mod by modding the DLL. Now, unfortunately, in the process of doing that, I have introduced one of the worst kinds of bugs to the game. I now have an intermittent CTD caused by a memory access exception when my new system is activated. It doesn't happen every time (maybe 1/4 times) but it's definitely a problem.

Now, I think this is due to my lack of understanding of the defines Firaxis is using in the place of new and delete, and I'm probably not using them properly.

I've introduced the following code files:

Spoiler :

WoTHornOfValere.h

// ----------------------------------------------------------------
// WoTMod New File
// Created by: S3rgeus
// ----------------------------------------------------------------

#pragma once

#include "CvPlot.h"
#include "CvUnit.h"

// Horn of Valere
class HornOfValere
{
public:
	HornOfValere();
	HornOfValere(int iXPos, int iYPos);
	HornOfValere(CvPlot* pkPlot);
	~HornOfValere();

	CvPlot* GetPlot() const;
	int GetX() const;
	int GetY() const;
	int GetTurnsSinceHornBlown() const;

	void SetPlot(CvPlot* pkPlot);
	void SetX(int iNewXPos);
	void SetY(int iNewYPos);
	void SetPosition(int iNewXPos, int iNewYPos);
	void SetTurnsSinceHornBlown(int iNewValue);

	void IncrementTurnsSinceHornBlown();

	void DoTurn();

	void FindHorn(CvUnit* pUnit);
	void MoveHorn(int iNewXPos, int iNewYPos);
	void MoveHorn(CvPlot* pkNewPlot);

	bool CanFindHornOfValere(CvUnit* pUnit);

protected:
	CvPlot* m_pkPlot;
	int m_iXPosition;
	int m_iYPosition;
	bool m_bFound;
	int m_iTurnsSinceHornBlown;
};

WoTHornOfValere.cpp

#include "WoTHornOfValere.h"
#include "CvGlobals.h"
#include "CvGameCoreDLLPCH.h"
#include "CvGameCoreUtils.h"

HornOfValere::HornOfValere(): m_bFound(false)
{

}

HornOfValere::HornOfValere(int iXPos, int iYPos)
{
	HornOfValere(GC.getMap().plot(iXPos, iYPos));
}

HornOfValere::HornOfValere(CvPlot* pkPlot)
	: m_pkPlot(pkPlot), m_iXPosition(pkPlot->getX()), 
		m_iYPosition(pkPlot->getY()), m_bFound(false),
		m_iTurnsSinceHornBlown(50000) // arbitrarily large but not enough to overflow
{

}

HornOfValere::~HornOfValere()
{
	m_pkPlot = NULL;

	m_iYPosition = 0;
	m_iXPosition = 0;
	m_iTurnsSinceHornBlown = 0;
}

CvPlot* HornOfValere::GetPlot() const
{
	return m_pkPlot;
}

int HornOfValere::GetX() const
{
	return m_iXPosition;
}

int HornOfValere::GetY() const
{
	return m_iYPosition;
}

int HornOfValere::GetTurnsSinceHornBlown() const
{
	return m_iTurnsSinceHornBlown;
}

void HornOfValere::SetPlot(CvPlot* pkPlot)
{
	m_pkPlot = pkPlot;
}

void HornOfValere::SetX(int iNewXPos)
{
	m_iXPosition = iNewXPos;
}

void HornOfValere::SetY(int iNewYPos)
{
	m_iYPosition = iNewYPos;
}

void HornOfValere::SetPosition(int iNewXPos, int iNewYPos)
{
	SetX(iNewXPos);
	SetY(iNewYPos);
}

void HornOfValere::SetTurnsSinceHornBlown(int iNewValue)
{
	m_iTurnsSinceHornBlown = iNewValue;
}

void HornOfValere::IncrementTurnsSinceHornBlown() 
{
	m_iTurnsSinceHornBlown++;
}

void HornOfValere::DoTurn() 
{
	IDInfoVector currentUnits; 
	if (!m_bFound && m_pkPlot->getUnits(&currentUnits) > 0)
	{
		for (IDInfoVector::const_iterator itr = currentUnits.begin(); itr != currentUnits.end(); ++itr)
		{
			CvUnit* pUnit = ::getUnit(*itr);

			if(pUnit && pUnit->CanDiscoverHornOfValere())
			{
				FindHorn(pUnit);
			}
		}
	}

	IncrementTurnsSinceHornBlown();
}

void HornOfValere::FindHorn(CvUnit* pUnit)
{
	// TODO UI popup
	if (pUnit)
	{
		m_bFound = true;
		m_pkPlot->SetHornOfValere(false);

		ICvEngineScriptSystem1* pkScriptSystem = gDLL->GetScriptSystem();

		if (pkScriptSystem)
		{
			CvLuaArgsHandle args;
			args->Push(pUnit->getOwner());
			args->Push(pUnit->GetID());

			bool bResult;
			LuaSupport::CallHook(pkScriptSystem, "UnitDiscoveredHornOfValere", args.get(), bResult);
		}
	}
}

void HornOfValere::MoveHorn(int iNewXPos, int iNewYPos)
{
	CvPlot* pkNewPlot = GC.getMap().plot(iNewXPos, iNewYPos);

	MoveHorn(pkNewPlot);
}

void HornOfValere::MoveHorn(CvPlot* pkNewPlot)
{
	m_pkPlot->SetHornOfValere(false);
	
	m_iXPosition = pkNewPlot->getX();
	m_iYPosition = pkNewPlot->getY();

	pkNewPlot->SetHornOfValere(true);

	m_pkPlot = pkNewPlot;
}

I've added an instance of that object as a member of CvMap:

Spoiler :

CvMap.h

	// ----------------------------------------------------------------
	// WoTMod Addition
	// ----------------------------------------------------------------
	bool m_bHasHornOfValere;
	HornOfValere* m_pHornOfValere;

The crash seems to occur when the Horn of Valere is first placed on the map, using this function:

Spoiler :

void CvMap::PlaceHornOfValere(int iX, int iY)
{
	if (IsHasHornOfValere())
	{
		m_pHornOfValere->MoveHorn(iX, iY);
	}
	else
	{
		CvPlot* pkPlot = plot(iX, iY);
		m_pHornOfValere = FNEW(HornOfValere(pkPlot), c_eCiv5GameplayDLL, 0);

		SetHasHornOfValere(true);
	}
}

I've used Firaxis' FNEW to create a new instance of my custom class there, but the third parameter to FNEW confuses me a bit. The second parameter, I believe, tells the game which pool of memory we're drawing from? So it makes sense to use the GamePlayDLL pool. But the third is just marked as 'Tag'? (All of the other calls to FNEW that I've seen in CvMap use a value of 0 here, so that's why I've done that.)

Any help would be much appreciated! I know this is an almost impossible error to debug remotely, but any insight into FNEW or SAFE_DELETE and their counterparts would be very helpful.

EDIT: Oops, I forgot to say, and this is probably quite important, by attaching the VS debugger to CivV (thanks for the suggestion, hulkster!) I found the exception was thrown here: (I've typed out the stack trace, long story why I can't just copy and paste it)

CvDllGameContext::Free(void*) Line 183
CvDllGameContext::operatordelete(void*) Line 79
CvDllGame::DecrementReference() Line 57
CvDllGame::Destroy() Line 75

 

[Hambil]

I take it you've already looked at the source changes (he provides source) in Whoward's DLL for syntax/usage? If so, I'm of no help

 

[Hambil]

I also seem to remember him saying that plopping something new on the map is a real pain and requires a specific sequences of events. He was talking about lua though, and that might not apply to this at all. Just in case it does though: thread

 

[whoward69]

Without digging into the code, my gut feel is that you've probably created a cyclic dependancy between Map, plot and horn by storing the horn plot into m_pkPlot. Map will have pointers to both horn and hornPlot, horn has a pointer to hornPlot, so you are dependant on the destruction/free order within Map as to whether the hornPlot is valid by the time you get to the code in horn.

Also, although you never use them afterwards, in your destructor you set x = y = 0 - which happens to be a valid plot, did you mean -1?

 

[S3rgeus]

I take it you've already looked at the source changes (he provides source) in Whoward's DLL for syntax/usage? If so, I'm of no help

I downloaded a copy of Whoward's DLL a while back for help with some syntax, it's been quite useful!

I also seem to remember him saying that plopping something new on the map is a real pain and requires a specific sequences of events. He was talking about lua though, and that might not apply to this at all. Just in case it does though: thread

I think I'll have to deal with those order issues eventually when I'm implementing other features, but I doubt it will have caused this issue.

Without digging into the code, my gut feel is that you've probably created a cyclic dependancy between Map, plot and horn by storing the horn plot into m_pkPlot. Map will have pointers to both horn and hornPlot, horn has a pointer to hornPlot, so you are dependant on the destruction/free order within Map as to whether the hornPlot is valid by the time you get to the code in horn.

I think I see what you mean. You mean the game might be in a partially decomposed state when the code in WoTHornOfValere.cpp is called. I'm not really sure I completely understand the allocation of the objects involved then though. The Horn is being placed at the end of the first turn, at which point I wouldn't have expected destructors for the game and map objects to be being called.

The HornOfValere destructor doesn't depend on any code in m_pkPlot though, it just assigns null to the pointer, which I wouldn't have thought would require the instance being pointed to before to be in any valid state.

I did realize that I was never deleting CvMap.m_pHornOfValere in the CvMap destructor though, which may have been causing issues.

Also, although you never use them afterwards, in your destructor you set x = y = 0 - which happens to be a valid plot, did you mean -1?

Very true, I didn't really think about that. I saw the other Cv class destructors setting their integer members to 0 and figured there must have been a memory management reason to do so. (Didn't really think about what that could be.)

I'll see if the deletion in the CvMap destructor fixed the issue and look at testing the validity of the m_pkPlot pointer every time before it's used if that doesn't solve it. Wouldn't calls to PlaceHornOfValere during destruction of the map cause null pointer exceptions rather than memory access exceptions though? I'm not sure, I've gotten too used to managed languages recently. It's an annoying bug to test for since I can't reliably reproduce it, but hopefully I'll know soon if it's fixed.

 

[whoward69]

Do you really need to store both the horn's (x, y) location *and* the CvPlot object for that location as you can easily get one from the other. Firaxis tend to store just (x, y) and create the plot object as needed. There may be a very good reason for this that we don't fully understand

 

[S3rgeus]

Do you really need to store both the horn's (x, y) location *and* the CvPlot object for that location as you can easily get one from the other. Firaxis tend to store just (x, y) and create the plot object as needed. There may be a very good reason for this that we don't fully understand

Rereading your first post yesterday, I had this same thought. I'll remove the pointer to the plot and retrieve it from the map when it's needed and see if that fixes the issue.

 

[S3rgeus]

Do you really need to store both the horn's (x, y) location *and* the CvPlot object for that location as you can easily get one from the other. Firaxis tend to store just (x, y) and create the plot object as needed. There may be a very good reason for this that we don't fully understand

It seems like removing the pointer has fixed the problem. Thanks! I can't be 100% sure because it's intermittent, but I've started several new games and gone through multiple save/loads and no crashes today. I think everything might be being shuffled around in memory on occasion, so stored pointers become invalid, but that's only a guess, since there were destructors being called mid-game during otherwise normal operation. It might be why everything is managed as a part of memory pools.