Fix misc. crash and leak bugs found by libfuzzer.

[AliceLR]

I wrote a simple libfuzzer frontend to try to find bugs in #57. Here's a patch with fixes for all of the unrelated loader bugs I've found so far. This is the squashed version of the patches; the original commit history is available here: master...AliceLR:fuzz-patch-1-original

Test files/ASan output for test files/bad fuzzer setup:
fuzz_modplug_20210622.tar.gz. This expects to be placed in a folder called fuzz at the base of the local libmodplug repository.

Base input files were these folders of test modules:

• MegaZeux valid test modules..
• libxmp valid test modules.
• libxmp invalid fuzzer modules.
• An extra folder containing ModLand modules: 5 parsecs to eternity.dmf, another worlds.mt2, believe me.mt2, en mt2.mt2, fly by night.ams, ironwire.ams, luminous.ams, pure minds.ams, reality cave-in!.ams, state of confusion.dmf, and target em.ams, plus unrelated MIDIs --fc.mid and Crazy Bus Title Screen.mid.
• An extra folder containing Unreal Anthology modules: Guardian.umx, Neve.umx, Phantom.umx.

Misc. patch:

• AMF (DSMI): fix out-of-bounds reads caused by missing order list bounds checks.
• DBM: fix leaks caused by duplicate instrument chunks being loaded.
• FAR: fix out-of-bounds reads due to not correctly bounding the maximum pattern read size.
• IT: fix out-of-bounds reads in the IT sample decompressors caused by allowing ITReadBits to read past the end of the buffer.
• MED: fix out-of-bounds reads due to a faulty MMD2PLAYSEQ bounds check.
• MED: fix out-of-bounds reads due to bad sample bounding.
• MED: fix out-of-bounds reads due to bad block name bounding (and potential missing nul terminators).
• OKT: fix out-of-bounds reads due to incorrect OKTSAMPLE bounding.
• OKT: fix out-of-bounds reads due to bad chunk header and order list bounding.
• OKT: fix playback errors caused by skipping the first two orders in the order list.
• S3M: fix out-of-bounds reads due to missing order list bounds check.
• S3M: fix out-of-bounds reads due to missing offset list bounds check.
• S3M: fix out-of-bounds reads due to missing panning table check.
• STM: fix pattern leaks and pattern size corruption caused by missing MAX_PATTERNS check.
• ULT: fix out-of-bounds reads due to incorrect event bounding.
• WAV: fix out-of-bounds reads due to not bounds checking the fmt chunk.
• WAV: fix hangs caused by missing chunk length bounds check.
• WAV: constify pointers derived from lpStream.
• XM: fix out-of-bounds reads due to broken XMSAMPLEHEADER check.
• XM: fix out-of-bounds reads due to missing pattern data checks.
• XM: fix slow loads caused by bad bounding in instrument/sample loops, add other various missing bounds checks.

AMS:

• AMS: fix AMS out-of-bounds reads due to missing song comments checks.
• AMS: fix AMS out-of-bounds reads due to missing order list check.
• AMS: fix AMS out-of-bounds reads due to missing pattern/track checks.
• AMS: fix AMS2 out-of-bounds reads due to missing/broken instrument and envelope bounds checks.
• AMS: fix AMS2 out-of-bounds reads due to missing sample bounds checks.
• AMS: fix ReadSample out-of-bounds reads due to overflow in packed size bounds check.
• AMS: fix AMSUnpack out-of-bounds reads due to missing RLE unpacking bounds checks.
• AMS: reduce AMSUnpack slow loads by rejecting samples with truncated or invalid RLE.
• AMS: reduce AMSUnpack slow loads by shrinking samples if their packed size couldn't possibly store them.
• AMS: constify pointers derived from lpStream.

DMF:

• DMF: fix faulty bounds checks for INFO, SEQU, and SMPI chunks.
• DMF: add numerous missing bounds checks for patterns and track data.
• DMF: fix out-of-bounds reads caused by missing sample bounds check.
• DMF: fix hangs caused by duplicate PATT chunks.
• DMF: fix sample leaks caused by duplicate SMPD chunks.
• DMF: fix slow loads caused by missing EOF check in DMFUnpack.
• DMF: constify pointers derived from lpStream.

MDL:

• MDL: fix out-of-bounds reads due to missing info chunk bounds check.
• MDL: fix out-of-bounds reads due to a missing bounds check when loading instruments.
• MDL: fix out-of-bounds reads and other bugs due to bad envelope bounding and missing duplicate envelope chunk checks.
• MDL: fix out-of-bounds reads due to broken track bounds checks.

MT2:

