Closed Bug 1296532 Opened 4 years ago Closed 4 years ago

mp4 triggers OOM [@mp4_demuxer::Saiz::Saiz]

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox51 --- fixed

People

(Reporter: tsmith, Assigned: gerald)

References

(Blocks 1 open bug)

Details

(Keywords: crash, csectype-oom, testcase, Whiteboard: [sg:dos])

Attachments

(7 files)

Attached file log.txt
Not sure if this is just an OOM or not. Please let me know if not and I will unhide the bug.

This can take a few seconds to crashes. About 30 seconds on Linux

mozglue!mozalloc_abort+0x2a [c:\mozilla-central\memory\mozalloc\mozalloc_abort.cpp @ 33]
mozglue!mozalloc_handle_oom+0x61 [c:\mozilla-central\memory\mozalloc\mozalloc_oom.cpp @ 46]
mozglue!moz_xrealloc+0x24 [c:\mozilla-central\memory\mozalloc\mozalloc.cpp @ 134]
xul!nsTArrayInfallibleAllocator::Realloc+0xd [c:\mozilla-central\objdir-ff\dist\include\nstarray.h @ 182]
xul!nsTArray_base<nsTArrayInfallibleAllocator,nsTArray_CopyWithMemutils>::EnsureCapacity<nsTArrayInfallibleAllocator>+0xb9 [c:\mozilla-central\objdir-ff\dist\include\nstarray-inl.h @ 183]
xul!nsTArray_Impl<signed char,nsTArrayInfallibleAllocator>::AppendElement<signed char &,nsTArrayInfallibleAllocator>+0x19 [c:\mozilla-central\objdir-ff\dist\include\nstarray.h @ 2038]
xul!mp4_demuxer::Saiz::Saiz+0x118 [c:\mozilla-central\media\libstagefright\binding\moofparser.cpp @ 856]
xul!mp4_demuxer::Moof::ParseTraf+0x113 [c:\mozilla-central\media\libstagefright\binding\moofparser.cpp @ 490]
xul!mp4_demuxer::Moof::Moof+0xda [c:\mozilla-central\media\libstagefright\binding\moofparser.cpp @ 367]
xul!mp4_demuxer::MoofParser::RebuildFragmentedIndex+0xe6 [c:\mozilla-central\media\libstagefright\binding\moofparser.cpp @ 51]
xul!mp4_demuxer::MoofParser::RebuildFragmentedIndex+0x25 [c:\mozilla-central\media\libstagefright\binding\moofparser.cpp @ 35]
xul!mozilla::MP4TrackDemuxer::EnsureUpToDateIndex+0x49 [c:\mozilla-central\dom\media\fmp4\mp4demuxer.cpp @ 270]
xul!mozilla::MP4TrackDemuxer::MP4TrackDemuxer+0xb4 [c:\mozilla-central\dom\media\fmp4\mp4demuxer.cpp @ 240]
xul!mozilla::MP4Demuxer::GetTrackDemuxer+0xae [c:\mozilla-central\dom\media\fmp4\mp4demuxer.cpp @ 172]
xul!mozilla::MediaFormatReader::OnDemuxerInitDone+0xc1 [c:\mozilla-central\dom\media\mediaformatreader.cpp @ 291]
xul!mozilla::MozPromise<enum nsresult,enum mozilla::DemuxerFailureReason,1>::InvokeCallbackMethod+0xb [c:\mozilla-central\objdir-ff\dist\include\mozilla\mozpromise.h @ 457]
xul!mozilla::MozPromise<enum nsresult,enum mozilla::DemuxerFailureReason,1>::MethodThenValue<mozilla::MediaFormatReader,void (__thiscall mozilla::MediaFormatReader::*)(enum nsresult),void (__thiscall mozilla::MediaFormatReader::*)(enum mozilla::DemuxerFailureReason)>::DoResolveOrRejectInternal+0x15 [c:\mozilla-central\objdir-ff\dist\include\mozilla\mozpromise.h @ 508]
xul!mozilla::MozPromise<enum nsresult,enum mozilla::DemuxerFailureReason,1>::ThenValueBase::DoResolveOrReject+0x7c [c:\mozilla-central\objdir-ff\dist\include\mozilla\mozpromise.h @ 407]
xul!mozilla::MozPromise<enum nsresult,enum mozilla::DemuxerFailureReason,1>::ThenValueBase::ResolveOrRejectRunnable::Run+0x6a [c:\mozilla-central\objdir-ff\dist\include\mozilla\mozpromise.h @ 324]
xul!mozilla::AutoTaskDispatcher::TaskGroupRunnable::Run+0x53 [c:\mozilla-central\objdir-ff\dist\include\mozilla\taskdispatcher.h @ 195]
xul!mozilla::TaskQueue::Runner::Run+0x85 [c:\mozilla-central\xpcom\threads\taskqueue.cpp @ 173]
xul!nsThreadPool::Run+0x2f3 [c:\mozilla-central\xpcom\threads\nsthreadpool.cpp @ 229]
xul!nsThread::ProcessNextEvent+0x233 [c:\mozilla-central\xpcom\threads\nsthread.cpp @ 1058]
xul!NS_ProcessNextEvent+0x26 [c:\mozilla-central\xpcom\glue\nsthreadutils.cpp @ 290]
xul!mozilla::ipc::MessagePumpForNonMainThreads::Run+0xee [c:\mozilla-central\ipc\glue\messagepump.cpp @ 338]
xul!MessageLoop::RunInternal+0x8 [c:\mozilla-central\ipc\chromium\src\base\message_loop.cc @ 232]
xul!MessageLoop::RunHandler+0x53 [c:\mozilla-central\ipc\chromium\src\base\message_loop.cc @ 226]
xul!MessageLoop::Run+0x19 [c:\mozilla-central\ipc\chromium\src\base\message_loop.cc @ 206]
xul!nsThread::ThreadFunc+0xb4 [c:\mozilla-central\xpcom\threads\nsthread.cpp @ 461]
nss3!_PR_NativeRunThread+0xc1 [c:\mozilla-central\nsprpub\pr\src\threads\combined\pruthr.c @ 419]
nss3!pr_root+0xb [c:\mozilla-central\nsprpub\pr\src\md\windows\w95thred.c @ 95]
ucrtbase!_crt_at_quick_exit+0x104
KERNEL32!BaseThreadInitThunk+0x24
ntdll!__RtlUserThreadStart+0x2f
ntdll!_RtlUserThreadStart+0x1b
Attached file asan_log.txt
Another variation
Attached video test_case.mp4
No longer blocks: fuzzing-stagefright
Flags: in-testsuite?
Similar to bug 1296473, this is causing an OOM by trying to allocate billions of bytes (adding the same byte one at a time to an infallible array, that's why it takes a little while to happen).
The 2nd log is similar, but the first big allocation worked and it's a later one (based on the first set of numbers) that failed.

Unhiding bug as it's not a security threat.
Assignee: nobody → gsquelart
Group: media-core-security
Whiteboard: [sg:dos]
Anthony, is it possible that this is related to the black-screen issues we've been seeing with Facebook Live video streams on Windows (which use MP4 containers). If so I'd suggest a higher prioritization.
Flags: needinfo?(ajones)
(In reply to Benjamin Smedberg [:bsmedberg] from comment #4)
> Anthony, is it possible that this is related to the black-screen issues
> we've been seeing with Facebook Live video streams on Windows (which use MP4
> containers). If so I'd suggest a higher prioritization.

The bug here happens for ill-formatted videos (with impossibly-high number-of-samples), and results in an OOM crash from an infallible allocation.
So I doubt it would "only" cause black-screen issues.

In any case I'm already working on it (it was a low-hanging fruit for me), here's a WIP try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2dee6b668ba9042700a332e3eaa3d6bb7ed8fc36

I'm hoping to get it reviewed & pushed in the next day or so. We'll see then if that helps with Facebook issues...
(In reply to Benjamin Smedberg [:bsmedberg] from comment #4)
> Anthony, is it possible that this is related to the black-screen issues
> we've been seeing with Facebook Live video streams on Windows (which use MP4
> containers). If so I'd suggest a higher prioritization.

No. The black video issues are caused by Flash.
Flags: needinfo?(ajones)
Comment on attachment 8785058 [details]
Bug 1296532 - Optionally allow crypto in stagefright gtest -

https://reviewboard.mozilla.org/r/74392/#review72212
Attachment #8785058 - Flags: review?(jyavenard) → review+
Comment on attachment 8785059 [details]
Bug 1296532 - Fix MoofParser tests -

https://reviewboard.mozilla.org/r/74394/#review72214
Attachment #8785059 - Flags: review?(jyavenard) → review+
Comment on attachment 8785060 [details]
Bug 1296532 - Added test case to stagefright gtest -

https://reviewboard.mozilla.org/r/74396/#review72216
Attachment #8785060 - Flags: review?(jyavenard) → review+
Comment on attachment 8785061 [details]
Bug 1296532 - Use fallible arrays in MoofParser -

https://reviewboard.mozilla.org/r/74398/#review72218

::: media/libstagefright/binding/MoofParser.cpp:547
(Diff revision 1)
>      LOG(Moof, "Incomplete Box (missing sampleCount)");
>      return false;
>    }
>    uint32_t sampleCount = reader->ReadU32();
>    if (sampleCount == 0) {
> +    reader->DiscardRemaining();

This is starting to get messy/bloated.

Please extract the AutoByteReader https://dxr.mozilla.org/mozilla-central/source/dom/media/flac/FlacFrameParser.cpp?q=AutoByteReader&redirect_type=direct#16

And place it in ByteReader.h. You've already started modifying ByteReader.h with utility class, mays as well continue

so you can remove all the DiscardRemaining

::: media/libstagefright/binding/MoofParser.cpp:912
(Diff revision 1)
>      mAuxInfoType = reader->ReadU32();
>      mAuxInfoTypeParameter = reader->ReadU32();
>    }
>    size_t count = reader->ReadU32();
>    need = (version ? sizeof(uint64_t) : sizeof(uint32_t)) * count;
> -  if (reader->Remaining() < count) {
> +  if (reader->Remaining() < need) {

I would be anal enough to say that this should be on a bug on its own and uplifted.
as I think it's the most serious problem of all as it could occur with valid files (the others typically won't)

::: media/libstagefright/binding/include/mp4_demuxer/MoofParser.h
(Diff revision 1)
>    MediaByteRange FirstCompleteMediaHeader();
>  
>    mozilla::MediaByteRange mInitRange;
>    RefPtr<Stream> mSource;
>    uint64_t mOffset;
> -  nsTArray<uint64_t> mMoofOffsets;

this is out of scope of this bug.

Fly by fixes need their own patch
Comment on attachment 8785061 [details]
Bug 1296532 - Use fallible arrays in MoofParser -

https://reviewboard.mozilla.org/r/74398/#review72226

This needs to be split up.
It's good overall, so not a real r-, but I'd like to see the followup fixes thank you
Attachment #8785061 - Flags: review?(jyavenard) → review-
Comment on attachment 8785061 [details]
Bug 1296532 - Use fallible arrays in MoofParser -

https://reviewboard.mozilla.org/r/74398/#review72218

> This is starting to get messy/bloated.
> 
> Please extract the AutoByteReader https://dxr.mozilla.org/mozilla-central/source/dom/media/flac/FlacFrameParser.cpp?q=AutoByteReader&redirect_type=direct#16
> 
> And place it in ByteReader.h. You've already started modifying ByteReader.h with utility class, mays as well continue
> 
> so you can remove all the DiscardRemaining

Good idea, thanks. I'll do that in a separate bug.

> I would be anal enough to say that this should be on a bug on its own and uplifted.
> as I think it's the most serious problem of all as it could occur with valid files (the others typically won't)

