SCUMM: Unaligned memory access in IMuseDigital

[kreudom]

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.

[dwatteau]

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)

[AndywinXp]

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

[dwatteau]

fsanitize=alignment warning leftovers with COMI

[dwatteau]

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.

[kreudom]

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

[kreudom]

Opened pull request here: ​https://github.com/scummvm/scummvm/pull/4209

[antoniou79]

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.