• MT2: fix out-of-bounds reads due to missing nDrumDataLen check.
• MT2: fix out-of-bounds reads due to missing pattern/track checks.
• MT2: fix out-of-bounds reads due to broken/nonsensical instrument bounds checks.
• MT2: fix out-of-bounds reads due to missing sample data length bounds check.
• MT2: fix out-of-bounds reads due to bad checks on group structs.
• MT2: fix hangs caused by overflows preventing the data chunk size bounds check from working.
• MT2: constify pattern data pointer derived from lpStream.

PSM:

• PSM: fix out-of-bounds reads due to dereferencing lpStream before any bounds checks.
• PSM: fix out-of-bounds reads due to reading pPsmPat.data from the stack instead of the input buffer.
• PSM: fix out-of-bounds reads due to invalid samples in patterns.
• PSM: fix missing pattern length byte-swapping.
• PSM: constify pattern data pointer derived from lpStream.

MMCMP:

• MMCMP: fix out-of-bounds reads due to broken initial subblock bounds check.
• MMCMP: reduce slow loads and potential bugs caused by broken and redundant subblock packing bounds checks.

MIDI:

• MIDI: fix stack corruption caused by using sprintf on an undersized buffer.
• MIDI: fix out-of-bounds reads caused by no bounds checks being performed on the mmread* and mid_read* functions.
• MIDI: fix NULL dereferences when attempting to reuse tracks containing no events.
• MIDI: fix leaks and hangs caused by not freeing MIDI structs or clearing the reentry flag when m_nChannels is 0.
• MIDI: fix leaks caused by not freeing MIDI structs when PAT_Load_Instruments fails.
• MIDI: fix leaks caused by not freeing MIDTRACKs.
• PAT: fix out-of-bounds reads caused by invalid GM patches.

[AliceLR]

• Fix out-of-bounds stack write in the MIDI loader caused by using sprintf instead of snprintf for a debug message on an insufficiently-sized buffer.

[AliceLR]

• Fix out-of-bounds reads in the PAT loader that can happen when invalid GM patches somehow get into the GM mapping table.
• Fix NULL dereferences in the MIDI loader when attempting to reuse tracks containing no events.

edit: due to the numerous slow loads/DoS issues in the MIDI loader I've just turned off the ABC/MID/PAT loaders for the purposes of my fuzzing runs right now. These should be fixed but it would likely be nontrivial and it is interfering with my ability to test the other loaders (execs/s dropped to ~10; some units running for up to 45 seconds!).

[AliceLR]

• Fixed potential crashes in the PSM loader due to not byte-swapping the pattern length values during pattern loading.
• Fixed crashes in the PSM loader due to attempting to read pattern data from the pPsmPat.data array on the stack instead of from the source bytes.

[AliceLR]

Since I'm still finding more things I'm going to just make this a draft for now. If it seems like it's not finding new bugs anymore I'll squash this and mark it ready for review.

• Fix out-of-bounds reads in the PSM loader due to not ignoring invalid sample values.
• Fix out-of-bounds reads in the MDL loader due to a missing bounds check when loading instruments
• Fix out-of-bounds reads in the MDL loader due to multiple broken track bounds checks.
• Fix leaks in the DBM loader caused by duplicate instrument chunks being loaded.
• Fix out-of-bounds reads in the MT2 loader due to missing nDrumDataLen bounds check.
• Fix out-of-bounds reads in the MT2 loader due to broken/nonsensical instrument bounds checks.
• Fix out-of-bounds reads in the MT2 loader due to a broken bounds check on group structs.

[sezero]

mmcmp.c in libmikmod and libxmp might need revisiting, too.

[AliceLR]

Both look good to me. MikMod uses a sane file IO abstraction that shouldn't have the same problem this did. libxmp's bounds checks are built into the fread calls it makes to load the blocks/sub blocks.

[AliceLR]

I preemptively checked MikMod and libxmp for the MMCMP bounds check cleanup I just pushed, and it looks like neither are affected by the bizarre/redundant bounds checking that libmodplug had in place either (MikMod checks writes against an end pointer rather than bounding unpk_size; libxmp writes to stdio).

[sezero]

Thanks for checking them. I remember having tried to minimize load times in libmikmod but our versions diverged and libmodplug seems to have lost interest so I haven't submitted any more patches for it.

