Closed Bug 498853 Opened 13 years ago Closed 13 years ago

Crash in vorbis_comment_clear


(Core :: Audio/Video, defect)

1.9.1 Branch
Not set





(Reporter: ladamski, Assigned: cajbir)


(Keywords: crash, testcase, verified1.9.1, Whiteboard: [sg:dos])


(3 files, 2 obsolete files)

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
Flags: blocking1.9.1?
Whiteboard: [sg:critical]
Flags: blocking1.9.1? → blocking1.9.1+
Attached patch FixSplinter Review
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.
Assignee: nobody → chris.double
Isn't this one [sg:dos] if it's a guaranteed null deref?
Who's going to review this?

I agree this is a null deref, not critical.
Whiteboard: [sg:critical] → [sg:critical][needs review]
Updating sg: tag, deblocking.
Flags: blocking1.9.1+ → blocking1.9.1-
Whiteboard: [sg:critical][needs review] → [sg:dos][needs review]
Attachment #383736 - Flags: review?(monty)
This is Lucas' testcase 1 with CRCs corrected, crashes browser without "disable
CRC check patch" applied.
Attachment #383671 - Attachment is obsolete: true
This is Lucas' testcase 2 with CRCs corrected, crashes browser without "disable
CRC check patch" applied.
Attachment #383672 - Attachment is obsolete: true
Reviewed by monty on irc. Pushed to mozilla-central:
Closed: 13 years ago
Resolution: --- → FIXED
Whiteboard: [sg:dos][needs review] → [sg:dos]
Attachment #383736 - Flags: review?(monty) → review+
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?
Severity: blocker → critical
Flags: in-testsuite?
Keywords: crash
OS: Mac OS X → All
Hardware: x86 → All
Target Milestone: --- → mozilla1.9.2a1
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! :)
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.
Flags: wanted1.9.0.x-
Group: core-security
You need to log in before you can comment on or make changes to this bug.