[GEN][ZH] Fix heap-use-after-free in W3DRadar::drawHeroIcon() #1035
Conversation
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.
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 |
|
I think this entire change can be made far simpler and have it touch fewer places by just changing the original design to cache the object ID of the hero, then retrieve the object by its object ID in the loop and check if it is null or not before continuing or removing the object ID from the cache. This is how quite a few other systems already work. |
The disadvantage of this approach is that every iteration needs to test if the object is valid, and then remove it if it no longer is used. Or it needs a prior cleanup update to detect which objects have been deleted. With the proposed implementation, we have a guarantee that all hero objects are valid and no headache. Also note that class Radar also maintains a list of all other Radar objects, and it also holds pointer to Object. So this is consistent with how it is done in class Radar. Also if id lookups can be avoided, then we should do that, because id lookups likely have additional cost, some more than others. |
yes but it also keeps objects and the radar decoupled. I also did not realise that the underlying radar class keeps a list of objects to be seen on the radar. Objects themselves also have to know about the radar too, which is quite nasty, as they removed themselves from it on destruction etc. But i can see where you were going with this overall. |
GeneralsMD/Code/Libraries/Source/WWVegas/WWDownload/CMakeLists.txt
Outdated
Show resolved
Hide resolved
If we want to decouple then it would be better to use a listener/event approach. |
Mauller
left a comment
Mauller
left a comment
There was a problem hiding this comment.
I would say this is okay overall with it keeping to what the radar base class already does.
xezon
force-pushed
the
xezon/fix-w3dradar-hero-bug
branch
from
e6437b9 to
114ce85
Compare
|
Rebased on latest main. Cmake changes removed. Replicated in Generals with 1 small conflict |
|
This change does not touch RTS_INTERNAL or Logging, so should be ok to merge now. |
barefootlogician
left a comment
barefootlogician
left a comment
There was a problem hiding this comment.
This should not have been merged. It significantly reduces maintainability
There was a problem hiding this comment.
why was this not added in the utility folder instead and why are two copies of the same file being added? Also why are we not using standard library algorithms instead?
There was a problem hiding this comment.
Also why are we not using standard library algorithms instead?
What do you propose we use instead?
There was a problem hiding this comment.
remove erase idiom
bool W3DRadar::removeObject(Object* obj)
{
bool removed = Radar::removeObject(obj);
if (!removed)
return false;
if (obj->isHero())
m_cachedHeroObjectList.erase(std::remove(m_cachedHeroObjectList.begin(), m_cachedHeroObjectList.end(), obj), m_cachedHeroObjectList.end());
return true;
}There was a problem hiding this comment.
As for find_and_erase_unordered, after we drop vc6 we can create an rts::unstable_remove function and use it in places in the code where it can be demonstrated with profiling that it yields a significant performance improvement.
There was a problem hiding this comment.
I'm not sure if this is to be considered an improvement:
stl::find_and_erase_unordered(m_cachedHeroObjectList, obj);
->
m_cachedHeroObjectList.erase(std::remove(m_cachedHeroObjectList.begin(), m_cachedHeroObjectList.end(), obj), m_cachedHeroObjectList.end());
we can create an rts::unstable_remove function and use it in places in the code where it can be demonstrated with profiling that it yields a significant performance improvement
Frankly, I disagree as not every improvement needs to be measured. In my opinion, improvements should not affect correctness naturally, and should not be disproportionate to the scope of the change. The scope of the change here is trivial.
There was a problem hiding this comment.
Why is O n/2 not a thing :D . It is to me. Going through half the range on average is faster than going through the full range always.
But why not use std::set instead
Because std::set is not space efficient/contiguous. We use std::vector because READ is most important per frame. Insert/erase happens sparingly and is therefore not performance priority.
There was a problem hiding this comment.
Why is O n/2 not a thing :D . It is to me. Going through half the range on average is faster than going through the full range always.
But why not use std::set instead
Because std::set is not space efficient/contiguous. We use std::vector because READ is most important per frame. Insert/erase happens sparingly and is therefore not performance priority.
Because thats how Big O notation works. Its about how the value scales in respect to scaling the input size n. Coefficients are dropped because they would be the same in the scaled value and the previous value and cancel out.
If it wasn't this way then I could write a helper function bar that only searches 10 elements. Obiously the performance of bar is always fixed ie constant time. Now I could write a function foo that iterates in steps of size 10 and calls bar on the next 10 elements. Technically foo would need 10 times less iterations than a function that that iterated with a step size of 1. Now I could claim I've created a search function thats 10 times faster because it has a time complexity of O (n/10) instead of O (n)
There was a problem hiding this comment.
There was a problem hiding this comment.
Well foo is C n/20 then
…heSuperHackers#1240) Enemy and friendly Hero units no longer show on Radar and no longer crash the game on save/load
5 participants
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