(I pinged million times for my patches here, also sent private email, no result. Not sure what you will get with this patchset, but I will merge it in my fork and to SDL_mixer and SDL_sound's own sources.)

[AliceLR]

OK. Fuzzing has slowed down significantly at finding new bugs, so I'll probably squash/reorganize this patch soon so it's ready to be applied elsewhere. Since there have been so many changes, I think I'll do a commit for each loader instead of a single commit.

[AliceLR]

It got to the point where MMCMP was also significantly dropping execs so I've turned it off (it seems to be correctly bounded now anyway). I'll let this run for a little bit with just the non-MIDI loaders and then get to finally squashing these commits and verifying these changes didn't break any legitimate files.

[AliceLR]

Due to the Oktalyzer order list bounding being fairly closely connected to the Oktalyzer order list bug I fixed in #53, I've imported that fix into this branch. In my initial commit I accidentally bounded it wrong because I forgot about that bug.

[AliceLR]

DMF problems keep popping up just when I think this is done. I think I've had to add bounds/EOF checks to just about every place in the DMF loader so maybe it will stop happening now.

edit: I finally added some test AMS/MT2 files and I'm glad I did, because I'm finding more issues. Sorry for the delay... : )

edit: libmodplug crashes when given a valid Velvet Studio AMS... 🤨

[sezero]

I applied this locally to support older MSVC and other toolchains, to be safe.
(Also attaching as a patch file: patch.txt)

diff --git a/src/load_mid.cpp b/src/load_mid.cpp
index 4a93322..caf5b2c 100644
--- a/src/load_mid.cpp
+++ b/src/load_mid.cpp
@@ -1125,6 +1125,12 @@ static void mid_notes_to_percussion(MIDTRACK *tp, ULONG adjust, ULONG tmin)
 	if( lno && lno->next ) mid_stripoff(tp, lno);
 }
 
