Skip to content

[GEN][ZH] Fix crash upon beacon removal in GameLogic::logicMessageDispatcher #1065

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

Caball009
Copy link

@Caball009 Caball009 commented Jun 14, 2025

case GameMessage::MSG_REMOVE_BEACON:
{
AIGroup *allSelectedObjects = NULL;
allSelectedObjects = TheAI->createGroup();
thisPlayer->getCurrentSelectionAsAIGroup(allSelectedObjects); // need to act on all objects, so we can hide teammates' beacons.
if( allSelectedObjects )
{
const VecObjectID& selectedObjects = allSelectedObjects->getAllIDs();
for (VecObjectID::const_iterator it = selectedObjects.begin(); it != selectedObjects.end(); ++it)
{
Object *beacon = findObjectByID(*it);
if (beacon)
{
const ThingTemplate *thing = TheThingFactory->findTemplate( beacon->getControllingPlayer()->getPlayerTemplate()->getBeaconTemplate() );
if (thing->isEquivalentTo(beacon->getTemplate()))

There's no sanity check for beacon->getControllingPlayer()->getPlayerTemplate() which causes a crash if the template is a nullptr. I wasn't able to find out much about why this happens, though I did notice that the controlling player was marked as PLAYER_COMPUTER.

The game no longer crashes with this PR.

@Caball009 Caball009 changed the title beacon removal crash fix [GEN][ZH] Fix crash upon beacon removal in GameLogic::logicMessageDispatcher Jun 14, 2025
@Caball009 Caball009 marked this pull request as ready for review June 14, 2025 20:36
@xezon
Copy link

xezon commented Jun 14, 2025

Looks similar to #972

So only getPlayerTemplate() needs null test, yes?

@Caball009
Copy link
Author

Looks similar to #972

So only getPlayerTemplate() needs null test, yes?

Yes, that would be enough to fix this particular crash.

I won't pretend to have much insight which pointers can or cannot be nullptrs in this code base. Here's a place where they're also checked:

if (obj->getControllingPlayer()
&& obj->getControllingPlayer()->getPlayerTemplate()
&& obj->getControllingPlayer()->getPlayerTemplate()->getBeaconTemplate().compare(obj->getTemplate()->getName()) == 0
)
{

@Caball009 Caball009 linked an issue Jun 16, 2025 that may be closed by this pull request
4 tasks
@Caball009 Caball009 added Bug Something is not working right, typically is user facing Minor Severity: Minor < Major < Critical < Blocker Gen Relates to Generals ZH Relates to Zero Hour Crash This is a crash Major Severity: Minor < Major < Critical < Blocker and removed Minor Severity: Minor < Major < Critical < Blocker labels Jun 16, 2025
@Mauller
Copy link

Mauller commented Jun 16, 2025

This issue is likely similar to the AiCommand issue i had with Hackers.

In this instance the original player may have been deleted before the command is acted upon.

@Caball009
Copy link
Author

This looks very similar to #987.

@xezon
Copy link

xezon commented Jun 18, 2025

Can you try figure out the reason for the null template? Maybe there is something that needs to be fixed higher up.

Other than that, it would be good to only change what is necessary to fix this crash. If another pointer would cause crashing with a different repro, then that warrants another investigation independent of this one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something is not working right, typically is user facing Crash This is a crash Gen Relates to Generals Major Severity: Minor < Major < Critical < Blocker ZH Relates to Zero Hour
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Access violation in AsciiString constructor when placing a beacon
3 participants