search

rmtew / incursion-roguelike

The legendary computer game Incursion: Hall of the Goblin King!
https://incursion-roguelike.net/
Other
9 stars 1 forks source link

Fix NULL Map dereference in MapIterate #34

Open HexDecimal opened 3 months ago

HexDecimal commented 3 months ago

Missing NULL test in the for-loop initializer meant that the map pointer 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 i parameter since I don't think anything using this macro accesses that parameter inside the loop body. I'm not sure if map needs 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.isHostileToPartyOf the players local map pointer was NULL.

Incursion.exe!Array<long,1000,10>::_Paren(unsigned int index) Line 590
    at ...\src\Base.cpp(590)
Incursion.exe!NArray<long,1000,10>::operator[](int index) Line 63
    at ...\inc\Base.h(63)
Incursion.exe!Creature::isHostileToPartyOf(Creature * c) Line 1567
    at ...\src\Social.cpp(1567)
Incursion.exe!VMachine::CallMemberFunc(short funcid, long h, char n) Line 1327
    at ...\inc\dispatch.h(1327)
Incursion.exe!VMachine::Execute(EventInfo * e, unsigned int _xID, long CP) Line 470
    at ...\src\VMachine.cpp(470)
Incursion.exe!Resource::Event(EventInfo & e, unsigned int xID, short Event) Line 1121
    at ...\src\Annot.cpp(1121)
Incursion.exe!ThrowEvent(EventInfo & e) Line 170
    at ...\src\Event.cpp(170)
Incursion.exe!RealThrow(EventInfo & e) Line 341
    at ...\src\Event.cpp(341)
Incursion.exe!ReThrow(short ev, EventInfo & e) Line 376
    at ...\src\Event.cpp(376)
Incursion.exe!Creature::Strike(EventInfo & e) Line 4267
    at ...\src\Fight.cpp(4267)
Incursion.exe!Creature::Event(EventInfo & e) Line 762
    at ...\src\Creature.cpp(762)
Incursion.exe!ThrowTo(EventInfo & e, Object * t) Line 285
    at ...\src\Event.cpp(285)
Incursion.exe!ThrowEvent(EventInfo & e) Line 244
    at ...\src\Event.cpp(244)
Incursion.exe!RealThrow(EventInfo & e) Line 344
    at ...\src\Event.cpp(344)
Incursion.exe!ReThrow(short ev, EventInfo & e) Line 376
    at ...\src\Event.cpp(376)
Incursion.exe!Creature::OAttack(EventInfo & e) Line 2640
    at ...\src\Fight.cpp(2640)
Incursion.exe!Creature::Event(EventInfo & e) Line 752
    at ...\src\Creature.cpp(752)
Incursion.exe!ThrowTo(EventInfo & e, Object * t) Line 285
    at ...\src\Event.cpp(285)
Incursion.exe!ThrowEvent(EventInfo & e) Line 244
    at ...\src\Event.cpp(244)
Incursion.exe!RealThrow(EventInfo & e) Line 344
    at ...\src\Event.cpp(344)
Incursion.exe!ReThrow(short ev, EventInfo & e) Line 376
    at ...\src\Event.cpp(376)
Incursion.exe!Creature::ProvokeAoO(Creature * c, bool from_move) Line 2751
    at ...\src\Fight.cpp(2751)
Incursion.exe!Monster::PickUp(EventInfo & e) Line 196
    at ...\src\Inv.cpp(196)
Incursion.exe!Creature::Event(EventInfo & e) Line 711
    at ...\src\Creature.cpp(711)
Incursion.exe!ThrowTo(EventInfo & e, Object * t) Line 285
    at ...\src\Event.cpp(285)
Incursion.exe!ThrowEvent(EventInfo & e) Line 244
    at ...\src\Event.cpp(244)
Incursion.exe!RealThrow(EventInfo & e) Line 344
    at ...\src\Event.cpp(344)
Incursion.exe!Throw(short Ev, Object * p1, Object * p2, Object * p3, Object * p4) Line 410
    at ...\src\Event.cpp(410)
Incursion.exe!Monster::ChooseAction() Line 1083
    at ...\src\Monster.cpp(1083)
Incursion.exe!Game::Play() Line 279
    at ...\src\Main.cpp(279)
Incursion.exe!Game::StartMenu() Line 2163
    at ...\src\Main.cpp(2163)
Incursion.exe!main(int argc, char * * argv) Line 348
    at ...\src\Wlibtcod.cpp(348)
[External Code]
rmtew commented 3 months ago

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?

HexDecimal commented 3 months ago

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 MapIterate definition has m && m->Things[i] in the for-loop condition which implies that doing nothing on NULL was the intended behavior. So either the condition is wrong, the initializer is wrong, or the map pointer being nulled during the loop is actually common.

rmtew commented 3 months ago

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.

HexDecimal commented 3 months ago

Well from a player perspective not crashing would be the far better option. For devs, at least having MapIterate documented is a step in the right direction.

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 MapIterate:

https://github.com/rmtew/incursion-roguelike/blob/490994c4a55dc04dc924ca08081caa2765df4190/src/Social.cpp#L2440-L2441

In fact if (m) shows up in a lot of places. So Thing objects may or may not have their map pointer at various times. The existence of the map pointer might not be something to take for granted.

rmtew commented 3 months ago

If things are this broken that a maps do not have anything in them, then why would players want them to get worse?

HexDecimal commented 3 months ago

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 MapIterate to do nothing in this state. Much like in other places with if (m) guards.

A lot of the code follows outdated C conventions, this is the C solution to this type of problem.

Also I just realized that ThrowEvent, RealThrow, and ThrowTo have nothing to do with throwing items. These really need to be better documented.

rmtew commented 3 months ago

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.