Closed Bug 1253471 Opened 4 years ago Closed 4 years ago

Embeded video returns an error in Firefox

Categories

(Core :: Audio/Video: Playback, defect, P2)

44 Branch
x86_64
Windows 7
defect

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox44 --- wontfix
firefox45 + wontfix
firefox46 + fixed
firefox47 + fixed
firefox48 --- fixed
firefox-esr45 - wontfix

People

(Reporter: eugeneghummer, Assigned: gerald)

References

(Blocks 1 open bug)

Details

(Keywords: regression)

Attachments

(2 files, 1 obsolete file)

Attached file mediainfo_log
User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/48.0.2564.116 Safari/537.36

Steps to reproduce:

1. Embed a video directly into a Webpage. 

<iframe src='http://embed.azubu.tv/video188762SVgameshowcsgoen' frameborder='0' width='960' height='540' allowfullscreen webkitallowfullscreen mozallowfullscreen></iframe>



Actual results:

There is a decode error for the next Firefox releases:

- Firefox 44.0 +
https://gyazo.com/7ef4d6bc8afd7a5885adfd988481866f

- Firefox 45.0 Beta
https://gyazo.com/e57f592fb749fa972d4165cbd0287951

- Firefox 47.0a1 nightly (2016-03-03)
https://gyazo.com/7097d5434b0717d8f388ee31634664dd


But the video in question has no playback issues on the outdated versions of Firefox:

- Firefox 43.0, 43.03.3
https://gyazo.com/0dbacefbd5cb3f916dd50efb888299fd

It appears that somewhere between version 43 and 44 of Firefox, there was a change to media handling to have the decode error. 


Expected results:

There are no playback issues.
Severity: normal → critical
OS: Unspecified → Windows 7
Priority: -- → P1
Hardware: Unspecified → x86_64
Keywords: embed
Whiteboard: embed, mp4, decode, changes at backend
Severity: critical → normal
Component: Untriaged → Audio/Video: Playback
Keywords: embed
Priority: P1 → --
Product: Firefox → Core
Whiteboard: embed, mp4, decode, changes at backend
Regression range:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=0a1ac3ab8cb90dc9d26bd5702e43f6a51ec018d1&tochange=2a57c0a0cf19b32f5fa9cb72c44a92e3319d14a0

Gerald Squelart — Bug 1232069 - Check box sizes before alloc&copy. r=jya
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(gsquelart)
Keywords: regression
Version: unspecified → 44 Branch
if this video doesnt play because of this change it means that the mp4 contains invalid box and is deemed a security risk and as such now refused
Debugging a little bit, I see a "moov" box that is 41,543,807 bytes long, or about 40MB, but we have set a limit of 32MB.
(Note: the stream size is >24TB! But it's a TV stream, so I'm guessing they just want to make it pseudo-infinite.)

Jean-Yves, I see you first introduced this 32MB limit in bug 1149278 (which was then reused in bug 1232069, to sanity-check the metadata range before processing). Any particular reason for this number?
Flags: needinfo?(gsquelart) → needinfo?(jyavenard)
We have to copy the metadata and keep it in RAM.

It was felt that 32MB limit was appropriate, seeing that IIRC, a 60fps video sample table size would exceed a 32MB after about 5 years.
Flags: needinfo?(jyavenard)
Is this really a regression then or is it intended behavior?
A bit more data, after relaxing the limit to 64MB:

The data for the video 'trak' takes 22,466,470 bytes, including 18,720,652 for the 'stsz' (table of sizes of all samples), ~2MB in 'stsc' (sample to chunk map) and ~1.5MB in 'co64' (64-bit chunk offsets).
There are 4,680,158 samples, the total duration is 93,603,160,000 us (about 26 hours), that's a framerate of 50Hz.
(jya: How did you get 5 years from 32MB?)

Similar deal on the audio side: trak=~19MB with stsz=~17MB, co64=~1.5MB. 4,387,650 samples.

After parsing, the exported video index table (needed during playback) takes ~214MB (sizeof(MediaSource::Indice)=48, *4.7M).

So what do you think Jean-Yves? Should we increase our limit to allow this kind of video? -- Please let me know if you'd like more details.
Flags: needinfo?(jyavenard)
Depends if we're happy to keep a copy of a 64MB blob in RAM on top of an enormous sample table. 

I didn't come up with the 32MB limit. Anthony wanted 16MB to start with. 

