498824 - Crash in Theora th_comment_clear

Status: VERIFIED FIXED

(Core :: Audio/Video, defect)

Description

[Lucas Adamski [:ladamski]]

Fuzzing crash, requires CRC checks to be disabled as well.

Crash in th_comment_clear at info.c:117 :

for(i=0;i<_tc->comments;i++)_ogg_free(_tc->user_comments[i]); 

where _tc->comments == 28197 but _tc->user_comments == NULL

#0	0x120d2fc7 in th_comment_clear at info.c:117
#1	0x120cf39d in oc_dec_headerin at decinfo.c:188
#2	0x120cf4bb in th_decode_headerin at decinfo.c:230
#3	0x120cf8f7 in theora_decode_header at decapiwrapper.c:145
#4	0x120ba1c1 in oggplay_callback_theora at oggplay_callback.c:106
#5	0x120bae72 in oggplay_callback_predetected at oggplay_callback.c:652
#6	0x120c809c in oggz_read_sync at oggz_read.c:486
#7	0x120c8469 in oggz_read at oggz_read.c:606
#8	0x120b8f02 in oggplay_initialise at oggplay.c:122
#9	0x120b8fb3 in oggplay_open_with_reader at oggplay.c:159
#10	0x120a7fc0 in nsOggDecodeStateMachine::LoadOggHeaders at nsOggDecoder.cpp:1752
#11	0x120ac669 in nsOggDecodeStateMachine::Run at nsOggDecoder.cpp:1422
#12	0x005759e4 in nsThread::ProcessNextEvent at nsThread.cpp:510
#13	0x004fe968 in NS_ProcessNextEvent_P at nsThreadUtils.cpp:227
#14	0x00575bf3 in nsThread::ThreadFunc at nsThread.cpp:254
#15	0x00728465 in _pt_root at ptthread.c:228
#16	0x96291155 in _pthread_start
#17	0x96291012 in thread_start

Comment 1

[Lucas Adamski [:ladamski]]

Attachment: crash repro... if you have CRC checks disabled

Comment 2

[cajbir (:cajbir)]

This doesn't crash on 1.9.1 for me with crc checks disabled.

Comment 3

[Lucas Adamski [:ladamski]]

Attachment: another example

Comment 4

[Lucas Adamski [:ladamski]]

Attachment: another example

Comment 5

[Lucas Adamski [:ladamski]]

Attachment: another example

Comment 6

[cajbir (:cajbir)]

None of these crash for me - can you confirm you're using latest 1.9.1 branch?

Note that they do crash in oggplayer - with the same stack trace except for the one from comment 5 which is the usual vorbis_synthesis crash which we've fixed.

Comment 7

[cajbir (:cajbir)]

Attachment: Fix

I can't reproduce the bug in mozilla-1.9.1 possibly because of recent patches that went through that bail out on certain header errors. Or maybe I didn't get the checksum disabling correct. 

I can reproduce the bug in oggplayer (which doesn't have those patches) so I've taken the fix that got oggplayer working. If someone who can reproduce the crash can test that would be great.

The fix was obtained from thusnelda svn (codename for next release of theora) and was pointed to me by Tim as the fix for the issue.

Comment 8

[Lucas Adamski [:ladamski]]

Hmm, yeah these still crash on the latest build of 1.9.1 for me (just pulled and built again).  On both Linux and OS X.  All should be the same crash as well; I get:

video-1frag.ogg.3.ogg 0x1356cfc7 in th_comment_clear (_tc=0x1d4548d0) at /hg/moz
illa-1.9.1/media/libtheora/lib/dec/info.c:117
video-1frag.ogg.4.ogg 0x1356cfc7 in th_comment_clear (_tc=0x1d4508c0) at /hg/moz
illa-1.9.1/media/libtheora/lib/dec/info.c:117
video-1frag.ogg.5.ogg 0x1356cfc7 in th_comment_clear (_tc=0x1d451c30) at /hg/moz
illa-1.9.1/media/libtheora/lib/dec/info.c:117
video-1frag.ogg.6.ogg 0x1356cfc7 in th_comment_clear (_tc=0x1d451160) at /hg/moz
illa-1.9.1/media/libtheora/lib/dec/info.c:117

Comment 9

[cajbir (:cajbir)]

Is that with the patch in comment 7 applied?

Comment 10

[Mike Beltzner [:beltzner, not reading bugmail]]

Blocking as per shaver, like the others.

Comment 11

[Lucas Adamski [:ladamski]]

Sorry, I missed your earlier question.  I applied the patch and it seems to have address the crashing though CPU stays pegged (esp. if I open two or more of those repros together).

Comment 12

[Martijn Wargers (dead)]

(In reply to comment #10)
> Blocking as per shaver, like the others.

Why? These are crashers that don't seem to happen normally.

Comment 13

[cajbir (:cajbir)]

I managed to get the browser to crash without the patch - I commented out the crc check in the wrong file (it needs to be ogg_framing.c, not framing.c which is not needed and is removed in another bug to be landed). 

None of the files in this bug crash with the patch applied and the CPU is not pegged. The files appear as invalid by showing the 'broken media' symbol (An X in a grayish background).

Comment 14

[cajbir (:cajbir)]

Attachment: liboggplay side of fix

This patch handles the return value from libtheora, propogating it down to liboggz so decoding is aborted.

Combined with attachment 383640 [details] [diff] [review] this fixes the crash and 100% cpu pegging.

Comment 15

[cajbir (:cajbir)]

Viktor, if you could cast your eye over attachment 383705 [details] [diff] [review] for the liboggplay fix it would be appreciated, thanks.

Comment 16

[Viktor Gal]

hmm I wanted to comment as a review, but yeah i dont have permission. both attachment 383705 [details] [diff] [review] and  attachment 383640 [details] [diff] [review] are fine - i.e. necessary - and shall be applied.

Comment 17

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 383640 [details] [diff] [review]
Fix

This patch is from Tim and is as reviewed as it can be

Comment 18

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 383705 [details] [diff] [review]
liboggplay side of fix

reviewed by Viktor

Comment 19

[Jeff Walden [:Waldo]]

(In reply to comment #16)
> hmm I wanted to comment as a review, but yeah i dont have permission.

You have the right Bugzilla bits to do so now, as well as to file bugs as NEW rather than UNCONFIRMED.

Comment 20

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Thanks Waldo!

Comment 21

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Testcase - crashes browser

This is Lucas' testcase 1 with CRCs corrected, crashes browser without "disable CRC check patch" applied.

Comment 22

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Testcase 2 - crashes browser

This is Lucas' testcase 2 with CRCs corrected, crashes browser without "disable CRC check patch" applied.

Comment 23

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Testcase 3 - crashes browser

This is Lucas' testcase 3 with CRCs corrected, crashes browser without "disable CRC check patch" applied.

Comment 24

[cajbir (:cajbir)]

Pushed to mozilla-central:
http://hg.mozilla.org/mozilla-central/rev/1a24562b09ad
http://hg.mozilla.org/mozilla-central/rev/aebc04ab967d

Comment 25

[cajbir (:cajbir)]

Pushed to mozilla-1.9.1:
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/75c41fb0ddc3
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/934118b76ecf

Comment 26

[Aaron Train [:aaronmt]]

VERIFIED Fixed on latest 1.9.1 branch

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1pre) Gecko/20090623 Shiretoko/3.5pre

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1pre) Gecko/20090623 Shiretoko/3.5pre

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1pre) Gecko/20090623 Shiretoko/3.5pre