-
Notifications
You must be signed in to change notification settings - Fork 474
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
Catch errors and OOM when decoding ID3 frames. #369
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please provide a test asset in this PR with a problematically large ID3 tag that reproduces the failure this PR is fixing? You can probably do this by either:
- Adding a test case to
Id3DecoderTest
with the bytes written out in java (probably only if it's very short, which seems like it might be unlikely since we're testing an OOM :p) - Or copying and modifying one of the existing assets and using that in a new test case in
Id3DecoderTest
(see how they're currently used inId3PeekerTest
).
libraries/extractor/src/main/java/androidx/media3/extractor/metadata/id3/Id3Decoder.java
Outdated
Show resolved
Hide resolved
I can provide a test asset in private but it's probably copyrighted including the image. (Got the file from an user). That file crash most of the id3 tools I have so can't even extract the tags. (The other tools refuse to read them) So not sure how to be able to build one similar. |
Bump on this one, I'm not able to generate the test asset with the tools I have at my disposal. I can still send the copyrighted file by mail if anyone is able to extract the tags and reproduce in a non copyrighted way. If you can generate and send me the test asset then I can finish this PR. |
After some further discussion we said it's probably alright not to have a test file for this case. We have other OOM error clauses that guard against invalid media without a unit test, so this doesn't feel a lot different. |
libraries/extractor/src/main/java/androidx/media3/extractor/metadata/id3/Id3Decoder.java
Outdated
Show resolved
Hide resolved
libraries/extractor/src/main/java/androidx/media3/extractor/metadata/id3/Id3Decoder.java
Outdated
Show resolved
Hide resolved
Invalid frames have no impact on ExoPlayer ability to play the media and should not fail on errors. Some tools can add 100Mb images in the tags that will trigger recoverable OOM with this fix.
2995a55
to
c365193
Compare
I'm going to send this for internal review now. You may see some more commits being added as I make changes in response to review feedback. Please refrain from pushing any more substantive changes as it will complicate the internal review - thanks! |
c365193
to
7bb0afd
Compare
7bb0afd
to
f935f59
Compare
PiperOrigin-RevId: 595650068 (cherry picked from commit 8eda9f2)
Invalid frames have no impact on ExoPlayer ability to play the media and should not fail on errors.
Some tools can add 100Mb images in the tags that will trigger recoverable OOM with this fix, previously the load would fail and the OOM caught too late.