Closed Bug 916836 Opened 7 years ago Closed 7 years ago

crash in mozilla::MP3Buffer::ExtractFrameHeader(unsigned char const*)

Categories

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

26 Branch
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla27
Tracking Status
firefox26 + verified
firefox27 --- verified

People

(Reporter: tracy, Assigned: cpearce)

Details

(Keywords: crash, regression, topcrash)

Crash Data

Attachments

(1 file)

Component: Web Audio → Video/Audio
The obvious candidate is: Chris Pearce — Bug 910897 - Use MP3FrameParser in DirectShow in order to calculate the duration. r=padenot
Keywords: regression
Assignee: nobody → cpearce
Attached patch PatchSplinter Review
We're crashing because we're running MP3FrameParser::ParseBuffer() with a very large aLngth. This happens when the first parse in MP3FrameParser::Parse() reads the leftover data from the previous parse (mBuffer) and finds a frame in it that extends off the end of the incoming buffer. The length field passed into MP3FrameParser::ParseBuffer() is unsigned, and so we then overflow and things get messed up from then on in.
Attachment #806339 - Flags: review?(paul)
I forgot to mention; I could reproduce this by serving an MP3 file to Firefox with a very very slow connection (0.1KBps).
Comment on attachment 806339 [details] [diff] [review]
Patch

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

::: content/media/MP3FrameParser.cpp
@@ +428,5 @@
> +      if (mOffset > mLength) {
> +        mLength = mOffset;
> +      }
> +      return;
> +    } 

nit: trailing space.
Attachment #806339 - Flags: review?(paul) → review+
https://hg.mozilla.org/mozilla-central/rev/a0073cef7bb9
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
Will this be applied to Aurora (Fx26)?  It is currently the #2 (12.72%) crasher closely behind the Empty crash (13.26%)
Flags: needinfo?(cpearce)
There are no reports of this crashing Nightly since the fix was checked in there. (last crashy build was 20130919030202)
Status: RESOLVED → VERIFIED
(In reply to Tracy Walker [:tracy] from comment #9)
> Will this be applied to Aurora (Fx26)?

Yes.

(In reply to Tracy Walker [:tracy] from comment #10)
> There are no reports of this crashing Nightly since the fix was checked in
> there. (last crashy build was 20130919030202)

Thanks! I wanted to wait for a few days so that we could be sure this patch was all that needed to be uplifted.
Flags: needinfo?(cpearce)
Comment on attachment 806339 [details] [diff] [review]
Patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 861693, MP3 decoding support on WinXP
User impact if declined: Crashes when playing MP3 files on slow networks.
Testing completed (on m-c, etc.): Been on m-c for several days, crash fixed there.
Risk to taking this patch (and alternatives if risky): Low.
String or IDL/UUID changes made by this patch: None.
Attachment #806339 - Flags: approval-mozilla-aurora?
Attachment #806339 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Couldn't reproduce this crash by using STR from comment 4 on Nightly (from 2013-09-15 and 2013-09-12) with Netlimiter (bandwidth set to 0.1KBps) nor with links from comment 0.

In Socorro there are no crashes in the last month for:
- Firefox 27 beta: http://goo.gl/Ywlinq
- Firefox 26 RC: http://goo.gl/o4ME0r

Are the crash stats enough to call this verified? If not, could you please advice how to reproduce in order to verify this is fixed?
Flags: needinfo?(cpearce)
(In reply to Alexandra Lucinet, QA Mentor [:adalucinet] from comment #14)
> In Socorro there are no crashes in the last month for:
> - Firefox 27 beta: http://goo.gl/Ywlinq
> - Firefox 26 RC: http://goo.gl/o4ME0r
> 
> Are the crash stats enough to call this verified?


As it was filed as a topcrash based on Socorro data, I think being absent from there is enough to verify this, yes.
kairo++
Flags: needinfo?(cpearce)
(In reply to Robert Kaiser (:kairo@mozilla.com) from comment #15)
> As it was filed as a topcrash based on Socorro data, I think being absent
> from there is enough to verify this, yes.

Thanks for the reply.
Marking as verified on Firefox 26 and 27 beta.
You need to log in before you can comment on or make changes to this bug.