Opened 3 years ago

Closed 3 years ago

#12950 closed defect (fixed)

TITANIC: Asking "what should I do?" will cause a crash

Reported by: tylerszabo Owned by: dreammaster
Priority: normal Component: Engine: Titanic
Version: Keywords:
Cc: Game: Starship Titanic

Description

General Information

  • ScummVM Version: 2.2.0 (Sep 14 2020 23:43:32)
  • Bug details: Load the attached saved game titanic-win.002 then click on DoorBot's icon to summon. Once greeted type "what should I do?" and await a crash.
  • Language of game: English
  • Version of game: GOG installed via GOG Galaxy
  • Operating System: Windows 10 Pro (19041.1.amd64fre.vb_release.191206-1406)
  • Saved game: (attached)

Bug Details

Summoning DoorBot and asking "what should I do?" will cause a crash.

Additional Notes

In addition to DoorBot asking BellBot or DeskBot "what should I do?" will also cause a crash. The punctuation doesn't seem to matter.

At the start of the game (before the credits/intro) the input will just be ignored.

Expected Behavior

DoorBot responds with one of a variety of generic unhelpful responses.

Saved Game and Steps to Reproduce

Load the attached saved game titanic-win.002 then click on DoorBot's icon to summon. Once greeted type "what should I do?" and await a crash.

Attachments (1)

titanic-win.002 (105.3 KB ) - added by tylerszabo 3 years ago.

Download all attachments as: .zip

Change History (4)

by tylerszabo, 3 years ago

Attachment: titanic-win.002 added

comment:1 by antoniou79, 3 years ago

Summary: Asking "what should I do?" will cause a crashTITANIC: Asking "what should I do?" will cause a crash

I think I can reproduce this on the latest code (master branch) on Windows 10, msys2 build.
Just writing "should" suffices to trigger the crash (segmentation fault).

Seems to occur because in this part of code (TTparser::considerRequests()), in this case, _conceptP is nullptr but there's no check for it. For some reason the execution seems to go into findByWordClass() and there this is treated as non-null which leads to segmentation fault.

TTconcept *conceptP = _conceptP->findByWordClass(WC_ACTION);

https://github.com/scummvm/scummvm/blob/dc1717067322bade8c43536679ece9a9b9a87b49/engines/titanic/true_talk/tt_parser.cpp#L1000

Oddly, while debugging with Visual Studio, the execution goes into findByWordClass() but this is treated as null and the method returns nullptr.

We could fix this by doing something like:

TTconcept *conceptP = (_conceptP != nullptr) ? _conceptP->findByWordClass(WC_ACTION) : nullptr;

However, I can see multiple other instances in the same class, where we use _conceptP methods and members unchecked. Not sure if we should fix all the other cases too, or fix the reason why _conceptP is nullptr at that part of the code -- maybe the code wrongly assumes that it should have been initialized earlier or maybe it should have been initialized and it's not?

Last edited 3 years ago by antoniou79 (previous) (diff)

comment:2 by tylerszabo, 3 years ago

I just got local builds working and even Release flavor from MSVC seems to not exhibit the issue. It does look like a nuance of treatment of undefined behavior between compilers. Perhaps, since it's occurring in multiple places that TTconcept * should be replaced with something like PTTconcept that will perform checks automatically and perhaps add non-fatal logging?

comment:3 by dreammaster, 3 years ago

Owner: set to dreammaster
Resolution: fixed
Status: newclosed

For now, I've changed the method to be static and pass in the TTconcept as a parameter. This is semantically the same as the method in the original, so this should work around any undefined behaviour issues causing by it previously being an instance method.

Note: See TracTickets for help on using tickets.