Closed Bug 813562 Opened 7 years ago Closed 7 years ago
DASH crash [@_vorbis
4.11 KB, text/plain
694 bytes, text/plain
1.04 KB, patch
|Details | Diff | Splinter Review|
1.21 KB, text/html
5.69 KB, patch
|Details | Diff | Splinter Review|
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
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?
(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?
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.
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 #708664 - Flags: review?(cpearce) → review+
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
Flags: in-testsuite? → in-testsuite+
Was 19 unaffected by this issue? The patch didn't get sec-approval+ before checkin.
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.
You need to log in before you can comment on or make changes to this bug.