Opened 10 months ago

Closed 9 months ago

#14578 closed defect (fixed)

SHERLOCK: ROSETATTOO: Incorrect talk history due to out-of-bounds access

Reported by: PushmePullyu Owned by: sev-
Priority: normal Component: Engine: Sherlock
Version: Keywords:
Cc: PushmePullyu Game: Sherlock Holmes: Case of the Rose Tattoo

Description (last modified by PushmePullyu)

Tested with master e0a146f9086ae523229d6e8173c39d290e669449 on Linux x86_64 using the English CD version of Rose Tattoo.

The talk history is held in

Common::Array<Sherlock::TalkHistoryEntry> Sherlock::Talk::_talkHistory

Every TalkHistoryEntry holds 16 bools, each corresponding to a statement:

engines/sherlock/talk.h:

struct TalkHistoryEntry {
	bool _data[16];
	...
}

However, some talk files have more than 16 statements. Since this is never checked, it will lead to out-of-bounds writes/reads, accessing memory in the next TalkHistoryEntry instead.

Other than text color, incorrect talk history also affects recording of statements in the journal and updating the _holmesQuotient.

To reproduce with "WIGG01B.TLK" statement 17 (_talkHistory[984]._data[16]) and "SPIT36D.TLK" statement 1 (_talkHistory[985]._data[0]):

  1. Load attached save
  2. Talk to Wiggins; note that option 3 is not marked as used before (it should be shown brighter than option 1)
  3. Abort the talk without selecting any statement by pressing ESC
  4. Exit to the city map
  5. Go to Spitalfields (rightmost icon on the map)
  6. Exit to the city map
  7. Go back to 221B Baker St. (magnifying glass icon in the NW part of the city)
  8. Talk to Wiggins again; note that option 3 is now marked as used (it should be shown darker)

Notes:

  • Rose Tattoo's "WATS02A.TLK" contains 31 statements, which seems to be the largest number used.
  • I did not test this with Scalpel nor do I know its maximum number of statements.
  • Scalpel's talk files never exceed 16 statements, so it is not affected by the problem

Attachments (1)

rosetattoo.016 (26.7 KB ) - added by PushmePullyu 10 months ago.

Download all attachments as: .zip

Change History (4)

by PushmePullyu, 10 months ago

Attachment: rosetattoo.016 added

comment:1 by PushmePullyu, 10 months ago

Owner: set to PushmePullyu
Resolution: pending
Status: newpending

comment:2 by PushmePullyu, 9 months ago

Description: modified (diff)

comment:3 by sev-, 9 months ago

Owner: changed from PushmePullyu to sev-
Resolution: pendingfixed
Status: pendingclosed

The PR has now been merged. Thank you!

Note: See TracTickets for help on using tickets.