Skip to content

[GEN][ZH] Fix crash within Team::killTeam() caused by an invalid player retrieval in ScriptEngine::getPlayerFromAsciiString() #1221

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 1 commit into
base: main
Choose a base branch
from

Conversation

Mauller
Copy link

@Mauller Mauller commented Jul 1, 2025

This PR fixes a crash caused within some mod maps. The crash occurs within Team::killTeam() when the command is given to kill a specific player.

The issue here is that the script attempts to kill a player with a blank name. As uninitialised players in the player list all have blank names, this causes the ScriptEngine::getPlayerFromAsciiString() function to retrieve the first invalid player that it finds.

The crash occurs due to the uninitialised player not having a player template associated with it. This then causes a crash when the KillTeam() function attempts to retrieve it, so it can destroy any placed beacons by that player.

So the workaround for this was to simply test that when attempting to find the player by the player name, we test that we don't have an empty string, otherwise we implicitly return a NULL pointer.

@Mauller Mauller self-assigned this Jul 1, 2025
@Mauller Mauller added Major Severity: Minor < Major < Critical < Blocker Gen Relates to Generals ZH Relates to Zero Hour Crash This is a crash, very bad Mod Relates to Mods or modding labels Jul 1, 2025
@xezon
Copy link

xezon commented Jul 1, 2025

Would it perhaps also make sense to return Player::killPlayer early if it is not actually an active player that can be killed? That would make the function also more robust.

…er retrieval in ScriptEngine::getPlayerFromAsciiString()
@Mauller Mauller force-pushed the fix-invalid-player-crash branch from 6052950 to 8271a38 Compare July 1, 2025 20:04
@Mauller
Copy link
Author

Mauller commented Jul 1, 2025

Would it perhaps also make sense to return Player::killPlayer early if it is not actually an active player that can be killed? That would make the function also more robust.

I can have a look at that as well as a followup, i will need to check back over what the invalid players look like to determine what would be best to check against, but the m_playerNameKey seems to be the easiest as this tends to be blank for unused players.

it is also what the getPlayerFromAsciiString function internally uses to search for the player.

@Mauller
Copy link
Author

Mauller commented Jul 2, 2025

So players have a m_isplayerdead variable which is initialised to false, but as with all things it is part of the transfer and no doubt even uninitialised players get transfered.

So the only real thing that could also be checked in the Player::killPlayer function is the namekey.

@xezon
Copy link

xezon commented Jul 2, 2025

Can we not check for null player template?

image

@Caball009
Copy link

Can we not check for null player template?

This reminds me of #1065 and similar issues. I wonder whether they share a common root cause.

@Mauller
Copy link
Author

Mauller commented Jul 3, 2025

Okay so looking into this a bit more it appears that my fix actually breaks the AOD map from working as it originally did. But i think the AOD map was also using broken functionality to begin with by making use of the player ??? symbol which is where i believe the blank playername is coming from in the script actions.

So the original script was transfering the players units / buildings to the undefined player, then destroying all units on that player.
It was then giving the player a unit off map.

i don't quite know what the intention was meant to be with this but it's essentially relying on broken and undefined behaviour.

image

I still think the best thing overall is to properly fix the reason behind the crashing and not just patch up the crash point and allow people to rely on undefined behaviour.

@xezon
Copy link

xezon commented Jul 3, 2025

If there is something that changes behaviour and is not crashing, then I suggest put that behind !RETAIL_COMPATIBLE_CRC

@Mauller
Copy link
Author

Mauller commented Jul 3, 2025

If there is something that changes behaviour and is not crashing, then I suggest put that behind !RETAIL_COMPATIBLE_CRC

More context, in the vanilla game this map crashes. But if you are running gentool, it doesn't crash.

Thats what I mean by that map using undefined behaviour to do something. Since that behaviour should not normally work anyway.

It seems that one of the code injections that gentool does to prevent some issue prevents this crash while allowing the behaviour that let's this map work.

@xezon
Copy link

xezon commented Jul 4, 2025

Another thought here: Should findPlayerWithNameKey return a player when the name key refers to an empty name in the first place? How many players are there with empty names? And is any of them valid?

@aliendroid1
Copy link

