Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

XEEN: fix out-of-bounds accesses #3501

Merged

Conversation

@benoit-pierre
Copy link
Contributor

@benoit-pierre benoit-pierre commented Nov 7, 2021

As reported by the compiler:

engines/xeen/combat.cpp: In constructor ‘Xeen::Combat::Combat(Xeen::XeenEngine*)’:
engines/xeen/combat.cpp:97:21: warning: array subscript 32 is above array bounds of ‘int [32][32]’ [-Warray-bounds]
   97 |         Common::fill(&_monsterMap[0][0], &_monsterMap[32][32], 0);
      |         ~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from ./engines/xeen/character.h:30,
                 from engines/xeen/combat.cpp:25:
./engines/xeen/combat.h:166:13: note: while referencing ‘Xeen::Combat::_monsterMap’
  166 |         int _monsterMap[32][32];
      |             ^~~~~~~~~~~
engines/xeen/combat.cpp: In member function ‘void Xeen::Combat::moveMonsters()’:
engines/xeen/combat.cpp:455:21: warning: array subscript 32 is above array bounds of ‘int [32][32]’ [-Warray-bounds]
  455 |         Common::fill(&_monsterMap[0][0], &_monsterMap[32][32], 0);
      |         ~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from ./engines/xeen/character.h:30,
                 from engines/xeen/combat.cpp:25:
./engines/xeen/combat.h:166:13: note: while referencing ‘Xeen::Combat::_monsterMap’
  166 |         int _monsterMap[32][32];
      |             ^~~~~~~~~~~
@benoit-pierre
Copy link
Contributor Author

@benoit-pierre benoit-pierre commented Nov 7, 2021

This other call in engines/xeen/dialogs/dialogs_spells.cpp:1030 looks fishy too:

	int grid[7][7];

	SpriteResource sprites(ccNum ? "detectmn.icn" : "detctmon.icn");
	Common::fill(&grid[0][0], &grid[6][6], 0);

Shouldn't that be &grid[6][7]?

Loading

@dreammaster
Copy link
Member

@dreammaster dreammaster commented Nov 7, 2021

Thanks for fixing the combat fills, and you're right about the grid filling. It's the minor annoyance of fill taking end + 1 as the last parameter.. you really need to pay attention with multi-dimensional arrays. Merging this in, and I'll do a further commit for spells, thanks.

Loading

@dreammaster dreammaster merged commit 2fbd26d into scummvm:master Nov 7, 2021
5 of 8 checks passed
Loading
@benoit-pierre
Copy link
Contributor Author

@benoit-pierre benoit-pierre commented Nov 7, 2021

Maybe it would make sense to add 2 helper macros:

#define FILL_1D_ARRAY(Array, Val)  Common::fill(&(Array)[0], &(Array)[sizeof (Array) / sizeof ((Array)[0])], (Val))
#define FILL_2D_ARRAY(Array, Val)  Common::fill(&(Array)[0][0], &(Array)[sizeof (Array) / sizeof ((Array)[0]) - 1][sizeof ((Array)[0]) / sizeof ((Array)[0][0])], (Val))

Example use:

diff --git i/engines/xeen/combat.cpp w/engines/xeen/combat.cpp
@@ -109,13 +109,13 @@ static const int MONSTER_ITEM_RANGES[6] = { 10, 20, 50, 100, 100, 100 };
 /*------------------------------------------------------------------------*/
 
 Combat::Combat(XeenEngine *vm): _vm(vm), _missVoc("miss.voc") {
-	Common::fill(&_attackMonsters[0], &_attackMonsters[26], 0);
-	Common::fill(&_shootingRow[0], &_shootingRow[MAX_PARTY_COUNT], 0);
-	Common::fill(&_monsterMap[0][0], &_monsterMap[31][32], 0);
-	Common::fill(&_monsterMoved[0], &_monsterMoved[MAX_NUM_MONSTERS], false);
-	Common::fill(&_rangeAttacking[0], &_rangeAttacking[MAX_NUM_MONSTERS], false);
-	Common::fill(&_gmonHit[0], &_gmonHit[36], 0);
-	Common::fill(&_missedShot[0], &_missedShot[MAX_PARTY_COUNT], 0);
+        FILL_1D_ARRAY(_attackMonsters, 0);
+        FILL_1D_ARRAY(_shootingRow, 0);
+        FILL_2D_ARRAY(_monsterMap, 0);
+        FILL_1D_ARRAY(_monsterMoved, false);
+        FILL_1D_ARRAY(_rangeAttacking, false);
+        FILL_1D_ARRAY(_gmonHit, 0);
+        FILL_1D_ARRAY(_missedShot, 0);
 	_globalCombat = 0;
 	_whosTurn = -1;
 	_itemFlag = false;

This would simplify things and take care of a few compilation warnings and/or bugs (like this one: https://github.com/scummvm/scummvm/blob/master/engines/glk/hugo/stringfn.h#L45).

Loading

@benoit-pierre benoit-pierre deleted the xeen_fix_out-of-bounds_accesses_1 branch Nov 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants