BACKENDS: IOS: MP3 playback failing, libmad writing out of bounds

[sluicebox]

I've ripped and encoded the King's Quest 6 end credits CD track to track1.mp3 but the IOS build crashes immediately upon playback. Windows and Mac builds work. I've tried several unrelated mp3s. Failure occurs during construction of MP3Stream due to libmad's mad_synth_frame() writing beyond its struct and corrupting adjacent values. This looks unrelated to the game or CD-audio context in which it's being used. I think any mp3 playback with MP3Stream would crash.

class BaseMP3Stream : public virtual AudioStream {
	...
	mad_synth _synth;
	uint _channels; // overwritten with 0 by mad_synth_frame()
	uint _rate;     // overwritten with 0 by mad_synth_frame()

I debugged this sequence:

1. MP3Stream constructor correctly sets _rate to 441000
2. MP3Stream constructor calls BaseMP3Stream::decodeMP3Data()
3. BaseMP3Stream::decodeMP3Data() calls mad_synth_frame(&_synth, &_frame) which overwrites _channels and _rate with zero
4. Audio::Timestamp fails an assertion because 0 is an illegal rate

I'm building with Xcode 12.4 (latest) on an M1 with macos 11.2.3 (latest) for an iphone 12 mini with ios 14.4.1. I'm using the instructions on the wiki and scummvm-ios7-libs-v2.zip. I tripped this a few months ago with earlier versions and now got around to debugging it.

To reproduce with attached KQ6 CD save:

1. Name any mp3 "track1.mp3" and put it in the game directory
2. Set "Text and speech" to Speech in ScummVM Audio options (Subtitles will trigger alternate credits without music)
3. Load the save and wait three seconds

I suspect that any MP3Stream usage from any engine will trigger this. I'm hoping something is just off about the prebuilt libmad. Happy to help test.

[criezy]

This is indeed a known issue with any game that use mp3. This was reported on the forum a few times, and I have seen the issue myself. However it looks like we never created a bug report for it. So thank you for doing it.

My conclusion were the same as yours, and I am indeed hoping that recompiling libmad for iOS will fix the issue. I just never go around to doing it. This is listed as a task in our iOS/Android Trello board.

Drascula is a good game to use for testing as it is available from our download page with a mp3 music addon available, and it crashes right at the start.

[sluicebox]

Oh very good, I'm glad it wasn't just me.

Thanks for keeping up the ios support, criezy. I love having Dagger of Amon-Ra on me at all times; it's a security blanket and genuinely hilarious.

There should be a competition to see who has the funniest organization codesigning their phone's ScummVM. =)

[sev-]

This would be nice to get fixed before the release.

[dwatteau]

I'm not sure what builds scummvm-ios7-libs-v2.zip, but the md_size.diff and length-check.patch patches for libmad in ​https://deb.debian.org/debian/pool/main/libm/libmad/libmad_0.15.1b-10.diff.gz could help, I believe.

[lephilousophe]

libmad.a is a fat MachO with 4 architectures: arm, arm64, x86, x86_64.
libmad.h is an include file with platform specific defines which seem to come from x86 build (note the FPM_INTEL define).

I suspect that the overflow comes from this discrepancy between the header used by ScummVM code to allocate the structure and the code included in libmad.a.

[dwatteau]

I don't know if scummvm-ios7-libs-v2.zip comes from an automated or a manual build, but something like this on top of the final mad.h file could maybe help:

--- mad.h.orig
+++ mad.h
@@ -24,7 +24,16 @@
 extern "C" {
 # endif
 
-# define FPM_INTEL
+/* XXX: Handle iOS Mach-O Universal libraries */
+# if defined(__aarch64__) || defined(__arm64__) || defined(__x86_64__)
+#  define FPM_64BIT
+# elif defined(__arm__)
+#  define FPM_ARM
+# elif defined(__i386__)
+#  define FPM_INTEL
+# else
+#  define FPM_DEFAULT
+# endif
 
 
 

As long as the same respective values are given to --enable-fpm= while building for each architecture, before the final call to lipo.

[larsamannen]

This is fixed in ScummVM 2.7.0 release build. This build contains an updated libmad archive.
If building the project yourself you can for now use the libmad.a and header file from
​https://github.com/larsamannen/scummvm-ios7-libs-v3

[carlo-bramini]

Perhaps, the SCUMMVM team may evaluate the chance to avoid using patched thirdy party libraries, by abandoning the obsolete libmad and migrate to libmpg123:

https://bugs.scummvm.org/ticket/13455

You may also find interesting a recent change to Audacity audio editor, which evaluated the benefits before having this enhancement in its master branch:

​https://github.com/audacity/audacity/issues/2236

[sluicebox]

Closing this as fixed, I can now KQ6 in peace. Thank you larsamannen!

carlo-bramini: I'm sure everyone on the team would be receptive to using a more maintained mp3 library. PRs welcome!

Take this ticket, for example. We knew what the problem was from the start, that we "just" needed to build a popular library correctly for this popular platform, and it still took two years. Because we "just" needed someone who had the skill, and had the time, and had the interest. In other words, the three hardest parts. =)

I don't know if you've taken a look at the ScummVM code that interacts with libmad, but to me, it's a nontrivial amount. And it's from 20 years ago. I would expect that rewriting all of that would require knowledge in both libraries, in addition to audio expertise. Also, the new library must work on every single platform that the old one does. All so that mp3 playback keep working with no perceptable difference. (No one will notice a theoretical playback quality difference of mp3s during adventure games.) Who would volunteer for that task? Sounds much harder than this, and this took two years. PRs welcome!

Also, if the standard is "avoid third party libraries that requires platform specific header patches"... I have some really bad news for you about C++... ​https://github.com/scummvm/dockerized-bb/tree/master/toolchains

I just want it clear that the obstacles to switching libraries don't include lack of consideration. Speaking for myself, it looks hard. I could be happily proven wrong with a PR! I'll check back on this ticket in a few years and see. =)