Fix NULL Map dereference in MapIterate #34
Conversation
Missing NULL test in the for-loop initializer meant that the `map` pointer was accessed before being tested. I've documented the macro and made it safer to use. It might be possible to remove the `i` parameter since I don't think anything using this macro accesses that parameter. I'm not sure if `map` needs null testing every loop.
|
My policy is that we do not fix these crashes. The fix is hiding the real problem, which is likely corrupt game state because something did not work right. It is better to crash early than let things potentially get more corrupt and compound the problems further and further. People have been throwing things in Incursion for decades. Why is map not set? How can this happen? What does it mean? |
It's something I'd like to know too. In that case I should probably replace this with more robust code instead. It's just that the current |
|
I am very pedantic about things being better for the players, so feel free to point out if you think I am overly so. Every change we make introduces uncertainty and we should only make them if we are sure we improving things for players is my bar that either of us needs to meet. I find myself asking when a map would ever be completely empty. This feels to me like the real problem. Something has corrupted things before this point. MapIterate is doing us a favour by crashing and showing us. |
|
Well from a player perspective not crashing would be the far better option. For devs, at least having It seems that Thing's missing map pointers are not entirely unusual in this codebase, there are other places which test for this specifically before invoking incursion-roguelike/src/Social.cpp Lines 2440 to 2441 in 490994c In fact |
|
If things are this broken that a maps do not have anything in them, then why would players want them to get worse? |
|
You may have misread the issue. Maps still have stuff in them, but Thing's do not have a reference to the map at various times. This just tells A lot of the code follows outdated C conventions, this is the C solution to this type of problem. Also I just realized that |
|
We need to identify why something broken is involved in game logic. If we change the systems which fine under normal circumstances to hide the problem, then we are not helping the player or developers. Find out how a thing comes to have no map or a map with no things in it or whatever, and find out why it is involved broken like this in game logic. We are not going to be patching MapIterate to work around the core issue. That some broken thing or map is left in place. |
Missing NULL test in the for-loop initializer meant that the
mappointer was accessed before being tested. It was easier to fix this than figure out why the Player's map was set to NULL.I've documented the macro and made it safer to use. It might be possible to remove the
iparameter since I don't think anything using this macro accesses that parameter inside the loop body. I'm not sure ifmapneeds null testing every loop. This is the safest I can make it without editing the callers.Apparently this crash was caused by an NPC throwing something at the player. When the code reached
player.isHostileToPartyOfthe players local map pointer was NULL.