Closed Bug 813562 Opened 7 years ago Closed 7 years ago

DASH crash [@_vorbis_block_ripcord]

Categories

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

x86_64
macOS
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla20
Tracking Status
firefox19 --- unaffected
firefox20 --- fixed
firefox-esr10 --- unaffected
firefox-esr17 --- unaffected
b2g18 --- unaffected

People

(Reporter: posidron, Assigned: sworkman)

References

(Blocks 1 open bug)

Details

(Keywords: crash, sec-high, Whiteboard: [adv-main20-])

Attachments

(5 files, 1 obsolete file)

Attached file callstack
This happened while fuzzing DASH, the logged testcase did not reproduce the issue but I am getting such crashes often during the run. Marking it sec-high based on the invalid address.

./content/media/webm/WebMReader.cpp:129

  vorbis_block_clear(&mVorbisBlock); 

I believe this is related to 813549. Each testcases uses 'autoplay' in <video> and the fuzzer switches to the next testcase without any cleanup (eg. stopping the video).

The last warnings were:

WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x804B000A: file /Users/cdiehl/Code/Mozilla/mc-inbound-asan/content/base/src/nsImageLoadingContent.cpp, line 554
WARNING: NS_ENSURE_TRUE(!(aUrl.IsEmpty())) failed: file /Users/cdiehl/Code/Mozilla/mc-inbound-asan/netwerk/dash/mpd/Representation.cpp, line 88
WARNING: NS_ENSURE_TRUE(mVideoRepDecoder) failed: file /Users/cdiehl/Code/Mozilla/mc-inbound-asan/content/media/dash/DASHDecoder.cpp, line 425


Test against m-inbound with changeset: 113725:35c328e03795
Attached file last-logged.mpd
Assignee: nobody → sworkman
Status: NEW → ASSIGNED
Attachment #683722 - Flags: feedback?(cdiehl)
Thanks for the testing Christoph. It's still early days for DASH, but it's good to get the bugs knocked down early.

The problem seems to be uninitialized vars in WebMReader being destroyed/cleared. This appears in DASH because WebMReader::Init is called a little later than for regular WebM. So, those vars are still in a very raw state when the error is encountered and the system doesn't cleanup properly.

I've added some lines to clear the memory in the constructor of WebMReader, which should stop the VP8 and vorbis cleanup functions from trying to use unitializes pointers. I did a quick test locally, playing a DASH video and a regular WebM one - seems to run fine.

Any chance you could give me some feedback and try this patch in your fuzzing tests?

Also, this patch should fix bug 813549 as well: I just uploaded here in the hidden bug to be cautious.
(In reply to Steve Workman [:sworkman] from comment #3)
> Thanks for the testing Christoph. It's still early days for DASH, but it's
> good to get the bugs knocked down early.
> 

Thought the same. :-)

> Any chance you could give me some feedback and try this patch in your
> fuzzing tests?
> 
> Also, this patch should fix bug 813549 as well: I just uploaded here in the
> hidden bug to be cautious.


Yep, fixed.
Attachment #683722 - Flags: feedback?(cdiehl) → feedback+
Comment on attachment 683722 [details] [diff] [review]
V1.0 Proposed fix - memsets member vars to 0

Christoph - awesome, thanks for testing.

Matthew, can you review the patch, please?
(Btw, I'll be off for the next few days due to Thanksgiving, so I won't get a chance to land it until Monday - assuming r+ before then :) ).
Attachment #683722 - Flags: review?(kinetik)
Comment on attachment 683722 [details] [diff] [review]
V1.0 Proposed fix - memsets member vars to 0

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

This assumes that passing a zeroed structure to the codec's destroy function is safe.  It looks like we already do this in OggCodecState.cpp for Vorbis, and a quick inspection of vpx_codec_destroy suggests that it can also handle this, so I guess it's fine.
Attachment #683722 - Flags: review?(kinetik) → review+
https://tbpl.mozilla.org/?tree=Try&rev=6c35e29b8935
All fine on this one, except Otoro was burning.

https://tbpl.mozilla.org/?tree=Try&rev=6efdcf53cef0
Update repo and Otoro is fine on this one; patch unchanged.
https://hg.mozilla.org/mozilla-central/rev/88ee3b680835

Can we get a test for this?
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
(In reply to Ryan VanderMeulen from comment #9)
> https://hg.mozilla.org/mozilla-central/rev/88ee3b680835
> 
> Can we get a test for this?

Christoph, since you did the fuzzing, any ideas for a test?
Attached file testcase/crashtest
Under normal circumstances yes, but since this triggered "randomly" after some time. 
I have attached a testcase which simulates the same behavior, not sure if that's exactly what you want though. :-)
As requested, a simple test case. Specifically, this should cover garbled MPDs and garbled WebM files pointed to by the MPD.
Note: This does not cover other decode error cases for MPDs, like wrong profile or non-compliant tags, attributes etc. The test is only to emulate the fuzzing case discovered by Christoph. Those other cases can be added in another bug later.
Attachment #707896 - Flags: review?(cpearce)
Comment on attachment 707896 [details] [diff] [review]
v1.0 Test case for garbled DASH MPD and WebM media

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

::: content/media/test/manifest.js
@@ +349,5 @@
>  
>  // These are files suitable for testing various decoder failures that are
>  // expected to fire MEDIA_ERR_DECODE.  Used by test_decode_error, which expects
>  // an error and emptied event, and no loadedmetadata or ended event.
> +var gDecodeErrorTests = gDASHBadMediaTests.concat([

Is this patch based on top of another patch? gDASHBadMediaTests doesn't appear manifest.js in m-c.
Sorry, Chris. Didn't update patch.
Attachment #707896 - Attachment is obsolete: true
Attachment #707896 - Flags: review?(cpearce)
Attachment #708664 - Flags: review?(cpearce)
Attachment #708664 - Flags: review?(cpearce) → review+
Was 19 unaffected by this issue?

The patch didn't get sec-approval+ before checkin.
Per IRC with Christoph, 19 is unaffected.
Whiteboard: [adv-main20-]
Al, yeah, sorry about that. I didn't understand/misunderstood sec-approval stuff at the time. In any case, DASH work in Gecko has been halted, so it will not be preff'd on any time soon.
Group: core-security
You need to log in before you can comment on or make changes to this bug.