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

ILLUSIONS: Fix text handling crash on big-endian systems #3125

Merged

Conversation

@dwatteau
Copy link
Contributor

@dwatteau dwatteau commented Jul 3, 2021

From what I understand, Illusions games use wide strings, and we swap their endianness on big-endian systems. However, some strings would sometimes remain in the wrong endianness, such as the second sentence from the army guy at the start: the 0x0057 character (87d) being misread as 0x5700 (22272), and so getCharInfo() wasn't happy with that.

The problem appears to be in the loop doing the uint16 endianness swap:

#if defined(SCUMM_BIG_ENDIAN)
	for (byte *ptr = (byte *)_text; ptr != _tblPtr; ptr += 2) {
		WRITE_UINT16(ptr, SWAP_BYTES_16(READ_UINT16(ptr)));
	}
#endif

It stops just before _tblPtr. But, from what I understand, _tblPtr is always going to be way pat all TalkEntry text parts, as hinted by these debug lines:

TalkEntry::load() _talkId: 000F00C8; textOffs: 0000065C; tblOffs: 0000285A; voiceNameOffs: 00003C04
TalkEntry::load() _talkId: 000F00C9; textOffs: 000006E8; tblOffs: 00002896; voiceNameOffs: 00003C0C
TalkEntry::load() _talkId: 000F00CA; textOffs: 000007A8; tblOffs: 000028E8; voiceNameOffs: 00003C14
TalkEntry::load() _talkId: 000F00CB; textOffs: 0000080C; tblOffs: 0000290E; voiceNameOffs: 00003C1C

... effectively reversing the string multiple times, and at incorrect positions. Thus, some characters would remain in the incorrect endianness, and the game would crash.

So, I've changed this condition so that the loop stops right before any full-zero uint16 value (since that's also how TextDrawer::textHasChar operates). I have no deep understanding of how the engine works, but it seems to fix the problem.

Tested with Duckman on Debian 11 PowerPC, and also tested by raziel on AmigaOS4 (thanks!). (Not tested back on little-endian systems, since the change is in a SCUMM_BIG_ENDIAN ifdef.)

(I also tried doing a PS3 build, but it seems that scummvm/dockerized-toolchains:ps3 can't produce working binaries at the moment, because I can't even rebuild a working 2.2.0 binary, and the current PS3 snapshots are also KO, while the official 2.2.0 release runs fine).

Should hopefully fix and close Trac tickets #11528 and #11236 (and also a PS3 thread on the forum).

The loop reversing the wide characters on big-endian systems would loop
until _tblPtr, which is way past all TalkEntry text parts, effectively
reversing the string multiple times at incorrect positions.  Thus, some
characters would remain in the incorrect endianness, and the game would
crash.

Trac tickets #11528 and #11236.
@raziel-
Copy link
Contributor

@raziel- raziel- commented Jul 3, 2021

Sweet :-D

@bluegr
Copy link
Member

@bluegr bluegr commented Jul 3, 2021

Since this is a BE specific fix and it has been confirmed to fix related crashes in such systems, it's safe to be merged. Thanks for your work! Merging

@bluegr bluegr closed this Jul 3, 2021
@bluegr bluegr reopened this Jul 3, 2021
@bluegr bluegr merged commit a933dc7 into scummvm:master Jul 3, 2021
1 check passed
1 check passed
@codacy-production
Codacy Static Code Analysis Codacy Static Code Analysis
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants