Skip to content

Conversation

@xezon
Copy link

@xezon xezon commented Dec 6, 2025

This change fixes undefined behavior in TunnelTracker::onTunnelDestroyed and incomplete asset transfer in Team::setControllingPlayer.

This issue happens in GLA 03 mission, where at least 2 tunnels are transfered by script and Team::setControllingPlayer to the local player but are not unregistered from the tunnel tracker of the previous owner. This then causes undefined behavior in TunnelTracker::onTunnelDestroyed on mission end when all objects are destroyed and unregistered from the tunnel trackers.

 	generalszh.exe!std::list<enum ObjectID,std::allocator<enum ObjectID>>::front() Line 1222	C++
>	generalszh.exe!TunnelTracker::onTunnelDestroyed(const Object * deadTunnel) Line 236	C++
 	generalszh.exe!TunnelContain::onDelete() Line 429	C++
 	generalszh.exe!Object::onDestroy() Line 773	C++
 	generalszh.exe!GameLogic::destroyObject(Object * obj) Line 4067	C++
 	generalszh.exe!GameLogic::destroyAllObjectsImmediate() Line 329	C++
 	generalszh.exe!GameLogic::reset() Line 477	C++
 	generalszh.exe!GameEngine::resetSubsystems() Line 811	C++
 	generalszh.exe!GameEngine::reset() Line 790	C++
 	generalszh.exe!Win32GameEngine::reset() Line 77	C++
 	generalszh.exe!GameLogic::clearGameData(bool showScoreScreen) Line 277	C++
 	generalszh.exe!GameLogic::logicMessageDispatcher(GameMessage * msg, void * userData) Line 472	C++
 	generalszh.exe!GameLogic::processCommandList(CommandList * list) Line 2624	C++
 	generalszh.exe!GameLogic::update() Line 3763	C++
 	generalszh.exe!SubsystemInterface::UPDATE() Line 73	C++
 	generalszh.exe!GameEngine::update() Line 923	C++
 	generalszh.exe!Win32GameEngine::update() Line 90	C++
 	generalszh.exe!GameEngine::execute() Line 986	C++
 	generalszh.exe!GameMain() Line 55	C++
 	generalszh.exe!WinMain(HINSTANCE__ * hInstance, HINSTANCE__ * hPrevInstance, char * lpCmdLine, int nCmdShow) Line 897	C++
 	generalszh.exe!invoke_main() Line 107	C++
 	generalszh.exe!__scrt_common_main_seh() Line 288	C++
 	generalszh.exe!__scrt_common_main() Line 331	C++
 	generalszh.exe!WinMainCRTStartup(void * __formal) Line 17	C++
 	kernel32.dll!759d5d49()	Unknown
 	[Frames below may be incorrect and/or missing, no symbols loaded for kernel32.dll]	
 	ntdll.dll!7721d6db()	Unknown
 	ntdll.dll!7721d661()	Unknown
void TunnelTracker::onTunnelDestroyed( const Object *deadTunnel )
{
	m_tunnelCount--;
////////
	m_tunnelIDs.remove( deadTunnel->getID() );
	// ^^ REVIEW NOTE ^^ This will remove nothing if the tunnel was never registered, but `m_tunnelCount` is still decremented!
////////
	m_needsFullHealTimeUpdate = true;

	if( m_tunnelCount == 0 )
	{
		// Kill everyone in our contain list.  Cave in!
		iterateContained( destroyObject, NULL, FALSE );
		m_containList.clear();
		m_containListSize = 0;
	}
	else
	{
////////
		Object *validTunnel = TheGameLogic->findObjectByID( m_tunnelIDs.front() );
		// ^^ REVIEW NOTE ^^ When m_tunnelCount has wrapped from 0 to uint max,
		// then this can attempt to dereference front while the list is already empty
////////
		// Otherwise, make sure nobody inside remembers the dead tunnel as the one they entered
		// (scripts need to use so there must be something valid here)
		for(ContainedItemsList::iterator it = m_containList.begin(); it != m_containList.end(); )
		{
			Object* obj = *it;
			++it;
			if( obj->getContainedBy() == deadTunnel )
				obj->onContainedBy( validTunnel );
		}
	}
}

…omplete asset transfer in Team::setControllingPlayer
@xezon xezon added Minor Severity: Minor < Major < Critical < Blocker Fix Is fixing something, but is not user facing Script Is related to Script Engine, SCB labels Dec 6, 2025
}
else
{
obj->onCapture(oldOwner, newController);
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It worked ok in GLA 03 mission and avoids the tunnel transfer issue, but I do not know if it creates new problems. Something to keep an eye on.

Copy link

@Mauller Mauller Dec 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

handlePartitionCellMaintenance gets called within object::onCapture so shouldn't need calling here.
it also gets called within tunnelContain::onCapture as well which will get triggered by object::onCapture as it loops through all behaviour modules and calls their onCapture function.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They are exclusive calls depending on RETAIL_COMPATIBLE_CRC

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we test if the object is a tunnel and perform TunnelContain::onCapture() only on the tunnels? since they are a special case in this instance due to the tunnel system needing to be updated properly.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason I used RETAIL_COMPATIBLE_CRC is to avoid any chance of mismatch because it looks risky for any object.

The crash is avoided anyway with the fix in TunnelTracker::onTunnelDestroyed.

@Caball009
Copy link

Caball009 commented Dec 6, 2025

Keeping track of the container size in a separate variable is quite unfortunate, and it's done in quite a bunch of places.

@xezon
Copy link
Author

xezon commented Dec 6, 2025

Keeping track of the container size in a separate variable is quite unfortunate, and it's done in quite a bunch of places.

They do this because STLPort std::list does not store size but needs to compute it by iterating all nodes.

// The Team doesn't change, it just starts to return a different answer when you ask for
// the controlling player. I don't want to make the major change of onCapture on everyone,
// so I will do the minor fix for the specific bug, which is harmless even when misused.
// TheSuperHackers @fix xezon 07/12/2025 Now does onCapture on everyone.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be worth cleaning up the legacy comment as it no longer applies anymore

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did not want to nuke the EA comment so I just appended information. If you have a suggestion for how to rewrite the EA comment, please write.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Fix Is fixing something, but is not user facing Minor Severity: Minor < Major < Critical < Blocker Script Is related to Script Engine, SCB

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants