Stagefright: CHECK(data_offset == data_end) failed in SampleTable.cpp

RESOLVED FIXED in Firefox 45

Status

()

P1
normal
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: tsmith, Assigned: gerald)

Tracking

(Blocks: 1 bug, {crash, testcase})

unspecified
mozilla45
crash, testcase
Points:
---

Firefox Tracking Flags

(firefox45 fixed)

Details

(Whiteboard: checkin after 1181719 but before 1181220, 1181215, 1181213)

Attachments

(3 attachments, 2 obsolete attachments)

(Reporter)

Description

3 years ago
Created attachment 8630568 [details]
test_case.mp4

A/SampleTable(34431): /builds/slave/m-cen-l64-asan-d-ntly-00000000/build/src/media/libstagefright/frameworks/av/media/libstagefright/SampleTable.cpp:604 CHECK(data_offset == data_end) failed.
(Reporter)

Updated

3 years ago
Blocks: 872136
(Reporter)

Updated

3 years ago
Summary: CHECK(data_offset == data_end) failed in SampleTable.cpp → Stagefright: CHECK(data_offset == data_end) failed in SampleTable.cpp
Component: Audio/Video → Audio/Video: Playback
Gerald, here's another Stagefright bug.
Assignee: nobody → gsquelart
Priority: -- → P1
(Assignee)

Comment 2

3 years ago
Created attachment 8683539 [details] [diff] [review]
1181223-gtest.patch

Part 1: Added gtest case.
Attachment #8683539 - Flags: review?(giles)
(Assignee)

Comment 3

3 years ago
Created attachment 8683543 [details] [diff] [review]
1181223-handle-wrong-saio-saiz-size.patch

Part 2: Handle wrong saio/saiz size instead of assuming it is correct.
Attachment #8683543 - Flags: review?(giles)
(Assignee)

Comment 4

3 years ago
I should probably have explained why the proposed fix just warns about the issue but continues processing past the oversized 'saio'/'saiz' box.

I initially tried to treat it as an "ERROR_MALFORMED", which would stop processing and reject the file completely. But this made one particular test (test_eme_non_mse_fails.html) fail on almost all platforms:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=29ff724b97e9

My guess is that the test file might have a corrupt (but still usable) saio/saiz box, though the fact that it doesn't fail on Mac (my dev platform of course, making debugging "interesting") is worrying.

Anyway, I decided to just warn&continue, because the box being larger than what its content needs is not a big problem (it shouldn't crash or expose an exploitable vulnerability).
Also libstagefright should become obsolete soon, so trying to fix this properly could be a waste of time.

But we should keep this in mind, in case the new mp4 metadata parser also makes test_eme_non_mse_fails.html fail because of a wrong saio/saiz size.

Ralph, what do you think? Investigate now; open a bug; something else?
Flags: needinfo?(giles)
I'd open a new bug if you have time to investigate whether the test file is corrupt, but land this fix in the meantime. It's always good to clear up issues with the test. Or at least document if it's a broken file we need to support.
Flags: needinfo?(giles)
Attachment #8683539 - Flags: review?(giles) → review+
Comment on attachment 8683543 [details] [diff] [review]
1181223-handle-wrong-saio-saiz-size.patch

Review of attachment 8683543 [details] [diff] [review]:
-----------------------------------------------------------------

I assume the parser recovers by starting the next box at the declared boundary rather than being confused by the partial consumption.
Attachment #8683543 - Flags: review?(giles) → review+
(Assignee)

Comment 7

3 years ago
(In reply to Ralph Giles (:rillian) from comment #6)

Thank you for all the reviews today.

> Comment on attachment 8683543 [details] [diff] [review]
> 1181223-handle-wrong-saio-saiz-size.patch
> 
> Review of attachment 8683543 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I assume the parser recovers by starting the next box at the declared
> boundary rather than being confused by the partial consumption.

Correct.
(Assignee)

Comment 8

3 years ago
Created attachment 8683996 [details] [diff] [review]
1181223-gtest.patch

Clarified check-in comment, no actual changes; carrying r+.
Attachment #8683539 - Attachment is obsolete: true
Attachment #8683996 - Flags: review+
(Assignee)

Comment 9

3 years ago
Created attachment 8683999 [details] [diff] [review]
1181223-handle-wrong-saio-saiz-size.patch

Added comment to explain that the parser will skip over the oversized box; no actual code changes; carrying r+.
Attachment #8683543 - Attachment is obsolete: true
Attachment #8683999 - Flags: review+
(Assignee)

Comment 10

3 years ago
Check-in in this order: bug 1181719, bug 1181223, bug 1181220, bug 1181215, bug 1181213
try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=20e7b755a695
Keywords: checkin-needed
Whiteboard: checkin after 1181719 but before 1181220, 1181215, 1181213

Comment 12

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/741fc69a1679
https://hg.mozilla.org/mozilla-central/rev/cd0c64a4656b
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox45: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
You need to log in before you can comment on or make changes to this bug.