I'll open a new bug for that.

> this is out of scope of this bug.
> 
> Fly by fixes need their own patch

I won't bother opening a new bug for this, I think it's too trivial to merit all the extra paperwork. I suppose it can stay until we remove stagefright completely one day.
Depends on: 1298259
Comment on attachment 8785061 [details]
Bug 1296532 - Use fallible arrays in MoofParser -

https://reviewboard.mozilla.org/r/74398/#review72268

::: media/libstagefright/binding/MoofParser.cpp
(Diff revision 2)
>  
>      // Sometimes audio streams don't properly mark their samples as keyframes,
>      // because every audio sample is a keyframe.
>      sample.mSync = !(sampleFlags & 0x1010000) || aIsAudio;
>  
> -    // FIXME: Make this infallible after bug 968520 is done.

technically, that comment is still valid.

The allocation with SetCapacity was made fallible, we know AppendElement will always be true.
Attachment #8785061 - Flags: review?(jyavenard) → review+
Comment on attachment 8785061 [details]
Bug 1296532 - Use fallible arrays in MoofParser -

https://reviewboard.mozilla.org/r/74398/#review72268

Thank you for the review.

> technically, that comment is still valid.
> 
> The allocation with SetCapacity was made fallible, we know AppendElement will always be true.

Bug 968520 has landed in 43, and I didn't see an obvious way to do an infallible AppendElement on a FallibleTArray, that's why I removed the comment.
But I'll put it back, for someone else to deal with it another time...
Blocks: 1298271
Pushed by gsquelart@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8c218f4870cf
Optionally allow crypto in stagefright gtest - r=jya
https://hg.mozilla.org/integration/autoland/rev/4552565382c0
Fix MoofParser tests - r=jya
https://hg.mozilla.org/integration/autoland/rev/328a5b31fa52
Added test case to stagefright gtest - r=jya
https://hg.mozilla.org/integration/autoland/rev/19b4bf02c0bd
Use fallible arrays in MoofParser - r=jya
You need to log in before you can comment on or make changes to this bug.