498853 - Crash in vorbis_comment_clear

Status: VERIFIED FIXED

(Core :: Audio/Video, defect)

Description

[Lucas Adamski [:ladamski]]

Attachment: repro.. requires CRC checks to be turned off

Another fuzzing bug that requires ogg CRC checks to be disabled.

Program received signal EXC_BAD_ACCESS, Could not access memory.
Reason: KERN_PROTECTION_FAILURE at address: 0x00000000
[Switching to process 41557 thread 0x6103]
0x1357dfaa in vorbis_comment_clear (vc=0x1d44dac8) at /hg/mozilla-1.9.1/media/libvorbis/lib/vorbis_info.c:134
134           if(vc->user_comments[i])_ogg_free(vc->user_comments[i]);


#0	0x1357dfaa in vorbis_comment_clear at vorbis_info.c:134
#1	0x1357e604 in _vorbis_unpack_comment at vorbis_info.c:257
#2	0x1357ed15 in vorbis_synthesis_headerin at vorbis_info.c:407
#3	0x1354fcca in fs_vorbis_decode at fishsound_vorbis.c:131
#4	0x1354f132 in fish_sound_decode at fishsound_decode.c:117
#5	0x13554b0f in oggplay_callback_audio at oggplay_callback.c:391
#6	0x1356205e in oggz_read_sync at oggz_read.c:483
#7	0x13562469 in oggz_read at oggz_read.c:606
#8	0x13552f02 in oggplay_initialise at oggplay.c:122
#9	0x13552fb3 in oggplay_open_with_reader at oggplay.c:159
#10	0x13541fc0 in nsOggDecodeStateMachine::LoadOggHeaders at nsOggDecoder.cpp:1752
#11	0x13546669 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: repro.. requires CRC checks to be turned off

Comment 2

[cajbir (:cajbir)]

Attachment: Fix

The comment reading code bails out before actually reading any. This results in 
vc->user_comments being NULL. There's a check for it following the code that indexes into it. I just moved the check to before the indexing is done.

Both files no longer crash.

Comment 3

[Daniel Veditz [:dveditz]]

Isn't this one [sg:dos] if it's a guaranteed null deref?

Comment 4

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

Who's going to review this?

I agree this is a null deref, not critical.

Comment 5

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Updating sg: tag, deblocking.

Comment 6

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

Attachment: Testcase 1 - crashes browser

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

Comment 7

[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 8

[cajbir (:cajbir)]

Reviewed by monty on irc. Pushed to mozilla-central:
http://hg.mozilla.org/mozilla-central/rev/592ca0329827

Comment 9

[cajbir (:cajbir)]

a=beltzner via email
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/e0bc2e84f900

Comment 10

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Verified fixed on trunk and 1.9.1 with builds on all platforms like

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090623 Minefield/3.6a1pre ID:20090623035831

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1) Gecko/20090624 Firefox/3.5 ID:20090624012136

Chris, do we have a test which can check all those strange ogg files in a row?

Comment 11

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

Yup, it should be pretty easy to add these as crashtests. I think we should probably wait until Lucas stops finding them, just so that our checkin logs don't give the bad guys any ideas! :)

Comment 12

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

I think we should probably land all testcases that are fixed in 1.9.1. The only thing they really expose is that we're doing fuzzing, but that won't be news to anyone.

However, I'm planning to refactor tests shortly (as soon as my directory reorg is reviewed). Then I'll create a harness to crashtest media files in parallel.