Invalid ID3 frame size message when frame size is correct

[psamuel]

I have ID3v2.3 and ID3v2.4 files that have the image as the last tag in the ID3v2.x header. These files were tagged using my own C program. I've checked, rechecked and checked again and my size values in the APIC tag are correct. What I'm seeing is the "Invalid ID3 frame size" message. The problem as I see it is on line 1101 of lib/image/Exiftool/ID3.pm (Version 12.09, same problem in 9.78)

       if (not defined $len or $offset + $len + 10 > $size) {

In my case the result of $offset + $len + 10 is indeed > $size as the value of $size is the value obtained from the ID3v2.x tags which is supposed to be 10 less than the true size to account for the ID3v2.x 10 byte header (assuming no footer). I'm proposing to replace the line with

       if (not defined $len or $offset + $len  > $size) {

but I'm not sure if that will break other things.

[psamuel]

After further analysis I think my fix is correct. The problem is that the total size of the ID3v2 tags ($size) AND the size of each of the tags ($len) is 10 less that reality because of the 10 byte header. However the value of $offset is the true offset into the file. So to ensure the current tag doesn't go beyond the total size of the tags, 10 must also be added to $size or the additional 10 be removed.

Original line 1101:
                if (not defined $len or $offset + $len + 10 > $size) {

New line 1101 (option 1)
                if (not defined $len or $offset + $len > $size) {

New line 1101 (option 2)
                if (not defined $len or $offset + $len + 10 > $size + 10) {

In addition the section that deals with ID3v2.2 tags, which have a 6 byte header, is also probably in error. Specifically line 1084.

Original line 1084
            last if $offset + 6 > $size;

New line 1084 (option 1)
            last if $offset > $size;

New line 1084 (option 2)
            last if $offset + 6> $size + 6;

[Phil Harvey]

Your fix isn't right for a couple of reasons, but I need to do some research into the ID3 specification to see what is correct.

The reasons are:

1.  The current code is reading ahead to the next ID, so it needs to be sure that at least 4 more bytes are available after the current frame or it could fail with a runtime error in the substr() function.

2. Line 1109 also adds 10 when testing.  This would need to be changed too.

The existing code would be fine if there was a terminating frame in the specification.  I need to find time to look into this.  Given that nobody has complained about this in 15 years, I would be surprised if this really needs to be changed.

- Phil

Edit:  I've taken a quick look at the specification and another look at the code.  The warning in question could only ever happen if the ID3 version is 2.4, and the frame length is > 127, and it is the last frame.  This combination would make it a very rare occurrence, so this could explain why nobody has complained until now.  Also, I see no mention of a terminating frame in the specification, but I don't have my ID3 sample files here right now and need to check them out before I make any changes.

[psamuel]

It is happening with ID3v2.4 and the frame length is greater than 127. In a subsequent test I added a TXXX tag after the final APIC tag and that seemed to work. So I might just add a dummy TXXX tag to my C code as a terminator tag. Or pad to the nearest power of two bytes with NULLs. I also converted the test file to ID3v2.3 and I don't see the error and converted it back to ID3v2.4 and I do see the error. So leave it with me and I'll fix it at my end.

I've been neck deep in understanding a lot of the ID3v2.x specs for a while now (at least the tags that interest me) so I can help with deciphering if needed. There is no concept of a terminating tag - that's what the initial size bytes are for so they don't need one.

PS I only came across this issue as I was using exiftool to double check my C code. It also updates tags, adds images, slices audio by time stamp or frame number etc. If anyone is interested let me know and I'll post a URL with the latest version for download.