+#if (defined(_MSC_VER) && _MSC_VER < 1900) || defined(__WACTOMC__) || \
+    (defined(__MINGW32__) && !(defined(__USE_MINGW_ANSI_STDIO) && __USE_MINGW_ANSI_STDIO != 0))
+#define BAD_SNPRINTF
+#undef snprintf
+#define snprintf _snprintf
+#endif
 static void mid_prog_to_notes(MIDTRACK *tp, ULONG adjust, ULONG tmin)
 {
 	MIDEVENT *e, *lno = 0;
@@ -1174,6 +1180,9 @@ static void mid_prog_to_notes(MIDTRACK *tp, ULONG adjust, ULONG tmin)
 	if( ton > toff ) {
 		char info[128];
 		snprintf(info,sizeof(info),"channel %d, %ld > %ld note %d", tp->chan + 1, (long)ton, (long)toff, n);
+		#ifdef BAD_SNPRINTF
+		info[sizeof(info)-1] = '\0';
+		#endif
 		mid_message("melody track ends with note on (%s)", info);
 	}
 	if( lno && lno->next ) mid_stripoff(tp, lno);

[AliceLR]

I think I'd be OK with just unconditionally adding that nul (I usually do, but forgot it here).

[AliceLR]

Actually, since the extra define would still need to be done with snprintf, this could probably just be an sprintf (the buffer should be more than large enough now).

[sezero]

Even better, thanks.

[sezero]

How much work is there left with this?

I plan to apply the gdm patch to my fork, and to SDL_sound and SDL_mixer tomorrow,
I can apply this one too, if it's ready.

[AliceLR]

I don't know how much work is left here. I'm still getting infrequent ASan crashes, and these changes also haven't been verified very thoroughly against real modules, so I don't think this is ready to apply yet.

[sezero]

so I don't think this is ready to apply yet.

OK, will wait.

[sezero]

I merged these to the C-conversion in SDL_sound in a temporary branch,
here: sezero/SDL_sound@master...pr58
Can you please have look to see if there are any gotchas?. (Particular
differences are in load_it.c::ITReadBits() load_psm.c.)

[AliceLR]

libFuzzer has been very stable since the WAV hang fix, and I've aggregated what should be a full list of fixes from this branch. Some of the simpler patches don't really need extra verification, but I still need to sift through the rest and then do testing.

Can you please have look to see if there are any gotchas?. (Particular
differences are in load_it.c::ITReadBits() load_psm.c.)

The ITReadBits change looks correct to me (I'm not really a fan of what the original does... reference to a pointer hidden in a Win32 typedef 🥴). The other IT tweaks look accurate to my patch, too. I'm not 100% sure all of the ITUnpack* bounding I changed to reference pStop is correct, but it should be exactly equivalent to what was there before (so not a regression).

The load_psm.c changes also look good to me. I'll check the rest once I'm sure everything is fine on my end too (and sorry for dragging this out).

[AliceLR]

I've gone through the list of changes and am more convinced that my fixes are OK. Doing a last bit more testing before I squash everything. The main compatibility spots I wasn't sure about were the DMF and AMS sample packing slow load fixes, but I wrote a test program for these and aside from a small number of AMS2 modules that already have -major issues- with their samples loading in libmodplug (MPT 1.16 loads the affected samples as garbage), none of the modules I have actually flagged debug messages for those checks.

[AliceLR]

Commits are now squashed. @sezero, please make sure commits
70d73b9, f768b60, and 4317cb7 from the original commit history (master...AliceLR:fuzz-patch-1-original) are also applied to your version of the patch. In the meantime, I'll work on verifying the rest of your patch against mine...

edit: also, the new commit messages contain better summaries of the fixes than the old commit messages, so those might be worth pulling in too.

[Konstanty]

Nice work guys! Especially on the MIDI/PAT etc work - as this was a highly neglected area of libmodplug.

Please note that for the non MIDI/PAT changes - some of these fixes were performed on oob_read_fixes branches. This was based on several weeks of fuzzing identifying the errors.

[sezero]

Commits are now squashed. @sezero, please make sure commits
70d73b9, f768b60, and 4317cb7 from the original commit history (master...AliceLR:fuzz-patch-1-original) are also applied to your version of the patch. In the meantime, I'll work on verifying the rest of your patch against mine...

edit: also, the new commit messages contain better summaries of the fixes than the old commit messages, so those might be worth pulling in too.

Changes seem identical in my repos, thanks! When you say it's ready, I'll apply with your commit messages.

[AliceLR]

Nice work guys! Especially on the MIDI/PAT etc work - as this was a highly neglected area of libmodplug.

Please note that for the non MIDI/PAT changes - some of these fixes were performed on oob_read_fixes branches. This was based on several weeks of fuzzing identifying the errors.

That's good to know, I wasn't aware of that branch—would you prefer that I omit changes included in that branch, or are my implementations here OK?

Changes seem identical in my repos, thanks! When you say it's ready, I'll apply with your commit messages.

Still haven't finished checking, sorry. Will give your commit a glance and follow up ASAP.

[AliceLR]

@sezero I found the following issues in your patch (using sezero/SDL_sound@master...pr58 as a reference... sorry if this is out of date):

• AMS: commits 70d73b9 and f768b60 are still missing.
• DMF: commit 4317cb7 is still missing.
• MT2: the check at 551 in my patch is missing. This might have been mistaken for a bounding check related to the removed pms->szName memcpy, but it isn't—it makes sure adding pms->dwDataLen to dwMemPos will not overflow.

Otherwise, your branch looks good to me!

[sezero]

@sezero I found the following issues in your patch (using sezero/SDL_sound@master...pr58 as a reference... sorry if this is out of date):

* AMS: commits [70d73b9](https://github.com/Konstanty/libmodplug/commit/70d73b9f49671b8f85c7a664ac6b5a322d7df4c0) and [f768b60](https://github.com/Konstanty/libmodplug/commit/f768b60a79c3bad2c77f69261fb1c34a59f921b1) are still missing.

* DMF:  commit [4317cb7](https://github.com/Konstanty/libmodplug/commit/4317cb7de92a6695d171ae0c95053dd7b2a7bf4c) is still missing.

* MT2: the check at 551 in my patch is missing. This might have been mistaken for a bounding check related to the removed `pms->szName` `memcpy`, but it isn't—it makes sure adding `pms->dwDataLen` to `dwMemPos` will not overflow.

Just force-pushed in 6 batches (like you did, and with your commit messages),
the missing parts should be added now: If you ever have time, please confirm.

[AliceLR]

Just force-pushed in 6 batches (like you did, and with your commit messages),
the missing parts should be added now: If you ever have time, please confirm.

Looks good now, thanks!

[sezero]

This patchset is now in SDL_sound, SDL_mixer, and also in my fork of libmodplug.

[sagamusix]

edit: libmodplug crashes when given a valid Velvet Studio AMS... 🤨

Yup, it was disabled in OpenMPT SVN r688 for that reason. I cannot tell you exactly what was causing it because it was essentially replaced by a full rewrite in r1303.

[debrouxl]

Good to see further activity in this area :)
Too bad some of the already fixed issues were re-discovered - and the corresponding fixes re-developed - because the existing fixes made two years ago after my fuzzing runs (using an older version of the libxmp testcases as initial corpus) were on a non-default branch, though. For another project whose fuzzing is currently pegging my cores, I had a look at the list of Git branches before starting the fuzzing process.

libmodplug could arguably be popular enough to qualify for OSS-Fuzz integration, unlike libxmp.

[debrouxl]

Ping @Konstanty ? :)

[AliceLR]

Is there anything I need to change here so this can be merged?

#64 (comment)

[AliceLR]

I compared this branch against master and it looks like nothing here went missing or broke between merging #66 and #67. Closing, thanks!