Closed sezero closed 5 years ago
Thanks for the note, and also I would to do same for libid3tag I have modded to equip it with SDL_RWops support to integrate it with my SDL Mixer X.
(Note that library is used not only for meta-tags, but as a way to avoid some silly behavior of libMAD where it does interpretation of ID3 part as encoded MP3 chunk. In result, something weird may happen: getring wrong samplerate, and later - crash).
RE: libid3tag/libmad/crash: Do you have a case (a mp3 file maybe) where this crash happens with SDL_mixer built with libmad support?
Yeah, I have some MP3s where I getting weird result and later crash when running without of libid3tag, I have them at my home, and I'll send it to you at evening...
The main issue is that libMAD never skips ID3 tags and parses them like there are proper stream. In result, it gets some junk, like totally wrong sample rate value and other things are causing in-memory crap which later leading a crash. So, to avoid any possible junk, you must identify the ID3 and it's full length, then pass the proper start position, NOT at zero: https://github.com/WohlSoft/SDL-Mixer-X/blob/master/src/codecs/music_mad.c#L210
I have been using libmad as adapted from the sox library, and I skip tags like this, if you're interested (search for mp3_inputtag in the source): https://github.com/svn2github/uhexen2/blob/master/engine/h2shared/snd_mp3.c#L142
If you send the your problematic mp3 files, I can test them with both SDL_mixer and also with uhexen2 or quakespasm to decide a best solution.
mp3_inputtag you have shown is a good thing to put it here: https://github.com/WohlSoft/SDL-Mixer-X/blob/master/src/codecs/music_id3tag.c for the case when libid3tag support was disabled to be able skip ID3 tag even without of libid3. However, the libid3 is used at me especially for tags as SDL Mixer X introduces ability to know about of them on API level (music title, artist, album, and copyright notice).
@sezero , I was dig out the example that will guarantee crash on the startup (or it even play some time and will crash on attempt to stop and begin another song).
Feel free to test it out :fox_face: :wink: bad-mp3.zip
Also, about of libMAD, we with @jpcima have discussed about it and he said about of CVE candidat, but, looks it's already has CVE: https://www.cvedetails.com/cve/CVE-2017-8373/
Anyway, since libtimidity and libid3tag was renamed into "libtimidity-sdl" and "libid3tag-sdl", the issue has been solved.
Created https://github.com/WohlSoft/AudioCodecs/pull/7 for libmad CVE-2017-8372 and CVE-2017-8373
Took!
As for bad.mp3: thanks.
I couldn't make it to crash, but noticed that SDL_mixer plays it with a slower tempo than it should, possibly because it took a tag for a legitimate frame. Will work on it.
Did you have used CVE-patched libMAD on your end or pure? At me was a pure version, and the crash will happen. I'll try to test some non-MP3 junk files (some unknown files are identifying as MP3 and playing by libMAD as dirty noise, and later may crash) to confirm will crash happen or not...
Did you have used CVE-patched libMAD on your end or pure?
I tried both, couldn't make it crash (CentOS 6.10 / i686)
The crash will happen on SDL Mixer only when you'll switch songs, but while playing, will work "fine", try to play and switch different songs between each other include this bad mp3. At me is x86_64, the crash happens also on Windows 32-bit builds builds on x86_64. But possibly, will crash here too.
I filed this at SDL bugzilla: https://bugzilla.libsdl.org/show_bug.cgi?id=4295
The one you have here (copy from SDL2_mixer) is not libtimidity. This is libtimidity: https://sourceforge.net/projects/libtimidity/ (or the github mirror: https://github.com/sezero/libtimidity.git)
I suggest renaming yours to something else like timidity-sdl, etc.