Bug 1535980 - Ensure that the audio rate read by WebMDemuxer is correctly compared against the maximum allowable rate during metadata sanity checking to avoid out of range errors that could be produced by malformed media files. r=tsmith
ClosedPublic

Authored by azebrowski on Aug 9 2022, 11:47 PM.
Referenced Files
Unknown Object (File)
Feb 3 2024, 2:18 AM
Unknown Object (File)
Aug 15 2022, 5:40 PM
Unknown Object (File)
Aug 11 2022, 6:03 PM
Subscribers

Diff Detail

Repository
rMOZILLACENTRAL mozilla-central
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

phab-bot changed the visibility from "Custom Policy" to "Public (No Login Required)".
phab-bot changed the edit policy from "Custom Policy" to "Restricted Project (Project)".
phab-bot removed a project: secure-revision.
dom/media/webm/WebMDemuxer.cpp
385–388

AudioInfo::MAX_RATE is stored as a uint32_t as per here, whereas params.rate is stored as a float here. A better idea than the static_cast<decltype> could be to have params.rate and AudioInfo::MAX_RATE use the same datatype for storage, though I'm not sure of the potential side-effects caused by going this route.

The added check for values lower than zero is because params.rate was holding a negative number using the sample video from the bug report.

We could also make improvements to the sanity checks in libnestegg here, for example, though I'm not sure whether making changes to libnestegg is outside the scope of this work.

Code analysis found 1 defect in the diff 612671:

  • 1 defect found by clang-format
WARNING: Found 1 issue (warning level) that can be dismissed.

You can run this analysis locally with:

  • ./mach clang-format -p dom/media/webm/WebMDemuxer.cpp

For your convenience, here is a patch that fixes all the clang-format defects (use it in your repository with hg import or git apply -p0).


If you see a problem in this automated review, please report it here.

You can view these defects in the Diff Detail section of Phabricator diff 612671.

alwu added a subscriber: alwu.

LGTM, could we also add a crash test for this? because we already have the test file in comment2, thanks!

This revision is now accepted and ready to land.Aug 11 2022, 6:46 PM

This revision requires a Testing Policy Project Tag to be set before landing. Please apply one of testing-approved, testing-exception-unchanged, testing-exception-ui, testing-exception-elsewhere, testing-exception-other. Tip: this Firefox add-on makes it easy!

I have tested the patch and can confirm it does resolve the issue. Thank you!