1066319 - crash in OOM | large | mozalloc_abort(char const* const) | mozalloc_handle_oom(unsigned int) | moz_xmalloc | stagefright::MPEG4Source::start(stagefright::MetaData*)

Status: VERIFIED FIXED

(Core :: Audio/Video, defect)

Description

[Liz Henry (:lizzard) (relman/hg->git project)]

This bug was filed from the Socorro interface and is 
report bp-8c6c9625-d0fe-4238-8a8c-c1e702140905.
=============================================================
This crash signature first appeared on 2014-08-29 with the 2014082803 build. It's showing up on Firefox 34 and 35. Most of the urls for the crashes are for YouTube videos. 

Regression window from mozilla-central: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=0753f7b93ab7&tochange=3be45b58fc47


Crashing thread:
0 	mozalloc.dll 	mozalloc_abort(char const* const) 	memory/mozalloc/mozalloc_abort.cpp
1 	mozalloc.dll 	mozalloc_handle_oom(unsigned int) 	memory/mozalloc/mozalloc_oom.cpp
2 	mozalloc.dll 	moz_xmalloc 	memory/mozalloc/mozalloc.cpp
3 	xul.dll 	stagefright::MPEG4Source::start(stagefright::MetaData*) 	media/libstagefright/frameworks/av/media/libstagefright/MPEG4Extractor.cpp
4 	xul.dll 	mp4_demuxer::MP4Demuxer::Init() 	media/libstagefright/binding/mp4_demuxer.cpp
5 	xul.dll 	mozilla::MP4Reader::ReadMetadata(mozilla::MediaInfo*, nsDataHashtable<nsCStringHashKey, nsCString>**) 	content/media/fmp4/MP4Reader.cpp
6 	xul.dll 	mozilla::MediaDecoderStateMachine::DecodeMetadata() 	content/media/MediaDecoderStateMachine.cpp
7 	xul.dll 	mozilla::MediaDecoderStateMachine::CallDecodeMetadata() 	content/media/MediaDecoderStateMachine.cpp
8 	xul.dll 	nsRunnableMethodImpl<tag_nsresult ( nsMemoryReporterManager::*)(void), void, 1>::Run() 	xpcom/glue/nsThreadUtils.h
9 	xul.dll 	mozilla::MediaTaskQueue::Runner::Run() 	content/media/MediaTaskQueue.cpp
10 	xul.dll 	nsThreadPool::Run() 	xpcom/threads/nsThreadPool.cpp
11 	xul.dll 	nsThread::ProcessNextEvent(bool, bool*) 	xpcom/threads/nsThread.cpp
12 	xul.dll 	MessageLoop::RunHandler() 	ipc/chromium/src/base/message_loop.cc
13 	xul.dll 	_SEH_epilog4

Comment 1

[Liz Henry (:lizzard) (relman/hg->git project)]

There are also some similar signatures showing up on the same day on Android.

Comment 2

[Ralph Giles (:rillian)]

Probably MPEG4Extractor.cpp:2504

    mSrcBuffer = new uint8_t[max_size];

We read max_size from the file, or estimate it if it's zero. It's possible files with corrupt values are asking for too large a buffer.

Comment 3

[Ralph Giles (:rillian)]

Attachment: Reject samples with unreasonable sizes.

Comment 4

[Ralph Giles (:rillian)]

Attachment: Reject samples with unreasonable sizes. v2

Also handle overflow.

Comment 5

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Comment on attachment 8488888 [details] [diff] [review]
Reject samples with unreasonable sizes. v2

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

::: media/libstagefright/frameworks/av/media/libstagefright/MPEG4Extractor.cpp
@@ +41,5 @@
>  namespace stagefright {
>  
> +// Reject samples with unreasonable size table entries.
> +#define INPUT_SIZE_LIMIT (4*3110400)
> +#define INPUT_SIZE_INVALID(size) (size <= 0 || size >= INPUT_SIZE_LIMIT)

This should be a function rather than a #define. Make it inline if you prefer. Either add a comment about where the number comes from or show it as something like 4 * (1080 * 1920) * 3 / 2 to at least give some kind of hint.

Suggestions: I wouldn't bother defining a constant separately. Consider that INPUT_SIZE_LIMIT doesn't add any information compared to INPUT_SIZE_INVALID. Additionally I'd prefer we give things affirmative names such as ValidInputSize() and then !ValidInputSize().

Comment 6

[Ralph Giles (:rillian)]

Attachment: Reject samples with unreasonable sizes. v3

Make validation check a function, and document size constant inline.

> Either add a comment about where the number comes from or show it as
> something like 4 * (1080 * 1920) * 3 / 2 to at least give some kind of hint.

I was just following local style. :-)

