search

DFHack / stonesense

A retro isometric visualizer for Dwarf Fortress
https://github.com/DFHack/dfhack
Other
370 stars 54 forks source link

Fix some Tile memory safety issues #76

Closed lethosor closed 3 years ago

lethosor commented 3 years ago

0fb9abc5f8a7511269ab072317bbd366bc2fa9ca introduced some paths where Tile instances could be double-freed: specifically, WorldSegment::Reset() would call Tile::InvalidateAndDestroy() on individual tiles, followed by delete[] on the array containing them, and both of these call the Tile destructor.

Fixes an issue reported on Discord, and also fixes #73 (at least from my initial testing).

Marking as a draft because I would like to potentially revisit how Tile instances are stored. They contain at least one non-POD object (Tile::building::sprites is a vector, and in fact was what usually triggered the crash on my end), and the changes here might result in some memory leaks if those are not freed properly.

quietust commented 3 years ago

The primary goal of this patch is to revert the changes that caused the crashes on Linux - further improvements to the logic should probably be done in another patch.

lethosor commented 3 years ago

Yeah, I think we can go with this. Of note, the Tile() constructor commented on by @Cyprex isn't used at all now (it was added in 0fb9abc5f8a7511269ab072317bbd366bc2fa9ca to accomocate new Tile[]). memset should zero out all fields appropriately. I am still concerned about it leaking vectors' memory, but this is good to have to resolve the immediate crash.