MantisBT - VCMI
View Issue Details
IDProjectCategoryView StatusDate SubmittedLast Update
0003117VCMIAI - Adventure Mappublic2020-04-26 13:432020-05-01 03:35
Reportermoog 
Assigned ToShalthrea 
PrioritynormalSeveritycrashReproducibilityalways
StatusresolvedResolutionfixed 
Platformamd64OSLinuxOS VersionGentoo Linux
Product Version0.99 
Target VersionFixed in Version1.next 
Summary0003117: Game crashes during AI turn
DescriptionGame may randomly crash during AI turn for no apparent reason.

I have managed to get a save state where it is always possible to reproduce this issue.
Steps To Reproduce1. Load the included savestate.
2. Have Solmyr collect the sulphur, treasure chest, recruit gargoyles and collect crystals - in that particular order.
3. Have the other hero build a ship in the nearby stockyard and board the new ship.
4. Build sculptor's wings in the first city.
5. End turn.
Additional InformationThis issue is reproducible in git commits:
- dca5d86e7a6d18d2ddac7258f98a0ce08c691a6e (branch develop, March 14 2020)
- 55b54024a82aaad3b4572d674d424c2247a47930 (branch develop, March 18 2020)
TagsNo tags attached.
Relationships
Attached Files? bad_save.tar.xz (368,568) 2020-04-26 13:43
https://bugs.vcmi.eu/file_download.php?file_id=3061&type=bug
txt gdb.txt (461,549) 2020-04-26 13:45
https://bugs.vcmi.eu/file_download.php?file_id=3062&type=bug
Issue History
Date ModifiedUsernameFieldChange
2020-04-26 13:43moogNew Issue
2020-04-26 13:43moogFile Added: bad_save.tar.xz
2020-04-26 13:45moogFile Added: gdb.txt
2020-04-26 13:46moogNote Added: 0007891
2020-04-27 00:51ShalthreaNote Added: 0007893
2020-04-30 23:37ShalthreaNote Added: 0007896
2020-05-01 03:35SXXStatusnew => resolved
2020-05-01 03:35SXXFixed in Version => 1.next
2020-05-01 03:35SXXResolutionopen => fixed
2020-05-01 03:35SXXAssigned To => Shalthrea

Notes
(0007891)
moog   
2020-04-26 13:46   
gdb.txt is the GDB backtrace that I got when successfully reproducing the issue. It was made on the binaries from 55b54024a82aaad3b4572d674d424c2247a47930 commit.
(0007893)
Shalthrea   
2020-04-27 00:51   
Can confirm that it reproduces. Looks like we're getting a call to the `doChannelProbing` lambda inside `VCAI::moveHeroToTile(int3, HeroPtr)`, which expects the hero's location to contain some object other than the hero itself. When it doesn't, we get a `nullptr` dereference.
(0007896)
Shalthrea   
2020-04-30 23:37   
Alright, to be a bit more specific about what is going on here: there is a Cyra in a whirlpool, and the AI decides to have her revisit the whirlpool a number of times until she is down to a single gremlin. Once this occurs, the hero is considered "protected" from the ill effects of the whirlpool, and is allowed to choose freely from the whirlpool exits when revisiting (`CGWhirlpool::onHeroVisit`).

It just so happens that once Cyra is down to only a single gremlin, the AI no longer is actively pushing her to revisit the whirlpool. Instead, the AI is just finding no actions for her to take overall, and is nearing the end of `VCAI::wander`. However, the fix for
https://bugs.vcmi.eu/view.php?id=1126 [^]
steps in and forces her to do one last revisit of the whirlpool.

When `VCAI::showTeleportDialog` is triggered with this last revisit, and the message is thus filled with all the exit alternative, a routine begins to store these exits for later exploration, keeping them in `teleportChannelProbingList`. In most cases, this list would be filled by the main logic in `VCAI::moveHeroToTile`, and then immediately each entry would be visited and the list cleared. Unfortunately, since this is a case of revisiting, there is special case logic in `VCAI::moveHeroToTile` that kicks in. This special case logic never performs the probing, and never clears out the list.

Cyra's actions end, and Piquedram attempts to visit a campfire. Unfortunately, since `teleportChannelProbingList` is persisted between heroes, he then attempts to perform probing for the whirlpools of interest that remain in the list, but he is not standing on anything visitable. This leads to the scenario that I described in my previous comment, and we get a `nullptr` dereference.

While I'm not 100% comfortable with the design, I believe that the most appropriate action here is to clear `teleportChannelProbingList` at the end of the revisiting special case logic of `VCAI::moveHeroToTile`, and will prepare a pull request doing exactly this.