v2 patch was green on try. https://tbpl.mozilla.org/?tree=Try&rev=e0604057d776

Comment 7

[Ralph Giles (:rillian)]

Attachment: Reject samples with unreasonable sizes. v4

Fix typo, conform to more local style.

Comment 8

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

(In reply to Ralph Giles (:rillian) from comment #6)
> I was just following local style. :-)

Stupid stagefright.

Comment 9

[Ralph Giles (:rillian)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2a7fe756a652

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2a7fe756a652

Comment 11

[Ralph Giles (:rillian)]

Liz, can you verify this fixes the crash, or provide STR so I can?

Comment 12

[Liz Henry (:lizzard) (relman/hg->git project)]

Hi Ralph! I will follow up by checking crash stats over the next few days. I don't have good STR so will verify off of crash-stats to see if any crashes happen in later builds from this list : 

https://crash-stats.mozilla.com/report/list?product=Firefox&signature=OOM+|+large+|+mozalloc_abort%28char+const*+const%29+|+mozalloc_handle_oom%28unsigned+int%29+|+moz_xmalloc+|+stagefright%3A%3AMPEG4Source%3A%3Astart%28stagefright%3A%3AMetaData*%29#tab-table

Do you want to try uplifting it to Aurora as well?

Comment 13

[Ralph Giles (:rillian)]

Great, thanks. Yes, we should uplift.

Comment 14

[Ralph Giles (:rillian)]

Comment on attachment 8489027 [details] [diff] [review]
Reject samples with unreasonable sizes. v4

Approval Request Comment

[Feature/regressing bug #]: 1043696

[User impact if declined]: Viewing some videos will crash the browser.

[Describe test coverage new/current, TBPL]: Landed on m-c. Manual testing. No automated coverage.

[Risks and why]: Risk is low. This is a clean change to better handle errors. We don't have good steps-to-reproduce, so this may not address the complete problem, but the code is clearly better with the patch. 

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

Comment 15

[Lawrence Mandel [:lmandel] (use needinfo)]

Do we have enough data from crash-stats to verify this fix yet?

Comment 16

[Liz Henry (:lizzard) (relman/hg->git project)]

Yes, I can verify this for 35, and there are still a few crashes on Aurora in the 2014091700 and 2014091600 builds.

Comment 17

[Ralph Giles (:rillian)]

Thanks, Liz!

Comment 18

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8489027 [details] [diff] [review]
Reject samples with unreasonable sizes. v4

Now that this has been verified on m-c, Aurora+.

Comment 19

[Ralph Giles (:rillian)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/0d23cb46b0f3

Comment 20

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

Looking in Socorro I see the following data:

1. OOM | large | mozalloc_abort(char const* const) | mozalloc_handle_oom(unsigned int) | moz_xmalloc | stagefright::MPEG4Source::start(stagefright::MetaData*)
- NO crashes in versions 35 or 34 for builds after 19.09

2. abort | abort | __android_log_assert | stagefright::MPEG4Source::start(stagefright::MetaData*)
- 156 crashes in version 34 for builds after 19.09

3. abort | abort | __android_log_assert | stagefright::VectorImpl::itemLocation(unsigned int)
- 275 in versions 35 or 34 for builds after 19.09

Any thoughts?

Comment 21

[Liz Henry (:lizzard) (relman/hg->git project)]

I think there is more to do here, but this seems to have had a positive effect. 
We should report the 2nd and 3rd crash sigs in a different bug. 
Ralph, does that sound reasonable?

Comment 22

[Ralph Giles (:rillian)]

Please do. That number 2 is 34-specific suggests there's something we need to backport. Number 3 looks like an unknown bug.

Comment 23

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

Filled 2 bugs for the two signatures. Marking this as Verified.