fix: cleanupImmediately on one unit deletes others - #3311
Conversation
GarbageCollectUnit will drain unitsToBeRemoved to get one unit. The queue is filled at the end of one update and drained at the start of the next, but GameFrame runs before unitHandler.Update, so the queue is non-empty when deleting here with cleanupImmediately. Real Q: Do any current games have this bug? That tripped the assert in debug builds. In release, it would delete every other queued unit a frame early. A very mysterious-seeming bug from the receiving end probably. ParseUnit does not reject dead units, so maybe this would queue a unit a second time and DeleteUnit on the same pointer twice. Null checks are added against dereferencing in case.
|
Looks decent.
The engine technically doesn't make guarantees about when exactly units that finish are cleaned up, and there is an event (RenderUnitDestroyed) when that happens. As long as there aren't obvious visual issues (e.g. a frame where the unit still renders but fails to receive shaders) it should be fine. |
|
Ideally there is some guarantee for "cleanupImmediately", which does specify the "when". I think that is a functioning guarantee, though, and just has this one bug, calling DestroyUnit(cleanupImmediately=true) inside of GameFrame specifically. Maybe a unit script could synchronously delete itself through here, too, which may be a UAF, but the removal does occur. Seems correct. I will take a look at the pipeline from this change to the render update but it seems to me that it would be unaffected. |
Yes, I meant all the N-1 units that die normally via damage etc (as opposed to the one that is being removed with explicit cleanupImmediately from Lua) who would be getting cleaned up a frame early, who don't have a guarantee. |
2 participants
GarbageCollectUnit will drain unitsToBeRemoved to get one unit. The queue is filled at the end of one update and drained at the start of the next, but GameFrame runs before unitHandler.Update, so the queue is non-empty when deleting here with cleanupImmediately. Real Q: Do any current games have this bug?
That tripped the assert in debug builds. In release, it would delete every other queued unit a frame early. A very mysterious-seeming bug from the receiving end probably.
ParseUnit does not reject dead units, so maybe this would queue a unit a second time and DeleteUnit on the same pointer twice. Null checks are added against dereferencing in case.