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

nextStep can return -1 in Monster.c:2020 #70

Closed
krypt-n opened this issue Mar 29, 2020 · 8 comments
Closed

nextStep can return -1 in Monster.c:2020 #70

krypt-n opened this issue Mar 29, 2020 · 8 comments
Labels
bug Something isn't working

Comments

@krypt-n
Copy link

krypt-n commented Mar 29, 2020

Hi,

I built BrogueCE with -fsanitize=undefined and noticed the following errors:

src/brogue/Monsters.c:2021:34: runtime error: index -1 out of bounds for type 'const short [8][2]'
    #0 0x55acec595c28 in pathTowardCreature /home/mfunk/src/BrogueCE/src/brogue/Monsters.c:2021:34
    #1 0x55acec5afd1a in monstersTurn /home/mfunk/src/BrogueCE/src/brogue/Monsters.c:3351:17
    #2 0x55acec64955e in playerTurnEnded /home/mfunk/src/BrogueCE/src/brogue/Time.c:2410:25
    #3 0x55acec5c1926 in playerMoves /home/mfunk/src/BrogueCE/src/brogue/Movement.c
    #4 0x55acec4e37b6 in executeKeystroke /home/mfunk/src/BrogueCE/src/brogue/IO.c:2592:13
    #5 0x55acec561241 in mainBrogueJunction /home/mfunk/src/BrogueCE/src/brogue/MainMenu.c:852:25
    #6 0x55acec651d46 in _gameLoop /home/mfunk/src/BrogueCE/src/platform/sdl2-platform.c:367:5
    #7 0x55acec64e81e in main /home/mfunk/src/BrogueCE/src/platform/main.c:223:5
    #8 0x7f9ed44cc022 in __libc_start_main (/usr/lib/libc.so.6+0x27022)
    #9 0x55acec40752d in _start (/home/mfunk/src/BrogueCE/bin/brogue+0x1ef52d)

src/brogue/Monsters.c:2022:34: runtime error: index -1 out of bounds for type 'const short [8][2]'
    #0 0x55acec595cac in pathTowardCreature /home/mfunk/src/BrogueCE/src/brogue/Monsters.c:2022:34
    #1 0x55acec5afd1a in monstersTurn /home/mfunk/src/BrogueCE/src/brogue/Monsters.c:3351:17
    #2 0x55acec64955e in playerTurnEnded /home/mfunk/src/BrogueCE/src/brogue/Time.c:2410:25
    #3 0x55acec5c1926 in playerMoves /home/mfunk/src/BrogueCE/src/brogue/Movement.c
    #4 0x55acec4e37b6 in executeKeystroke /home/mfunk/src/BrogueCE/src/brogue/IO.c:2592:13
    #5 0x55acec561241 in mainBrogueJunction /home/mfunk/src/BrogueCE/src/brogue/MainMenu.c:852:25
    #6 0x55acec651d46 in _gameLoop /home/mfunk/src/BrogueCE/src/platform/sdl2-platform.c:367:5
    #7 0x55acec64e81e in main /home/mfunk/src/BrogueCE/src/platform/main.c:223:5
    #8 0x7f9ed44cc022 in __libc_start_main (/usr/lib/libc.so.6+0x27022)
    #9 0x55acec40752d in _start (/home/mfunk/src/BrogueCE/bin/brogue+0x1ef52d)

this suggests that dir is -1 in the following snippet:

    dir = nextStep(target->mapToMe, monst->xLoc, monst->yLoc, monst, true);
    targetLoc[0] = monst->xLoc + nbDirs[dir][0];
    targetLoc[1] = monst->yLoc + nbDirs[dir][1];

The errors reliably occur for me in this zipped recording at turn 686 with BrogueCE a53673d compiled with clang 9.
I am not really familiar with the codebase and cannot tell if this results in an actual observable bug or how to fix this in a good way. Feel free to close this issue if this is something that can be ignored.

@tmewett
Copy link
Owner

tmewett commented Mar 29, 2020

Thanks for the report. On what turn do you get these errors? And do they cause an out-of-sync error? I get an out-of-sync error on turn 686, does that seem right?

@krypt-n
Copy link
Author

krypt-n commented Mar 29, 2020

Yeah, the errors happen at turn 686, however I do not get an out-of-sync error.

@kz-dev
Copy link
Contributor

kz-dev commented Mar 30, 2020

I also have a out-of-sync at turn 686 when view your recording. You press "j" to go in water. Can we find seed in the recording file ? Or have you the seed ?

@krypt-n
Copy link
Author

krypt-n commented Mar 30, 2020

The seed is 232800572

@tmewett
Copy link
Owner

tmewett commented Mar 30, 2020

Ok! Looks like this is an out-of-sync bug. Good find, now we need to work out the cause.

@tmewett tmewett added the bug Something isn't working label Mar 30, 2020
@krypt-n
Copy link
Author

krypt-n commented Mar 30, 2020

I found another recording with this issue at turn 790, if that helps.

@tmewett
Copy link
Owner

tmewett commented Apr 3, 2020

I've found the cause of this issue.

If you go to turn 686 on your first recording, there are two small pools, each containing an eel. By some strange dungeon generation (bug?), those two eels are actually part of the same horde: one is a follower of the other. So the follower, being more than 2 cells away from its leader (condition in monstersTurn at Monsters.c:3350), tries to path towards the other eel with pathTowardCreature. However, there is no traversable path to follow - so nextStep returns -1, which isn't checked, and hence invalid access occurs.

This is a really good find, thanks for the report!

tmewett added a commit that referenced this issue Apr 5, 2020
Previously, if nextStep returned NO_DIRECTION (-1) an invalid access would
occur, causing an out-of-sync error. (#70)

This also slightly changes the logic. Previously, if moveMonsterPassivelyTowards
failed, a random step would be tried. Now a random step is only tried if
nextStep fails. I think this should be almost the same, since nextStep checks
whether it can pass any adjacent monsters, which seems to be why the
moveMonsterPassivelyTowards return value was checked.
@flend
Copy link
Collaborator

flend commented Apr 10, 2020

Amazing work guys!

@krypt-n krypt-n closed this as completed Apr 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

4 participants