Closed
Bug 813562
Opened 12 years ago
Closed 12 years ago
DASH crash [@_vorbis_block_ripcord]
Categories
(Core :: Audio/Video, defect)
Tracking
()
RESOLVED
FIXED
mozilla20
Tracking | Status | |
---|---|---|
firefox19 | --- | unaffected |
firefox20 | --- | fixed |
firefox-esr10 | --- | unaffected |
firefox-esr17 | --- | unaffected |
b2g18 | --- | unaffected |
People
(Reporter: posidron, Assigned: sworkman)
References
Details
(Keywords: crash, sec-high, Whiteboard: [adv-main20-])
Attachments
(5 files, 1 obsolete file)
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
Reporter | ||
Comment 1•12 years ago
|
||
Assignee | ||
Comment 2•12 years ago
|
||
Assignee | ||
Comment 3•12 years ago
|
||
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.
Reporter | ||
Comment 4•12 years ago
|
||
(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.
Reporter | ||
Updated•12 years ago
|
Attachment #683722 -
Flags: feedback?(cdiehl) → feedback+
Assignee | ||
Comment 5•12 years ago
|
||
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 6•12 years ago
|
||
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+
Assignee | ||
Comment 7•12 years ago
|
||
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.
Assignee | ||
Comment 8•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/88ee3b680835
Comment 9•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/88ee3b680835 Can we get a test for this?
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
status-firefox20:
--- → fixed
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
Assignee | ||
Comment 10•12 years ago
|
||
(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?
Reporter | ||
Comment 11•12 years ago
|
||
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. :-)
Updated•12 years ago
|
status-firefox-esr10:
--- → unaffected
status-firefox-esr17:
--- → unaffected
Updated•11 years ago
|
status-b2g18:
--- → unaffected
Assignee | ||
Comment 12•11 years ago
|
||
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 13•11 years ago
|
||
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.
Assignee | ||
Comment 14•11 years ago
|
||
Sorry, Chris. Didn't update patch.
Attachment #707896 -
Attachment is obsolete: true
Attachment #707896 -
Flags: review?(cpearce)
Attachment #708664 -
Flags: review?(cpearce)
Updated•11 years ago
|
Attachment #708664 -
Flags: review?(cpearce) → review+
Assignee | ||
Comment 15•11 years ago
|
||
Thanks Chris. Try passes: https://tbpl.mozilla.org/?tree=Try&rev=b06bd81b69b8 Landed on m-i: https://hg.mozilla.org/integration/mozilla-inbound/rev/41075512e652
Comment 16•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/41075512e652
Flags: in-testsuite? → in-testsuite+
Comment 17•11 years ago
|
||
Was 19 unaffected by this issue? The patch didn't get sec-approval+ before checkin.
Comment 18•11 years ago
|
||
Per IRC with Christoph, 19 is unaffected.
status-firefox19:
--- → unaffected
Updated•11 years ago
|
Whiteboard: [adv-main20-]
Assignee | ||
Comment 19•11 years ago
|
||
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.
Updated•11 years ago
|
Group: core-security
You need to log in
before you can comment on or make changes to this bug.
Description
•