Closed Bug 1189992 Opened 4 years ago Closed 4 years ago

Stagefright: fpe crash [@stagefright::SampleIterator::seekTo]

Categories

(Core :: Audio/Video: Playback, defect, P1, critical)

x86_64
Linux
defect

Tracking

()

RESOLVED FIXED
mozilla43
blocking-b2g 2.1S?
tracking-b2g backlog
Tracking Status
firefox41 --- wontfix
firefox42 --- fixed
firefox43 --- fixed

People

(Reporter: tsmith, Assigned: jya)

References

(Blocks 1 open bug)

Details

(Keywords: crash, testcase)

Crash Data

Attachments

(3 files)

Attached file call_stack.txt
No description provided.
[Blocking Requested - why for this release]:

[Tracking Requested - why for this release]:
blocking-b2g: --- → 2.1S?
Assignee: nobody → jyavenard
Priority: -- → P1
A bit of analysis: The error seems to be a division by 0 in this statement:
> (sampleIndex - mFirstChunkSampleIndex) / mSamplesPerChunk
So mSamplesPerChunk is 0 there. Some possibilities:
- findChunkRange() was not called before that statement, leaving mSamplesPerChunk with its initial value of 0.
- mSamplesPerChunk was not set to its correct value in findChunkRange().
- Some pointer is wrong and we end up with 0 in mSamplesPerChunk. Unlikely I think, it would probably have crashed somewhere else.

A naive fix would be to check for 0 before doing the division, but I'm afraid this would just hide the real cause that set/left mSamplesPerCheck as 0. Someone with better knowledge of this area should investigate further.
Attached video test_case.mp4
Keywords: testcase
If a chunk contains no samples it should be skipped and continue the search (and maybe hit EOS)
Attachment #8661572 - Flags: review?(gsquelart)
Comment on attachment 8661572 [details] [diff] [review]
Don't assume the last chunk always contains the sample we're looking for.

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

r+, if you're confident that this is sufficient to never fall in the case mSamplesPerChunk==0 at the division -- the whole code flow is not trivial to follow!
Attachment #8661572 - Flags: review?(gsquelart) → review+
this is an iterator ; the aim is finding the next chunk containing our samples.

There are two possible cases for why mSamplesPerChunk could be zero.

1. We have multiple chunks and we're not on the last one.
This fall into the case if (mSampleToChunkIndex + 1 < mTable->mNumSampleToChunkOffsets)
mStopChunkSampleIndex won't be modified as the product with mSamplesPerChunk is 0 ; and as such we continue the loop to find the next chunk with the samples we are seeking for.

2. We're on the last chunk
As we test if mSamplesPerChunk is non-zero; we don't modify mStopChunkSampleIndex and increment mSampleToChunkIndex. As mStopChunkSampleIndex is used to know when to stop the loop we fall on the test if (mSampleToChunkIndex == mTable->mNumSampleToChunkOffsets) and as we've hit EOS ; it will return ERROR_OUT_OF_RANGE

The only area this function is called is in MPEG4Extractor::exportIndex() which is our code .

I didn't want to simply return an error upon finding mSamplesPerChunk being zero ; simply because I see nowhere in the spec that states that such entry would be invalid , and as it's just a matter of skipping over we avoid breaking files that would have otherwise played (and not crashed).

The only problem this code had was if the *last* chunk entry had a sample count of 0.
Comment on attachment 8661572 [details] [diff] [review]
Don't assume the last chunk always contains the sample we're looking for.

Approval Request Comment
[Feature/regressing bug #]:1034444
[User impact if declined]: Crash on specially crafted MP4
[Describe test coverage new/current, TreeHerder]: local test, pending on inbound, thorough analysis of the code
[Risks and why]: Low, just skipping over empty entries. The rationale for the fix was described in comment 6, and was done in such a way that potential for regressions are lessen. 
[String/UUID change made/needed]: None
Attachment #8661572 - Flags: approval-mozilla-beta?
Attachment #8661572 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/61a6a8dcefd3
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
Crash Signature: [@stagefright::SampleIterator::seekTo]
Crash Signature: [@stagefright::SampleIterator::seekTo] → [@ stagefright::SampleIterator::seekTo ] [@ stagefright::SampleIterator::seekTo(unsigned int) ]
Comment on attachment 8661572 [details] [diff] [review]
Don't assume the last chunk always contains the sample we're looking for.

During RC week, only bugs which are sec-crit + easy to exploit, severe crash/hangs and major regressions that make the release unusable are being considered. Though this is a crash, it is not a high volume crash or a recent regression. Wontfix for 41.
Attachment #8661572 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
Comment on attachment 8661572 [details] [diff] [review]
Don't assume the last chunk always contains the sample we're looking for.

Fix a crash, taking it.
Attachment #8661572 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.