Closed Bug 1189992 Opened 5 years ago Closed 5 years ago
Stagefright: fpe crash [@stagefright::Sample
17.01 KB, text/plain
2.24 KB, video/mp4
1.21 KB, patch
|Details | Diff | Splinter Review|
No description provided.
[Blocking Requested - why for this release]: [Tracking Requested - why for this release]:
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.
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
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.