Another thought here: Should findPlayerWithNameKey return a player when the name key refers to an empty name in the first place? How many players are there with empty names? And is any of them valid?

Does the observer get assigned an empty name?

@aliendroid1
Copy link

Okay so looking into this a bit more it appears that my fix actually breaks the AOD map from working as it originally did. But i think the AOD map was also using broken functionality to begin with by making use of the player ??? symbol which is where i believe the blank playername is coming from in the script actions.

So the original script was transfering the players units / buildings to the undefined player, then destroying all units on that player. It was then giving the player a unit off map.

i don't quite know what the intention was meant to be with this but it's essentially relying on broken and undefined behaviour.

image

I still think the best thing overall is to properly fix the reason behind the crashing and not just patch up the crash point and allow people to rely on undefined behaviour.

The ??? could be a result of missing scripts or scripts not loaded properly or mismatches with ini files. You might be able to get more info by checking the dump file.

@Mauller
Copy link
Author

Mauller commented Jul 4, 2025

Another thought here: Should findPlayerWithNameKey return a player when the name key refers to an empty name in the first place? How many players are there with empty names? And is any of them valid?

I don't think it should return anything when the player name is blank. And it appears that all uninitialised players have the same blank player name, namekey and overall signature.

So i would say that my fix is the correct one. And we should not make a hacky fix to allow a mod map to work that was using undefined behaviour. If anything we could later on add the required behaviour if needed.

@Mauller
Copy link
Author

Mauller commented Jul 4, 2025

Does the observer get assigned an empty name?

No, the observer gets a name with "observer" as part of the name, all valid players have a name and a fully initialised player object. That includes AI's and observers.

The ??? could be a result of missing scripts or scripts not loaded properly or mismatches with ini files. You might be able to get more info by checking the dump file.

The ??? is something that was manually input into the script in world builder, it just happens to end up translating to a blank.

@xezon
Copy link

xezon commented Jul 4, 2025

How about we make findPlayerWithNameKey return NULL when the Empty String Name Key was passed? Then callers no longer need to checK for empty string before calling this function.

@aliendroid1
Copy link

I wonder if this is related
In team.cpp this leads to really recursive calls
Player *Team::getControllingPlayer() const
{
return m_proto->getControllingPlayer();
}

Screenshot 2025-07-04 024500

@Mauller
Copy link
Author

Mauller commented Jul 4, 2025

How about we make findPlayerWithNameKey return NULL when the Empty String Name Key was passed? Then callers no longer need to checK for empty string before calling this function.

I think under normal circumstances it's not an issue, it's only really within the ScriptEngine that it seems to be a problem in scripted maps.
The script engine only uses ScriptEngine::getPlayerFromAsciiString to retrieve players, so the original fix should be fine for that.

@xezon
Copy link

xezon commented Jul 6, 2025

Ok that is fair then. Can you add an assert in findPlayerWithNameKey then that input name key is no empty name key?

@Mauller
Copy link
Author

Mauller commented Jul 6, 2025

Ok that is fair then. Can you add an assert in findPlayerWithNameKey then that input name key is no empty name key?

So i checked this out a bit more, added an assert here and it seems like some config on startup can end up passing in a blank player name, so it seems like this can be normal behaviour during some initialisation.

@xezon
Copy link

xezon commented Jul 6, 2025

Hmm. And is that the correct behaviour then, or also bugged? I am asking because it appears strange that someone would like to lookup a player that has no name. What is this supposed to achieve? Maybe there are more bugs and not just with Team::killTeam().

@Mauller
Copy link
Author

Mauller commented Jul 7, 2025

Hmm. And is that the correct behaviour then, or also bugged? I am asking because it appears strange that someone would like to lookup a player that has no name. What is this supposed to achieve? Maybe there are more bugs and not just with Team::killTeam().

I will check it again later. I think some fields for unused players are still being initialised, things related to teams are at least.
It could be that there is a shortcoming in the player initialisation code and it is just looping through all player slots when it shouldn't be.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Crash This is a crash, very bad Gen Relates to Generals Major Severity: Minor < Major < Critical < Blocker Mod Relates to Mods or modding ZH Relates to Zero Hour
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cleanup of scripted players on mod maps can cause a read access violation in Team::KillTeam()
4 participants