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

Catch errors and OOM when decoding ID3 frames. #369

Merged
merged 3 commits into from Jan 4, 2024

Conversation

Tolriq
Copy link
Contributor

@Tolriq Tolriq commented May 1, 2023

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.

@icbaker icbaker self-assigned this May 2, 2023
@icbaker icbaker self-requested a review May 2, 2023 11:23
Copy link
Collaborator

@icbaker icbaker left a 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 in Id3PeekerTest).

@Tolriq
Copy link
Contributor Author

Tolriq commented May 2, 2023

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.

@Tolriq
Copy link
Contributor Author

Tolriq commented Aug 21, 2023

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.

@tonihei
Copy link
Collaborator

tonihei commented Nov 6, 2023

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.

Tolriq and others added 2 commits January 3, 2024 15:49
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.
@icbaker icbaker force-pushed the fix_invalid_frames branch 2 times, most recently from 2995a55 to c365193 Compare January 3, 2024 16:05
@icbaker
Copy link
Collaborator

icbaker commented Jan 3, 2024

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!

@copybara-service copybara-service bot merged commit 8eda9f2 into androidx:main Jan 4, 2024
1 check passed
microkatz pushed a commit that referenced this pull request Jan 11, 2024
PiperOrigin-RevId: 595650068
(cherry picked from commit 8eda9f2)
@androidx androidx locked and limited conversation to collaborators Mar 5, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants