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 #3502

Merged

Conversation

@benoit-pierre
Copy link
Contributor

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

Example crash: compile with --enable-asan, start a new game, click the "rest" button.

Running Might and Magic: World of Xeen (CD/DOS/English)
xeen.cc: 964078c53f649937ce9a1a3596ce3d9f, 13438429 bytes.
dark.cc: 7f755ce39ea614fa6adb016f8bfc6e43, 11288403 bytes.
=================================================================
==14000==ERROR: AddressSanitizer: global-buffer-overflow on address 0x55ec548b0380 at pc 0x55ec53af1874 bp 0x7ffe1f513e20 sp 0x7ffe1f513e10
READ of size 4 at 0x55ec548b0380 thread T0
    #0 0x55ec53af1873 in Xeen::Combat::moveMonsters() engines/xeen/combat.cpp:505
    #1 0x55ec53b1b0a1 in Xeen::Interface::chargeStep() engines/xeen/interface.cpp:604
    #2 0x55ec53b254b7 in Xeen::Interface::rest() engines/xeen/interface.cpp:1058
    #3 0x55ec53ad2a67 in Xeen::XeenEngine::gameLoop() engines/xeen/xeen.cpp:285
    #4 0x55ec53ad31fd in Xeen::XeenEngine::play() engines/xeen/xeen.cpp:254
    #5 0x55ec53ad39d9 in Xeen::XeenEngine::playGame() engines/xeen/xeen.cpp:210
    #6 0x55ec53ad39d9 in Xeen::XeenEngine::outerGameLoop() engines/xeen/xeen.cpp:165
    #7 0x55ec53ad5fcb in Xeen::XeenEngine::run() engines/xeen/xeen.cpp:140
    #8 0x55ec53a7d4e5 in runGame base/main.cpp:318
    #9 0x55ec53a82f69 in scummvm_main base/main.cpp:626
    #10 0x55ec53970329 in main backends/platform/sdl/posix/posix-main.cpp:45
    #11 0x14a4b4e0bb24 in __libc_start_main (/usr/lib/libc.so.6+0x27b24)
    #12 0x55ec5397322d in _start ([…]/scummvm/scummvm+0x59922d)

0x55ec548b0380 is located 32 bytes to the left of global variable 'MONSTER_GRID_Y' defined in 'engines/xeen/combat.cpp:39:18' (0x55ec548b03a0) of size 192
0x55ec548b0380 is located 0 bytes to the right of global variable 'MONSTER_GRID3' defined in 'engines/xeen/combat.cpp:46:18' (0x55ec548b02c0) of size 192
SUMMARY: AddressSanitizer: global-buffer-overflow engines/xeen/combat.cpp:505 in Xeen::Combat::moveMonsters()
Shadow bytes around the buggy address:
  0x0abe0a90e020: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0abe0a90e030: 00 00 00 00 00 00 00 00 f9 f9 f9 f9 00 00 00 00
  0x0abe0a90e040: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0abe0a90e050: 00 00 00 00 f9 f9 f9 f9 00 00 00 00 00 00 00 00
  0x0abe0a90e060: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
=>0x0abe0a90e070:[f9]f9 f9 f9 00 00 00 00 00 00 00 00 00 00 00 00
  0x0abe0a90e080: 00 00 00 00 00 00 00 00 00 00 00 00 f9 f9 f9 f9
  0x0abe0a90e090: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0abe0a90e0a0: 00 00 00 00 00 00 00 00 f9 f9 f9 f9 00 00 00 00
  0x0abe0a90e0b0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0abe0a90e0c0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07
  Heap left redzone:       fa
  Freed heap region:       fd
  Stack left redzone:      f1
  Stack mid redzone:       f2
  Stack right redzone:     f3
  Stack after return:      f5
  Stack use after scope:   f8
  Global redzone:          f9
  Global init order:       f6
  Poisoned by user:        f7
  Container overflow:      fc
  Array cookie:            ac
  Intra object redzone:    bb
  ASan internal:           fe
  Left alloca redzone:     ca
  Right alloca redzone:    cb
  Shadow gap:              cc
==14000==ABORTING

It looks like the code clearly expects a 7x7 grid:

int arrIndex = -1;
for (int yDiff = 3; yDiff >= -3; --yDiff) {
	for (int xDiff = -3; xDiff <= 3; ++xDiff) {
		Common::Point pt = party._mazePosition + Common::Point(xDiff, yDiff);
		++arrIndex;
[…]
if (canMonsterMove(pt, Res.MONSTER_GRID_BITMASK[MONSTER_GRID_BITINDEX1[arrIndex]],
		MONSTER_GRID_X[arrIndex], MONSTER_GRID_Y[arrIndex], idx)) {
	// Move the monster
	moveMonster(idx, Common::Point(MONSTER_GRID_X[arrIndex], MONSTER_GRID_Y[arrIndex]));
} else {
	if (canMonsterMove(pt, Res.MONSTER_GRID_BITMASK[MONSTER_GRID_BITINDEX2[arrIndex]],
		arrIndex >= 21 && arrIndex <= 27 ? MONSTER_GRID3[arrIndex] : 0,
		arrIndex >= 21 && arrIndex <= 27 ? 0 : MONSTER_GRID3[arrIndex],
		idx)) {
		if (arrIndex >= 21 && arrIndex <= 27) {
			moveMonster(idx, Common::Point(MONSTER_GRID3[arrIndex], 0));
		} else {
			moveMonster(idx, Common::Point(0, MONSTER_GRID3[arrIndex]));
		}
	}
}

So all the MONSTER_GRID_XXX[48] arrays were missing one cell.

@dreammaster
Copy link
Member

@dreammaster dreammaster commented Nov 7, 2021

Another good catch, thanks.

Loading

@dreammaster dreammaster merged commit a64eff9 into scummvm:master Nov 7, 2021
5 of 8 checks passed
Loading
@benoit-pierre benoit-pierre deleted the xeen_fix_out-of-bounds_accesses_2 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