Occasional use after free in drawEntities()

[lumidify]

Very rarely and unpredictably, the game crashes while trying to draw the entities. My guess is that an entity occasionally is removed by one of the three calls to drawEntities() in draw.c:draw() but is still referenced in the array entity.c:drawLayer, and thus the next call to drawEntities() crashes while trying to draw it (still in the same frame since drawLayer is cleared at the beginning of the next frame).

To test, I added a check to the garbage cleaning code in drawEntities() that additionally loops through all entities in drawLayer before removing an entity to check if it is still referenced there and prints out its address if that is the case. The next time the crash occurred, that test did find the entity referenced still:

REMOVING REFERENCED: 0x246d3a89000

When I checked the core file with gdb, the invalid entity had the same address:

#0  0x0000024497265275 in drawEntities (depth=1) at src/entity.c:292
292                             if (self->inUse == TRUE && !(self->flags & NO_DRAW) && self->layer == depth)
(gdb) bt
#0  0x0000024497265275 in drawEntities (depth=1) at src/entity.c:292
#1  0x000002449725da02 in draw () at src/draw.c:139
#2  0x000002449725e4a8 in main (argc=1, argv=0x7f7ffffc8e58) at src/main.c:368
Current language:  auto; currently minimal
(gdb) p self
$1 = (Entity *) 0x246d3a89000
(gdb) p self->inUse
Cannot access memory at address 0x246d3a89000

This particular call to drawEntities() on line 139 of draw.c is the second call in draw(), so it does point towards my guess that the first call removes the entity while it is still referenced by drawLayer (but not by any other entities because those are checked by the garbage cleaning).

I'm still not entirely sure what the actual root cause is because somewhere, inUse has to be set to FALSE for the entity so that it is destroyed by the garbage cleaner. Maybe it's an issue with doEntities() because the entity is always added to drawLayer if inUse was originally TRUE, even if that changes during the course of the function. I don't know enough about that part to be able to say whether that's possible, though.

Regardless, would it maybe be a good idea to move the garbage cleaning code to a separate function that's only called once at the end of each frame instead of calling it every time drawEntities() is called? Unless I have misunderstood something, that should at least fix this issue because drawLayer is cleared in the next frame anyways.

[riksweeney]

Updating the code to remove the referenced entities after the foreground layer has been drawn should solve this.

[lumidify]

My idea would be something like this: https://github.com/lumidify/edgar/commit/c18ae13b5e318c3a3832cd343bd420b66b7ee812. All it does is move the garbage cleaning code to a separate function that is called only once from the main loop at the end of each frame. If that looks good to you, I can create a pull request.

Note that I can't know for sure yet that this really fixes the problem since it's so rare, but if I've understood the reason for the crash correctly, it should.

[lumidify]

Closing this as it has been fixed now. Thanks!