[GEN][ZH] Fix heap-use-after-free in W3DRadar::drawHeroIcon() #1035
Conversation
| // This avoids potentially catastrophic issues when objects and subsystems have cross dependencies. | ||
| TheGameLogic->reset(); | ||
|
|
||
| TheSubsystemList->resetAll(); |
There was a problem hiding this comment.
With this change, this resetAll crashed on map unload in
> generalszh.exe!TunnelContain::iterateContained(void(*)(Object *, void *) func, void * userData, bool reverse) Line 191 C++
generalszh.exe!Object::isHero() Line 2044 C++
generalszh.exe!W3DRadar::addObject(Object * obj) Line 1003 C++
generalszh.exe!Player::becomingLocalPlayer(bool yes) Line 1138 C++
[Inline Frame] generalszh.exe!PlayerList::setLocalPlayer(Player *) Line 324 C++
generalszh.exe!PlayerList::init() Line 246 C++
generalszh.exe!PlayerList::reset() Line 127 C++
generalszh.exe!SubsystemInterfaceList::resetAll() Line 188 C++
[Inline Frame] generalszh.exe!GameLogic::isInMultiplayerGame() Line 418 C++
generalszh.exe!GameEngine::reset() Line 703 C++
generalszh.exe!GameLogic::clearGameData(bool showScoreScreen) Line 281 C++
generalszh.exe!GameLogic::logicMessageDispatcher(GameMessage * msg, void * userData) Line 464 C++
generalszh.exe!GameLogic::processCommandList(CommandList * list) Line 2597 C++
generalszh.exe!GameLogic::update() Line 3767 C++
[Inline Frame] generalszh.exe!SubsystemInterface::UPDATE() Line 134 C++
generalszh.exe!GameEngine::update() Line 766 C++
generalszh.exe!Win32GameEngine::update() Line 93 C++
generalszh.exe!GameEngine::execute() Line 823 C++
generalszh.exe!GameMain(int argc, char * * argv) Line 47 C++
generalszh.exe!WinMain(HINSTANCE__ * hInstance, HINSTANCE__ * hPrevInstance, char * lpCmdLine, int nCmdShow) Line 988 C++
[Inline Frame] generalszh.exe!invoke_main() Line 102 C++
generalszh.exe!__scrt_common_main_seh() Line 288 C++
kernel32.dll!75e3fcc9() Unknown
[Frames below may be incorrect and/or missing, no symbols loaded for kernel32.dll]
ntdll.dll!76f582ae() Unknown
ntdll.dll!76f5827e() Unknown
Exception thrown: read access violation.
**owningPlayer** was 0x150.
The problem is that the Object's TunnelContain module has a dependency on the player, but the player was already purged from the PlayerList. And so this would crash.
Resetting GameLogic first avoids this (and potentially similar issues in the future). Alternatively we could add more NULL checks in TunnelContain, but I think that is not necessary if Subsystem just have a proper reset sequence.
| @@ -118,7 +121,7 @@ class W3DRadar : public Radar | |||
| Real m_viewZoom; ///< camera zoom used for the view box we have | |||
| ICoord2D m_viewBox[ 4 ]; ///< radar cell points for the 4 corners of view box | |||
|
|
|||
| std::list<const Coord3D *> m_cachedHeroPosList; //< cache of hero positions for drawing icons in radar overlay | |||
| std::vector<const Object *> m_cachedHeroObjectList; //< cache of hero objects for drawing icons in radar overlay | |||
There was a problem hiding this comment.
I changed the container type here because list of coordinate pointers is not particularly great.
There was a problem hiding this comment.
Wouldn't it be better to change to std::vector<Coord2D>? As far as I can tell, the z coordinate is irrelevant for the radar and the hero coordinates are updated each frame, so using a pointer is not an advantage when the size of 2D coordinates is 2 * sizeof(float).
Changes:
void W3DRadar::drawHeroIcon( Int pixelX, Int pixelY, Int width, Int height, const Coord3D *pos )
->
void W3DRadar::drawHeroIcon( Int pixelX, Int pixelY, Int width, Int height, Coord2D pos )
m_cachedHeroPosList.push_back(obj->getPosition());
->
m_cachedHeroPosList.push_back(Coord2D(obj->getPosition()->x, obj->getPosition()->y));
There was a problem hiding this comment.
The position changes when the Hero moves. Putting a fixed position into the vector would paint the radar icon at a static position.
There was a problem hiding this comment.
I don't think it would. I just tried it and the position is not fixed on the radar. It wouldn't be if the position is updated each frame; i.e. if W3DRadar::renderObjectList is called for each frame.
There was a problem hiding this comment.
I suspect you test with calcHero code? The calcHero rebuilds the vector every now and then. This change does not.
xezon
added
Stability
|
The |
When a Hero unit is killed, a heap-use-after-free is hit in W3DRadar::drawHeroIcon().
This happens because W3DRadar::m_cachedHeroPosList contains pointers to object positions, which are never cleaned up when the objects are destroyed.
To solve this problem, the cache Hero list has been reworked and tied to the established
addObjectandremoveObjectfunctions.This fix also revealed a problem with the subsystems reset where cross dependencies can cause issues on GameEngine::reset. This is fixed as well.
I expect that this change fixes the user facing bug #1034. Not tested.
Asan Report
TODO