double freed pMP3->pData (solved: tricky user error)

[mgambrell]

Using code I just pulled from github, we see line 2834 frees pMP3->pData but doesn't null the pointer. this pointer is also freed in drmp3_uninit. it seems like this pointer should be nulled (and so too the dataCapacity needs to be set to 0) but................... why not keep it all around? it has to be disposed in uninit, and the user has to call that. so can't we just save it until then? I don't understand the rationale for trying to free it there when uninit can do the job. Maybe it's that the mp3 stream is deemed dead until some action happens to reset it? Seems complicated. Fixing it either way works for me.

[mgambrell]

OK, I have traced this more to realize the init process failed. I guess i shouldnt free something that wasnt successfully initialized. my bad. as for why the init process failed.. this is surprising me. I'm gonna leave this ticket open til i find out why

[mgambrell]

This was due to a 7 frame mp3 getting tripped by DRMP3_MAX_FRAME_SYNC_MATCHES which defaults to 10. Thank you for making it configurable. I would suggest adding this to "Build Options" in the docs (along with the compile time sample rate and anything else I may have missed). I hate when people tell me to write docs too. Sorry. I'll leave this open so you're sure to see my story. Thanks for dr_mp3!

[mackron]

That option is actually from minimp3 which dr_mp3 wraps around - I don't know what that option actually does specifically. What did you need to change it to to make it work?

There's no notion of a compile-time defined sample rate. The sample rate is assumed to be whatever sample rate is used by the first MP3 frame (MP3 streams with inconsistent sample rates are not supported).

[mgambrell]

I saw DRMP3_DEFAULT_SAMPLE_RATE but now I see it isn't really used.

What I changed to make it work was change DRMP3_MAX_FRAME_SYNC_MATCHES to 1.

What happens is the library tries to determine if the data is really an MP3. In order to do this it looks for DRMP3_MAX_FRAME_SYNC_MATCHES valid frames. But this can't succeed if there arent even that many frames.

A better solution for this would be to keep DRMP3_MAX_FRAME_SYNC_MATCHES at 10 but change drmp3d_match_frame() to give up with success if the data is exhausted precisely. However keep in mind the user would have to take care to crop weird tag data from the end of the stream which would get picked up as invalid frames.

[mackron]

Ah, right. I think DRMP3_DEFAULT_SAMPLE_RATE and DRMP3_DEFAULT_CHANNELS are leftovers from some previous APIs which I've since removed. Certainly they were for dr_mp3 and not anything to do with minimp3. I'll remove those - thanks for pointing that out.

@lieff Do you have any opinion on the drmp3d_match_frame() / DRMP3_MAX_FRAME_SYNC_MATCHES thing? I'd rather keep dr_mp3 consistent with minimp3, but I'm just not sure on the implications of changing the behaviour of that function.

[lieff]

Hi) About DRMP3_MAX_FRAME_SYNC_MATCHES: mp3 sync points are very weak (much weaker than MPEG`s ones for example) and false positives can occurs. To make search procedure stronger, MAX_FRAME_SYNC_MATCHES frames is checked, not much more can be done here. So it can be lowered, but I do not recommend set it to 1 in general case.

Files with less frames than DRMP3_MAX_FRAME_SYNC_MATCHES should be handled just fine (at least with minimp3_ex and there tests for such files). But successful removal of id3v1 and APE tags is critical to make such files to work.

Currently minimp3_ex have issue only with BIG APE tags (can be up to 4gb) and callbacks mode, because without seek to the end there problem to remove it in streaming manner. But this is special case and I've plan to fix it.

If there other garbage at the end of file, then file treated as damaged and such problem expected with such short file. And there no good solution in general. We need sync procedure, and there cases which fail with weak sync procedure as well.

@mgambrell Can you attach problem mp3 file? I can look at it.

[kcgen]

Pinging @mgambrell 😅 regarding request above. (I also have an interest in seeing this issue addressed)

[mackron]

I'm following the advice of @lieff because I just don't have enough knowledge of MP3 to make a qualified decision. I mean, it would make sense if it would just work, but at the very least we need a sample file to use as a test vector.

[mgambrell]

I was mistaken in my last post.

So, I have no idea what file was originally tripping this situation for me. But I did my best to guess it and encountered mainly tiny ( <10 frame) mp3 files WITH tags at the end. So the problem is not "it is classified as not-an-mp3 if there are fewer than 10 frames" as I originally said, but rather "it is classified as not-an-mp3 if there are less than 10 frames and the final frame is a TAG."

I restored the 10-frames value for checking and used this code to pass my "test suite" of trashy tiny mp3s that came to me. I consider this an improvement (plus or a minus a static assert to ensure DRMP3_HDR_SIZE >= 3) but I wouldn't warrant that any one change is a silver bullet to solve a problem whose solution can really only be approximated anyway.

As I wrote in a comment, I think in general the algorithm is improved if tags are aggressively identified and used to imply the end of the stream. There may be other tag formats that can go at the end of a file (like "ID3") so catching those here would be good too.

I'm attaching a file 74.mp3 that dr_mp3 currently declines to load but which is fixed by my patch below.

static int drmp3d_match_frame(const drmp3_uint8 *hdr, int mp3_bytes, int frame_bytes)
{
    int i, nmatch;
    for (i = 0, nmatch = 0; nmatch < DRMP3_MAX_FRAME_SYNC_MATCHES; nmatch++)
    {
        i += drmp3_hdr_frame_bytes(hdr + i, frame_bytes) + drmp3_hdr_padding(hdr + i);
        //if we're at or beyond the end, consider it a match if we've found even one good frame
        if (i + DRMP3_HDR_SIZE > mp3_bytes)
            return nmatch > 0;
        //now we know we have 4 bytes so we can check for 3 bytes of a "TAG" here
        //we consider "TAG" kind of like the end of the stream.
        if(!memcmp(hdr + i,"TAG",3))
          return nmatch > 0;
        //now see if this is a valid-looking mp3 header
        if (!drmp3_hdr_compare(hdr, hdr + i))
            return 0;
    }
    return 1;
}

74.zip

[lieff]

So this is tag removal issue I mention above. This is tricky part to handle since mp3 can have:

• id3v1 tag
• id3v2 tag
• APEv2 tag
• VBR tag

VBR tag can also be useful to decode process. 74.mp3 have id3v2 and id3v1 tags, minimp3_ex handles it without issues:

ffmpeg -i 74.mp3 -f s16le -acodec pcm_s16le out_ref.raw
minimp3 74.mp3 out_ref.raw out.raw
rate=44100 samples=11520 max_diff=1 PSNR=120.131446

and length is identical.

I see currently dr_mp3.h do not have tag removal code which needed to handle such files.

[mackron]

OK, lets leave this one with me and I'll sort it out when I get a chance. Will mark this as a bug because I think it should "Just Work". I've got a feature request to support tags as well so I'll just do a full featured tag parsing system I guess. No time estimate on this. Thanks @lieff!