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

[Mauller]

• Fixes: Cleanup of scripted players on mod maps can cause a read access violation in Team::KillTeam() #987

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.

[xezon]

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.

[Mauller]

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]

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]

Can we not check for null player template?

[Caball009]

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]

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.

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]

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

[Mauller]

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]

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]

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]

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.

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]

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]

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]

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]

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

[Mauller]

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]

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

[Mauller]

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]

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]

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.