Opened 2 years ago

Closed 2 years ago

#13777 closed patch (fixed)

SCUMM: Unaligned memory access in IMuseDigital

Reported by: kreudom Owned by: AndywinXp
Priority: normal Component: Engine: SCUMM
Version: Keywords: unaligned, strict-alignment, 3ds
Cc: Game: Monkey Island 3

Description

It is possible that the mapCurPos pointer in IMuseDigital::dispatchConvertMap is cast to int32* and then written to while not properly aligned.

I discovered the bug while trying to run The Curse of Monkey Island on my build of version 2.6.0 for the 3DS. Starting the game immediately results in a crash before the first menu of the game can be shown. This also occurs when building from the master branch.

Debugging revealed that this crash happens due to an unaligned write in IMuseDigital::dispatchConvertMap. The pointer mapCurPos can become unaligned if rawMap contains a TEXT block with a length that is not divisible by 4.

Replacing the pointer cast and assigment with memcpy resolves the issue and lets me start the game regularly. When compiling for the 3DS, the memcpy calls are optimized out, therefore I assume this should not noticeably impact performance.

I did not check if there are any other alignment issues in the surrounding code. I also did not check if it is correct to assume that the arguments to dispatchConvertMap are always correctly aligned.

I will attach my patch for your convenience.

Attachments (2)

0001-IMuseDigital-fix-unaligned-access.patch (2.2 KB ) - added by kreudom 2 years ago.
fsanitize-alignment-comi-dimuse-dispatch.txt (8.1 KB ) - added by dwatteau 2 years ago.
fsanitize=alignment warning leftovers with COMI

Download all attachments as: .zip

Change History (10)

comment:1 by dwatteau, 2 years ago

Game: Monkey Island 3
Keywords: unaligned strict-alignment 3ds added
Summary: Unaligned memory access in IMuseDigitalSCUMM: Unaligned memory access in IMuseDigital

comment:2 by dwatteau, 2 years ago

Hi,

Thanks for this test and patch, it looks good to me. Do you want to submit it yourself as a GitHub PR for review and integration? https://github.com/scummvm/scummvm/pulls

(If you don't want to use GitHub that's fine too, since you've submitted the .patch file)

comment:3 by AndywinXp, 2 years ago

Thanks! This looks good to me, I just need to check if there's the need to apply this somewhere else in the code

by dwatteau, 2 years ago

fsanitize=alignment warning leftovers with COMI

comment:4 by dwatteau, 2 years ago

Indeed, with the patch above I still see some warnings in COMI with a -fsanitize=alignment build with Clang++ 13.1 (SCUMM_NEED_ALIGNMENT also needs to be defined in config.h).

I've attached them in a .txt file above.

comment:5 by kreudom, 2 years ago

Thanks everyone, I will look through the leftover warnings from the log and then open a pull request on GitHub.

comment:7 by antoniou79, 2 years ago

This PR also fixes the issue that was brought to our attention recently (https://forums.scummvm.org/viewtopic.php?p=97397#p97397) on old Android devices (4.4.4), with arm cpu -- I was able to reproduce on an arm-v7a Huawei -- which had similar effect ; COMI would crash upon launching, and The Dig would crash just after a few frames into the intro.

Good work and thank you for the fix.

comment:8 by AndywinXp, 2 years ago

Owner: set to AndywinXp
Resolution: fixed
Status: newclosed
Note: See TracTickets for help on using tickets.