-
Notifications
You must be signed in to change notification settings - Fork 79
[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
base: main
Are you sure you want to change the base?
Conversation
Generals/Code/GameEngine/Source/GameLogic/ScriptEngine/ScriptEngine.cpp
Outdated
Show resolved
Hide resolved
Would it perhaps also make sense to return |
…er retrieval in ScriptEngine::getPlayerFromAsciiString()
6052950
to
8271a38
Compare
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 it is also what the |
So players have a So the only real thing that could also be checked in the |
This reminds me of #1065 and similar issues. I wonder whether they share a common root cause. |
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 So the original script was transfering the players units / buildings to the undefined player, then destroying all units on that player. 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. |
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. |
Another thought here: Should |
Does the observer get assigned an empty name? |
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. |
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. |
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 ??? is something that was manually input into the script in world builder, it just happens to end up translating to a blank. |
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. |
Ok that is fair then. Can you add an assert in |
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. |
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 |
I will check it again later. I think some fields for unused players are still being initialised, things related to teams are at least. |
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.