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

Status: VERIFIED FIXED

(Core :: Audio/Video, defect)

Description

[Tracy Walker [:tracy]]

This bug was filed from the Socorro interface and is 
report bp-1aaa86d6-aee4-43fb-9b8b-684a62130915.
=============================================================

This has exploded to #4 topcrasher on Nightly since builds of 20130911030258

Comments and urls point to streaming audio.

21 	https://www.facebook.com/
12 	http://www.m2radio.fr/m2-90/player/for-mobile-and-tablet
12 	http://blanik.static.abradio.cz/blanik-fm-stream64.html
11 	about:blank
10 	http://www.cnn.com/?refresh=1
7 	http://www.tumblr.com/dashboard
6 	http://www.comunicacionessic.com/
6 	https://www.google.com/
5 	http://forecast.weather.gov/MapClick.php?lat=41.63055013678316&lon=-72.871456...
5 	http://www.iloveradio.de/
4 	http://lprussia.com/music/altnc_lprussia.mp3
4 	about:newtab
3 	http://tegos.ru/new/radio_record/Klaas_-_Flight_To_Paris_mix.mp3
3 	https://www.facebook.com/?ref=tn_tnmn
3 	http://promodj.com/vividj/remixes/4128207/Natali_O_Bozhe_Kakoy_Muzhchina_Vita...
3 	http://www.jmccanneyscience.com/JamesMcCanneyScienceHour_September_12_2013.mp3

Comment 1

[Tracy Walker [:tracy]]

pushlog:  http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=e5ca10a2b3d0&tochange=9e9f74116749

Comment 2

[Benjamin Smedberg]

The obvious candidate is: Chris Pearce — Bug 910897 - Use MP3FrameParser in DirectShow in order to calculate the duration. r=padenot

Comment 3

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch

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.

Comment 4

[Chris Pearce [:cpearce (Not reading bugmail)]]

I forgot to mention; I could reproduce this by serving an MP3 file to Firefox with a very very slow connection (0.1KBps).

Comment 5

[Chris Pearce [:cpearce (Not reading bugmail)]]

Greenish on try:
https://tbpl.mozilla.org/?tree=Try&rev=d8f596e0a52d

Comment 6

[Paul Adenot (:padenot)]

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.

Comment 7

[Chris Pearce [:cpearce (Not reading bugmail)]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a0073cef7bb9

Will need this on aurora too.

Comment 8

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/a0073cef7bb9

Comment 9

[Tracy Walker [:tracy]]

Will this be applied to Aurora (Fx26)?  It is currently the #2 (12.72%) crasher closely behind the Empty crash (13.26%)

Comment 10

[Tracy Walker [:tracy]]

There are no reports of this crashing Nightly since the fix was checked in there. (last crashy build was 20130919030202)

Comment 11

[Chris Pearce [:cpearce (Not reading bugmail)]]

(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.

Comment 12

[Chris Pearce [:cpearce (Not reading bugmail)]]

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.

Comment 13

[Chris Pearce [:cpearce (Not reading bugmail)]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/e6486528e938

Comment 14

[Ada [:adalucinet]]

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?

Comment 15

[Robert Kaiser]

(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.

Comment 16

[Chris Pearce [:cpearce (Not reading bugmail)]]

kairo++

Comment 17

[Ada [:adalucinet]]

(In reply to Robert Kaiser (:[e-mail]) 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.