STM loader should ignore volume=0 samples for certain .STMs. #6
Comments
The only reference I found about load_stm.c history is this: |
|
I checked those versions and that does appear to be when that was changed: There's a second bug that the libxmp PR linked above also fixes, which is the sample positions are determined by the sample reserved field *16. For whatever reason the volume=0 hack alone seems to break fracture.stm and this fix is required to get the fracture.stm samples to load correctly. (edit: both libmodplug and libopenmpt also have this fix.) Here's fracture.stm (also available in the libxmp repo) for convenience. Working patch that allows MikMod to load and play both georythm.stm and fracture.stm correctly. I think the MOD2STM files and mysterious "some STM modules" that didn't have their samples loaded correctly deserve further investigation (though not using the reserved field might explain the latter). diff --git a/libmikmod/loaders/load_stm.c b/libmikmod/loaders/load_stm.c
index 406bea8..c97dda9 100644
--- a/libmikmod/loaders/load_stm.c
+++ b/libmikmod/loaders/load_stm.c
@@ -255,7 +255,6 @@ static BOOL STM_LoadPatterns(void)
static BOOL STM_Load(BOOL curious)
{
int t;
- ULONG MikMod_ISA; /* We must generate our own ISA, it's not stored in stm */
SAMPLE *q;
/* try to read stm header */
@@ -331,8 +330,6 @@ static BOOL STM_Load(BOOL curious)
if(!AllocSamples()) return 0;
if(!STM_LoadPatterns()) return 0;
- MikMod_ISA=_mm_ftell(modreader);
- MikMod_ISA=(MikMod_ISA+15)&0xfffffff0; /* normalize */
for(q=of.samples,t=0;t<of.numsmp;t++,q++) {
/* load sample info */
@@ -340,13 +337,10 @@ static BOOL STM_Load(BOOL curious)
q->speed = (mh->sample[t].c2spd * 8363) / 8448;
q->volume = mh->sample[t].volume;
q->length = mh->sample[t].length;
- if (/*!mh->sample[t].volume || */q->length==1) q->length=0;
+ if (!mh->sample[t].volume || q->length==1) q->length=0;
q->loopstart = mh->sample[t].loopbeg;
q->loopend = mh->sample[t].loopend;
- q->seekpos = MikMod_ISA;
-
- MikMod_ISA+=q->length;
- MikMod_ISA=(MikMod_ISA+15)&0xfffffff0; /* normalize */
+ q->seekpos = mh->sample[t].reserved << 4;
/* contrary to the STM specs, sample data is signed */
q->flags = SF_SIGNED;edit: from the same diff I uploaded above, the MOD2STM comment in the changelog might just be a reference to adding support for the |
|
Only briefly read your patch, yet. Is it well-tested anything other than those you noted above? |
|
Note: |
|
So far I've only tested that patch against a small handful of files so it definitely needs more testing.
Here's an updated patch that adds a bounds check. (edit: the check is specifically limited to q->length being set since otherwise this was incorrectly flagging some dummied samples) diff --git a/libmikmod/loaders/load_stm.c b/libmikmod/loaders/load_stm.c
index 406bea8..5e402d0 100644
--- a/libmikmod/loaders/load_stm.c
+++ b/libmikmod/loaders/load_stm.c
@@ -255,7 +255,8 @@ static BOOL STM_LoadPatterns(void)
static BOOL STM_Load(BOOL curious)
{
int t;
- ULONG MikMod_ISA; /* We must generate our own ISA, it's not stored in stm */
+ ULONG samplestart;
+ ULONG sampleend;
SAMPLE *q;
/* try to read stm header */
@@ -331,8 +332,10 @@ static BOOL STM_Load(BOOL curious)
if(!AllocSamples()) return 0;
if(!STM_LoadPatterns()) return 0;
- MikMod_ISA=_mm_ftell(modreader);
- MikMod_ISA=(MikMod_ISA+15)&0xfffffff0; /* normalize */
+
+ samplestart=_mm_ftell(modreader);
+ _mm_fseek(modreader,0,SEEK_END);
+ sampleend=_mm_ftell(modreader);
for(q=of.samples,t=0;t<of.numsmp;t++,q++) {
/* load sample info */
@@ -340,13 +343,16 @@ static BOOL STM_Load(BOOL curious)
q->speed = (mh->sample[t].c2spd * 8363) / 8448;
q->volume = mh->sample[t].volume;
q->length = mh->sample[t].length;
- if (/*!mh->sample[t].volume || */q->length==1) q->length=0;
+ if (!mh->sample[t].volume || q->length==1) q->length=0;
q->loopstart = mh->sample[t].loopbeg;
q->loopend = mh->sample[t].loopend;
- q->seekpos = MikMod_ISA;
+ q->seekpos = mh->sample[t].reserved << 4;
- MikMod_ISA+=q->length;
- MikMod_ISA=(MikMod_ISA+15)&0xfffffff0; /* normalize */
+ /* Sanity check to make sure samples are bounded within the file. */
+ if(q->length && (q->seekpos<samplestart || (q->seekpos+q->length)>sampleend)) {
+ _mm_errno = MMERR_LOADING_SAMPLEINFO;
+ return 0;
+ }
/* contrary to the STM specs, sample data is signed */
q->flags = SF_SIGNED; |
|
I've done a bit more testing on this patch. I found two unrelated bugs that I can diagnose and report as a separate issue. As far as this patch goes, the sample bounds check is maybe too aggressive but otherwise every .STM I throw at MikMod with this patch loads and has correct samples. [1]: every STM loaded seems to have a buggy extra order appended to the end of the orders list (off-by-one?). Order number is noted for most occurences. works=indistinguishable from ST 2.3 (DOSBox, SB16, 25k) playback as far as I can tell.
I also went though every .STM on modland.com (since there's a reasonably low number of them) and tested every track that MikMod 3.3.11 fails to load. It might be worth allowing some degree of sample truncation since a couple of these seem to rely on it to load.
|
|
Here's an updated patch that allows sample truncation for the two truncated .STMs in the previous comment. Not really sure about setting seekpos to 0 though. diff --git a/libmikmod/loaders/load_stm.c b/libmikmod/loaders/load_stm.c
index 406bea8..b4772dc 100644
--- a/libmikmod/loaders/load_stm.c
+++ b/libmikmod/loaders/load_stm.c
@@ -255,7 +255,8 @@ static BOOL STM_LoadPatterns(void)
static BOOL STM_Load(BOOL curious)
{
int t;
- ULONG MikMod_ISA; /* We must generate our own ISA, it's not stored in stm */
+ ULONG samplestart;
+ ULONG sampleend;
SAMPLE *q;
/* try to read stm header */
@@ -331,8 +332,10 @@ static BOOL STM_Load(BOOL curious)
if(!AllocSamples()) return 0;
if(!STM_LoadPatterns()) return 0;
- MikMod_ISA=_mm_ftell(modreader);
- MikMod_ISA=(MikMod_ISA+15)&0xfffffff0; /* normalize */
+
+ samplestart=_mm_ftell(modreader);
+ _mm_fseek(modreader,0,SEEK_END);
+ sampleend=_mm_ftell(modreader);
for(q=of.samples,t=0;t<of.numsmp;t++,q++) {
/* load sample info */
@@ -340,13 +343,35 @@ static BOOL STM_Load(BOOL curious)
q->speed = (mh->sample[t].c2spd * 8363) / 8448;
q->volume = mh->sample[t].volume;
q->length = mh->sample[t].length;
- if (/*!mh->sample[t].volume || */q->length==1) q->length=0;
+ if (!mh->sample[t].volume || q->length==1) q->length=0;
q->loopstart = mh->sample[t].loopbeg;
q->loopend = mh->sample[t].loopend;
- q->seekpos = MikMod_ISA;
+ q->seekpos = mh->sample[t].reserved << 4;
- MikMod_ISA+=q->length;
- MikMod_ISA=(MikMod_ISA+15)&0xfffffff0; /* normalize */
+ /* Sanity checks to make sure samples are bounded within the file. */
+ if(q->length) {
+ if(q->seekpos<samplestart) {
+#ifdef MIKMOD_DEBUG
+ fprintf(stderr,"rejected sample # %d (seekpos=%u < samplestart=%u)\n",t,q->seekpos,samplestart);
+#endif
+ _mm_errno = MMERR_LOADING_SAMPLEINFO;
+ return 0;
+ }
+ /* Some .STMs seem to rely on allowing truncated samples... */
+ if(q->seekpos>=sampleend) {
+#ifdef MIKMOD_DEBUG
+ fprintf(stderr,"truncating sample # %d from length %u to 0\n",t,q->length);
+#endif
+ q->seekpos = q->length = 0;
+ } else if(q->seekpos+q->length>sampleend) {
+#ifdef MIKMOD_DEBUG
+ fprintf(stderr,"truncating sample # %d from length %u to %u\n",t,q->length,sampleend - q->seekpos);
+#endif
+ q->length = sampleend - q->seekpos;
+ }
+ }
+ else
+ q->seekpos = 0;
/* contrary to the STM specs, sample data is signed */
q->flags = SF_SIGNED; |
Certain .STM files rely on dummying out samples with 0 volume. There's a check for this in load_stm.c, but it's commented out. I'm not sure why this is the case, but here's a .STM from Mod Archive that definitely relies on this behavior to work correctly.
georythm.stm.zip
Other players' behaviors: libxmp has the same bug but also allows the mod to load, resulting in corrupted samples that sound awful. libmodplug unconditionally dummies these samples out and the file plays correctly. libopenmpt plays the file correctly and does effectively the same check, but in a different location.
The text was updated successfully, but these errors were encountered: