search

n10v / id3v2

🎵 ID3 decoding and encoding library for Go
https://pkg.go.dev/github.com/bogem/id3v2/v2
MIT License
334 stars 50 forks source link

Remove the spurious EOF test from parseFrames(). #43

Open AlexisWilke opened 4 years ago

AlexisWilke commented 4 years ago

I bumped in a bug where you read the APIC type from the br reader and return the final error which happens to be an EOF.

You correctly ignore the EOF just after the call to the APIC function, however, later on you would check that error again and if EOF you would return early on. The result being that you miss all the data after the APIC frame. I have a test which generates random ID3 tags to make sure my application works as expected and I bumped in this EOF problem because of this bug. The 10 frames after the APIC that I have in that file would be ignored by your reader.

The EOF test you have at the beginning of the loop are enough to make sure you exit on EOF. And the loop should otherwise be exited only once you read the number of bytes available in the ID3 and that's done very well with the for framesSize > 0 { line.

AlexisWilke commented 4 years ago

Here is a sample file with an ID3 where the APIC is midstream. There are 10 more frames after the APIC. Loading this file should give you 52 frames. With the original, I get 42 instead.

sample.zip

Remove that one EOF test fixes the problem.

n10v commented 4 years ago

Hi @AlexisWilke, thank you for your PR! Could you please also write a test(s) if it takes not so many time? That would be awesome :)

AlexisWilke commented 4 years ago

Okay, I see what I did wrong.

The fact is that you parse the contents of the APIC tag. If wrong, especially, when too small, we get an unexpected EOF. So the error is correct.

Now, my frame is otherwise correct. That is, there is another frame right after and the total size of the ID3 tag is correct. Only the APIC tag contents are wrong. I still think that if possible, it's a good idea to go on (which my fix allows us to do).

I've added a test which shows how the APIC fails. I put another frame just before and just after. We always get the one before. The one after will be missing if you do not apply my fix. The APIC is never included since it is considered invalid (at least by your library).

So, that's up to you whether you want to allow for continuation or not. It's less of a problem than I thought and since I can force the frames to include specific data, I could fix my test that way and now the APIC works as expected whether I have my fix applied or not.

AlexisWilke commented 4 years ago

Just in case, I wanted to add that you are actually treating that special EOF as a normal "break point" in the ID3 data. If you prefer to keep that "break point" functionality, then maybe it needs to be reported to the caller so the caller knows that something went wrong while reading the ID3 tag.

As it is, as the client, it looks like the ID3 was fully correct...