libsdl-org / SDL_mixer

An audio mixer that supports various file formats for Simple Directmedia Layer.
zlib License
433 stars 146 forks source link

Tag size seen as larger than file size error #476

Closed Starbuck5 closed 1 year ago

Starbuck5 commented 1 year ago

I'm one of the pygame contributors, and we use SDL_mixer for all the audio decoding.

One of our users came in with this message printed to stdout every time they loaded a certain file: INFO: parse_id3v2: Tag size bigger than actual file size

They went into a hex editor to investigate and found that this was not the case.

The tag size is ox00000036 AKA 54 ID3_tag_size_100wav

After the header there are in fact 54 bytes ID3_tag_size_met_100wav

According to the spec

The ID3v2 tag size is the size of the complete tag after unsychronisation, including padding, excluding the header but not excluding the extended header (total tag size - 10). Only 28 bits (representing up to 256MB) are used in the size description to avoid the introducuction of 'false syncsignals'.

The tag size should include the size of the extended header, I believe this is where the parser in SDL_mixer messes up. https://github.com/libsdl-org/SDL_mixer/blob/main/src/codecs/mp3utils.c#L487-L507

If it sees an extended header it skips over it. But the tag length (tag_len) includes the size of the extended header, so the parser thinks its going to run over the end.

Here's the file (zipped into a folder, GitHub wouldn't let me just drop the WAV in): 100.zip

sezero commented 1 year ago

So, the fix would be the following? (CC: @Wohlstand )

diff --git a/src/codecs/mp3utils.c b/src/codecs/mp3utils.c
index 830e96c..0ef9a1f 100644
--- a/src/codecs/mp3utils.c
+++ b/src/codecs/mp3utils.c
@@ -491,7 +491,6 @@ static SDL_bool parse_id3v2(Mix_MusicMetaTags *out_tags, struct mp3file_t *src)
     }

     if (tag_extended_len) {
-        total_length += tag_extended_len + 4;
         MP3_RWseek(src, tag_extended_len + 4, RW_SEEK_CUR); /* Skip extended header and it's size value */
     }
Wohlstand commented 1 year ago

I'll take a look today.

Wohlstand commented 1 year ago

I understand the problem, but, it's not good that your example lacks title/artist/album/copyright fields filled, and I am required to verify that these tags get parsed. Actually, I am trying to find any MP3 file in my collection that has an ID3 tag with an extended header...

Wohlstand commented 1 year ago

Okay, I actually fixed this bug, and I also found an MP3 file example with an extra header and title tag.

sezero commented 1 year ago

@Starbuck5: Does the patch at #477 fix your issue?

Starbuck5 commented 1 year ago

I looked at the patch logic and it seems like it will.

I’m not sure if I’ll be able to test later today, but I’ll try to test later today or sometime tomorrow.

Thanks for such a quick response and PR!

Starbuck5 commented 1 year ago

Yes, that fixes it.