I leave that decision to him
Flags: needinfo?(jyavenard) → needinfo?(ajones)
(In reply to Jean-Yves Avenard [:jya] from comment #7)
> Depends if we're happy to keep a copy of a 64MB blob in RAM on top of an
> enormous sample table. 
> 
> I didn't come up with the 32MB limit. Anthony wanted 16MB to start with. 
> 
> I leave that decision to him

Why do we keep a copy of the moov in RAM?
(In reply to Anthony Jones (:kentuckyfriedtakahe, :k17e) from comment #8)
> Why do we keep a copy of the moov in RAM?

Though it is a valid question, the copy of the moov is only 15% of the memory taken by the metadata structures for this 26-hour video.
We might indeed want to look at some optimizations (e.g., getting rid of the source metadata after parsing, shrinking the exported index table, etc.)

But in any case the current 32MB box limit is preventing us from playing some videos.

So I think a decision is needed, whether we accept video streams with humongous metadata (and do whatever we can to play them now, even if we could optimize some things), or keep rejecting them and let other browsers deal with them for now.
(In reply to Gerald Squelart [:gerald] from comment #9)
> So I think a decision is needed, whether we accept video streams with
> humongous metadata (and do whatever we can to play them now, even if we
> could optimize some things), or keep rejecting them and let other browsers
> deal with them for now.

Lets just make the limit several times larger.
Flags: needinfo?(ajones)
Assignee: nobody → gsquelart
Blocks: 1253974
Gerald, 

Could you answer Comment 5 please?
Flags: needinfo?(gsquelart)
(In reply to Benjamin Kerensa [:bkerensa] from comment #12)
> Could you answer Comment 5 please?

Sorry, it slipped from my radar. Sure:


(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #5)
> Is this really a regression then or is it intended behavior?

I'd say it's intended behavior (limit mp4 boxes to what was thought "reasonable", to prevent OOM attacks) that introduced a regression (some videos that used to be playable became unplayable).

If you absolutely need to classify it one way or another, I'd probably go with the worst option: Regression.

Note that technically, even with the proposed patch the regression is still there: Videos with >128MB of metadata (corresponding to ~83 hours @ 50Hz) won't be played. But this seems even more unlikely than videos with >32MB of metadata (~21h@50Hz).
So we may want to revisit this limit, probably as part of working on follow-up bug 1253974; I'll add a note there.
Flags: needinfo?(gsquelart)
(In reply to Gerald Squelart [:gerald] from comment #9)
> > Why do we keep a copy of the moov in RAM?
> 
> Though it is a valid question, the copy of the moov is only 15% of the
> memory taken by the metadata structures for this 26-hour video.
> We might indeed want to look at some optimizations (e.g., getting rid of the
> source metadata after parsing, shrinking the exported index table, etc.)

My memory faulted me here.

This constant used to only be used when server didn't report a Content-Length nor a Content-Range, and we had to continue reading all boxes to find the moov.

Seeing that this video in particular would have been unplayable anyway as the server wouldn't have supported seeking, a 32MB limit is perfectly acceptable.

In fact it could be much less IMHO.

It's the security fix in bug 1232069 that extended the use of this constant that created the regression. It affects all version from 44 as it was uplifted there at the time.
(In reply to Jean-Yves Avenard [:jya] from comment #14)
> (In reply to Gerald Squelart [:gerald] from comment #9)
> It's the security fix in bug 1232069 that extended the use of this constant
> that created the regression. It affects all version from 44 as it was
> uplifted there at the time.

After further discussion with Jean-Yves, I see that this limit shouldn't have been used in the Metadata extraction routine.
I am preparing a second patch to remove this limit there (but still check for overflows&truncations, and of course actual OOMs).
After this, we can consider the regression fully fixed.
Can you add bug 1232069 to the "Blocks" field, please. I don't have permission for that.
(In reply to Loic from comment #16)
> Can you add bug 1232069 to the "Blocks" field, please. I don't have
> permission for that.

I assume you meant "Depends on", as bug 1232069 is already fixed & landed in 44+, so it can't be blocked by this bug here.
Depends on: CVE-2016-1946
I have always thought the logic about regression was to add the regressing bug (here bug 1232069) in the blocks field. :)
Priority: -- → P2
Duplicate of this bug: 1253459
Tracking then for 46+ - and this seems simple enough to uplift once it lands.
(In reply to Loic from comment #18)
> I have always thought the logic about regression was to add the regressing
> bug (here bug 1232069) in the blocks field. :)

The logic still feels backwards to me, but I see it's the accepted way in these lands, so I'll follow the herd. ;-)
No longer depends on: CVE-2016-1946
Comment on attachment 8727215 [details]
MozReview Request: Bug 1253471 - Bump box size limit to 128MB - r?k17e

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38395/diff/1-2/
Instead of relying on some arbitrary limit for ftyp+moov box sizes, we check
for overflow and possible type truncations, and then let memory allocation
routines (e.g. MediaByteBuffer::SetLength) deal with actual memory limitations.

Review commit: https://reviewboard.mozilla.org/r/38613/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/38613/
Attachment #8727705 - Flags: review?(jyavenard)
Comment on attachment 8727705 [details]
MozReview Request: Bug 1253471 - Remove Metadata hard-coded limit - r?jya

https://reviewboard.mozilla.org/r/38613/#review35259

::: media/libstagefright/binding/MoofParser.cpp:199
(Diff revision 1)
> -  if (!metadata->SetLength(ftyp.Length() + moov.Length(), fallible)) {
> +  if (!metadata->SetLength(totalLength.value(), fallible)) {

nit: could leave those as is as we now longer care for overflow.

make the patch smaller
Attachment #8727705 - Flags: review?(jyavenard) → review+
https://reviewboard.mozilla.org/r/38395/#review35261

::: media/libstagefright/binding/Box.cpp:18
(Diff revision 2)
> -const uint64_t Box::kMAX_BOX_READ = 32 * 1024 * 1024;
> +const uint64_t Box::kMAX_BOX_READ = 128 * 1024 * 1024;

are you still going on with those?

imho you shouldn't
Comment on attachment 8727215 [details]
MozReview Request: Bug 1253471 - Bump box size limit to 128MB - r?k17e

(In reply to Jean-Yves Avenard [:jya] from comment #27)
> https://reviewboard.mozilla.org/r/38395/#review35261
> 
> ::: media/libstagefright/binding/Box.cpp:18
> (Diff revision 2)
> > -const uint64_t Box::kMAX_BOX_READ = 32 * 1024 * 1024;
> > +const uint64_t Box::kMAX_BOX_READ = 128 * 1024 * 1024;
> 
> are you still going on with those?
> 
> imho you shouldn't

Anthony gave us the green light to do what we think is best, so I'm obsoleting this patch.
Attachment #8727215 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/f08207e3060c
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Comment on attachment 8727705 [details]
MozReview Request: Bug 1253471 - Remove Metadata hard-coded limit - r?jya

Approval Request Comment
[Feature/regressing bug #]: Bug 1232069 prevents long videos from playing

[User impact if declined]: Some sites with very long videos will keep showing a misleading "decode error" (they work in other browsers)

[Describe test coverage new/current, TreeHerder]: Lots of media tests that exercize the MP4 metadata parser, Aurora try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=acdfb6f79b01 , Beta try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=51d0be116d60

[Risks and why]: Some risks associated with new code that checks for potentially-harmful overflows&truncations, but the use of CheckedInt clearly shows what is being tested, and that code has been exercized through try & local testing (in particular, verified that the solution to bug 1232069 is still working as intended).

[String/UUID change made/needed]: None.


(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #21)
> Tracking then for 46+ - and this seems simple enough to uplift once it lands.

Please note that a different patch (from the time of that comment) has landed, which is not as simple.
I still believe it should be uplifted to re-enable playback on some websites.
Attachment #8727705 - Flags: approval-mozilla-beta?
Attachment #8727705 - Flags: approval-mozilla-aurora?
EugeneB, could you please verify this issue is fixed as expected on a latest Nightly build, say 03/09/2016? Thanks!
Flags: needinfo?(eugeneghummer)
Comment on attachment 8727705 [details]
MozReview Request: Bug 1253471 - Remove Metadata hard-coded limit - r?jya

Improves playback of large videos (>32 MB), taking it in Aurora47 and Beta46.
Attachment #8727705 - Flags: approval-mozilla-beta?
Attachment #8727705 - Flags: approval-mozilla-beta+
Attachment #8727705 - Flags: approval-mozilla-aurora?
Attachment #8727705 - Flags: approval-mozilla-aurora+
(In reply to Ritu Kothari (:ritu) from comment #33)
> Improves playback of large videos (>32 MB), taking it in Aurora47 and Beta46.

Thank you Ritu.

Just to be clear: The 32 MB limit was just for parts of the metadata ('ftyp' and 'moov' boxes), the full videos would be orders of magnitude larger, more than 20 hours long (which could be anything from GBs to TBs depending on duration, resolution and other factors).
(In reply to Ritu Kothari (:ritu) from comment #33)

The initial issue was not reproduced on a latest Nightly build 48.0a1(2016-03-10).

flags: needinfo?(rkothari@mozilla.com)
Flags: needinfo?(eugeneghummer)
You need to log in before you can comment on or make changes to this bug.