Crashes due to projectile calculation involving Scavengers or Features being player 9

[wzdev-ci]

keyword_MAX_PLAYERS keyword_isHumanPlayer resolution_fixed type_bug | by toy

---

In svn/trunk, [7890], the game hangs whenever there's an attack involving scavenger or map features (i.e. houses) which are being calculated in projectile.c as "player 9", which fails the ASSERT for "player < 8" in multiplay.c. This occurs in both single player skirmish and multiplayer. Below is a sample GDB dump, please let me know if any addition information is required.

...
sync    |11:59:09: [SendDroid] Droid created with id of 161441
life    |11:59:09: [SendDroid] ===> sending Droid from 1 id of 161441 
sync    |11:59:10: [sendDroidCheck] sent droid check at tick 148315
never   |11:59:10: [structureBuild] DACTION_NONE: done
never   |11:59:10: [droidUpdateBuild] DACTION_BUILD: done
sync    |11:59:10: [sendDroidCheck] sent droid check at tick 148618
sync    |11:59:10: [sendDroidCheck] sent droid check at tick 148920
sensor  |11:59:10: [combFire] combFire: Accurate prediction range (32)
sync    |11:59:11: [sendDroidCheck] sent droid check at tick 149220
error   |11:59:11: [isHumanPlayer] Player index (9) out of range
error   |11:59:11: [isHumanPlayer] Assert in Warzone: /Users/toyl/warzone/macosx/../src/multiplay.c:467 (player < MAX_PLAYERS), last script event: 'everySec'
/Users/toyl/warzone/macosx/../src/multiplay.c:467: failed assertion `player < 8'

Program received signal SIGABRT, Aborted.
0x96fa9e42 in __kill ()
(gdb) bt
#0  0x96fa9e42 in __kill ()
#1  0x96fa9e34 in kill$UNIX2003 ()
#2  0x9701c23a in raise ()
#3  0x97028679 in abort ()
#4  0x002a90dd in __eprintf (string=0x0, expression=0x0, line=0, filename=0x0) at /var/tmp/gcc/gcc-5493~1/src/gcc/libgcc2.c:1838
#5  0x00156c9d in isHumanPlayer (player=9) at /Users/toyl/warzone/macosx/../src/multiplay.c:467
#6  0x0015a904 in updateMultiStatsDamage (attacker=0, defender=9, inflicted=27) at /Users/toyl/warzone/macosx/../src/multistat.c:232
#7  0x00172d9c in proj_ImpactFunc (psObj=0x198442c0) at /Users/toyl/warzone/macosx/../src/projectile.c:1261
#8  0x00173da1 in proj_Update (psObj=0x198442c0) at /Users/toyl/warzone/macosx/../src/projectile.c:1527
#9  0x00173de1 in proj_UpdateAll () at /Users/toyl/warzone/macosx/../src/projectile.c:1545
#10 0x0012011d in gameLoop () at /Users/toyl/warzone/macosx/../src/loop.c:419
#11 0x0012272f in runGameLoop () at /Users/toyl/warzone/macosx/../src/main.c:654
#12 0x00122cbf in mainLoop () at /Users/toyl/warzone/macosx/../src/main.c:842
#13 0x0012356b in SDL_main (argc=1, argv=0x130e450) at /Users/toyl/warzone/macosx/../src/main.c:1077
#14 0x002a8db9 in -[SDLMain applicationDidFinishLaunching:] (self=0x132b330, _cmd=0x9658c944, note=0x131cf30) at /Users/toyl/warzone/macosx/external/SDL/src/main/macosx/SDLMain.m:302

---

Issue migrated from trac:740 at 2022-04-15 19:01:52 -0700

[wzdev-ci]

anonymous commented

---

Fixed... the patch I used is below:

Index: src/multistat.c
===================================================================
--- src/multistat.c (revision 7892)
+++ src/multistat.c (working copy)
@@ -210,6 +210,11 @@
        return;
    }

+   if (attacker >= MAX_PLAYERS)
+   {
+       return;
+   }
+   
    if(isHumanPlayer(attacker))
    {
        st = getMultiStats(attacker,true);  // get stats
@@ -228,7 +233,11 @@
        ingame.skScores[attacker][0] += (2*inflicted);  // increment skirmish players rough score.
    }

-
+   if (defender >= MAX_PLAYERS)
+   {
+       return;
+   }
+   
    if(isHumanPlayer(defender))
    {
        st = getMultiStats(defender,true);  // get stats

[wzdev-ci]

Per commented

---

That is not really a good fix. We need to know where this errant player 9 is coming from... Do you have a savegame that reproduces the issue?

[wzdev-ci]

toy commented

---

Replying to Warzone2100/old-trac-import#740 (comment:2):

That is not really a good fix. We need to know where this errant player 9 is coming from... Do you have a savegame that reproduces the issue?

True, I just found the same problem occurring in power.c (function: usePower) after I used the above fix (due to player=9). You can reproduce the above problem using the two-player map, Sk-Startup, since it has both Scavengers and empty houses which you can shoot and cause that error. (With scavenger enabled, your game might crash anytime since opponent player could also trigger the crash. On the other hand, with scavenger disabled, one of the sure way of crashing it is just to shoot at the empty houses on the map.)

I am new to this codes, so I don't really know the "right" approach or the original intended operation. However, I did come across the following comment in objmem.c:

/* Destroy a feature */
_ set the player to 0 since features have player = maxplayers+1. This screws up destroyObject
_ it's a bit of a hack, but hey, it works
void killFeature(FEATURE *psDel)
{
    ASSERT( psDel->type == OBJ_FEATURE,
        "killFeature: pointer is not a feature" );
    psDel->player = 0;
    destroyObject((BASE_OBJECT**)apsFeatureLists, (BASE_OBJECT*)psDel);
}

I am just loosely patching these bugs to make it playable for now... maybe when I understand the codes more, I will try to contribute real fixes.

Index: src/multistat.c
===================================================================
--- src/multistat.c (revision 7892)
+++ src/multistat.c (working copy)
@@ -210,6 +210,11 @@
        return;
    }

+   if (attacker >= MAX_PLAYERS)
+   {
+       return;
+   }
+   
    if(isHumanPlayer(attacker))
    {
        st = getMultiStats(attacker,true);  // get stats
@@ -228,7 +233,11 @@
        ingame.skScores[attacker][0] += (2*inflicted);  // increment skirmish players rough score.
    }

-
+   if (defender >= MAX_PLAYERS)
+   {
+       return;
+   }
+   
    if(isHumanPlayer(defender))
    {
        st = getMultiStats(defender,true);  // get stats
Index: src/power.c
===================================================================
--- src/power.c (revision 7892)
+++ src/power.c (working copy)
@@ -151,6 +151,8 @@

 void usePower(int player, float quantity)
 {
+   if(player >= MAX_PLAYERS)
+       return;
    ASSERT(asPower[player].currentPower >= quantity, "not enough power");
    asPower[player].currentPower -= quantity;
 }

[wzdev-ci]

Buginator commented

---

Replying to Warzone2100/old-trac-import#740 (comment:2):

That is not really a good fix. We need to know where this errant player 9 is coming from... Do you have a savegame that reproduces the issue?

I agree with Per. We need to find why/where/what version, this player 9 is coming from, since this should never happen.

[wzdev-ci]

toy commented

---

Replying to Warzone2100/old-trac-import#740 (comment:4):

Replying to Warzone2100/old-trac-import#740 (comment:2):

That is not really a good fix. We need to know where this errant player 9 is coming from... Do you have a savegame that reproduces the issue?

I agree with Per. We need to find why/where/what version, this player 9 is coming from, since this should never happen.

Whoever wrote killFeature() in objmem.c probably know... since that function was written exactly to counter this problem. Applying the patch for multistat.c works well for me so far... I made a mistake regarding the problem with power.c though... apparently it's not related... (see bug #468)

I don't think it hurts to put array index out-of-bound check before any attempt to access that array at the specified index, especially when the game has such an unclean approach to identifying player.

[wzdev-ci]

Per commented

---

The problem comes from feature.c, specifically the line: psFeature->player = MAX_PLAYERS+1; //set the player out of range to avoid targeting confusions

So this was done deliberately, and our more recent stringency in checking has turned this into a problem. I think the long-term fix is to expand MAX_PLAYERS to 9 and stuff anything related to civilians and scavengers into player 9.

[wzdev-ci]

toy commented

---

Replying to Warzone2100/old-trac-import#740 (comment:6):

The problem comes from feature.c, specifically the line: psFeature->player = MAX_PLAYERS+1; //set the player out of range to avoid targeting confusions

So this was done deliberately, and our more recent stringency in checking has turned this into a problem. I think the long-term fix is to expand MAX_PLAYERS to 9 and stuff anything related to civilians and scavengers into player 9.

Cool! By the way, I think there was a define line checking for MAX_PLAYERS = 4 or 8, and if not, it would throw a compile error. You might also want to change that if you choose to up the value.

Thanks!

[wzdev-ci]

Buginator commented

---

Replying to Warzone2100/old-trac-import#740 (comment:6):

The problem comes from feature.c, specifically the line: psFeature->player = MAX_PLAYERS+1; //set the player out of range to avoid targeting confusions

So this was done deliberately, and our more recent stringency in checking has turned this into a problem. I think the long-term fix is to expand MAX_PLAYERS to 9 and stuff anything related to civilians and scavengers into player 9.

Hmm, I thought we were going to allow 'spectators' at one point, so we really need to keep this in mind when doing future code enhancements.

[wzdev-ci]

dak180 uploaded file dks1.zip (190.7 KiB)

Saved game from 2 - 3 min before crash.

[wzdev-ci]

Per set resolution to fixed

[wzdev-ci]

Per commented

---

Fixed in #762

Let me know if there are any other, similar issues left.

[wzdev-ci]

Per changed status from new to closed