Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ID3 metadata COMM frame with size<4 decoding error #2663

Closed
ZuZuK opened this issue Apr 9, 2017 · 1 comment
Closed

ID3 metadata COMM frame with size<4 decoding error #2663

ZuZuK opened this issue Apr 9, 2017 · 1 comment
Labels

Comments

@ZuZuK
Copy link

ZuZuK commented Apr 9, 2017

Issue description

I found some *.mp3 tracks where ID3 metadata contains COMM frame with frameSize=1. In that case Id3Decoder throws NegativeArraySizeException on parsing such frame.
As I understand it's not illegal to set such data into COMM so player can just skip such frame as it's frameSize is only 1 byte. Other players have no problems with playing such files.

Reproduction steps

Just play track with such metadata

Link to test content

https://www.dropbox.com/s/qdsxgi8s462jql9/BAD_COMM_SAMPLE.mp3?dl=0

Version of ExoPlayer being used

2.3.1

Device(s) and version(s) of Android being used

Any device. I've tested on Emulator 6.0 and on Google Pixel 7.1.2

A full bug report captured from the device

Exo code problem and possible fix

This bug occurs because according to ID3 specification COMM frame should contains encoding byte and then language iso (3 bytes). So ID3Decoder (in method decodeCommentFrame) gets byte for encoding, then gets 3 bytes for language iso even if frameSize is less than 4 bytes. Then it is creating byte-array with size=frameSize-4. Then it crashes obviously.

My possible fix is to modify COMM frame decoding condition into ID3Decoder.decodeFrame

from

else if (frameId0 == 'C' && frameId1 == 'O' && frameId2 == 'M'
          && (frameId3 == 'M' || majorVersion == 2)) {
        frame = decodeCommentFrame(id3Data, frameSize);
      }

to

else if (frameId0 == 'C' && frameId1 == 'O' && frameId2 == 'M'
          && (frameId3 == 'M' || majorVersion == 2) && frameSize > 4) {
        frame = decodeCommentFrame(id3Data, frameSize);
      }

but maybe it's better to modify CommentFrame to contains some default language and empty content for example. don't know...

maybe it makes sense to check every frame on it's minimum frameSize to prevent such crashes in other frames decoding code

@ojw28 ojw28 added the bug label Apr 10, 2017
ojw28 added a commit that referenced this issue Apr 11, 2017
I've also removed unnecessary "empty" cases, since to add them
everywhere would bloat the code quite a lot. Note that
new String(new byte[0], 0, 0, encoding) is valid and will produce
and empty string.

Issue: #2663

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=152697288
@ojw28
Copy link
Contributor

ojw28 commented Apr 11, 2017

Fixed in dev-v2.

@ojw28 ojw28 closed this as completed Apr 11, 2017
@google google locked and limited conversation to collaborators Aug 10, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

2 participants