481601 - Crash in [@ _vorbis_block_ripcord - vorbis_block_clear]

Status: VERIFIED FIXED

(Core :: Audio/Video, defect)

Description

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

Attachment: testcase

The attached testcase crashes with the following stack:

#0  0x040ce440 in _vorbis_block_ripcord (vb=0x86a80c0) at /Users/roc/mozilla-checkin/media/libvorbis/lib/vorbis_block.c:141
#1  0x040ce4fd in vorbis_block_clear (vb=0x86a80c0) at /Users/roc/mozilla-checkin/media/libvorbis/lib/vorbis_block.c:163
#2  0x040a7bfc in fs_vorbis_delete (fsound=0x86a7f10) at /Users/roc/mozilla-checkin/media/libfishsound/src/libfishsound/fishsound_vorbis.c:459
#3  0x040a703b in fish_sound_delete (fsound=0x86a7f10) at /Users/roc/mozilla-checkin/media/libfishsound/src/libfishsound/fishsound.c:167
#4  0x040ac01b in oggplay_shutdown_audio (user_data=0x86a7490) at /Users/roc/mozilla-checkin/media/liboggplay/src/liboggplay/oggplay_callback.c:345
#5  0x040ac2f3 in oggplay_callback_shutdown (decoder=0x86a7490) at /Users/roc/mozilla-checkin/media/liboggplay/src/liboggplay/oggplay_callback.c:536
#6  0x040ab45a in oggplay_close (me=0x86a89f0) at /Users/roc/mozilla-checkin/media/liboggplay/src/liboggplay/oggplay.c:600
#7  0x0409f04b in nsOggDecodeStateMachine::~nsOggDecodeStateMachine (this=0x86a87c0) at /Users/roc/mozilla-checkin/content/media/video/src/nsOggDecoder.cpp:523
#8  0x004b5c6f in nsRunnable::Release (this=0x86a87c0) at nsThreadUtils.cpp:51
#9  0x040a064a in nsCOMPtr<nsOggDecodeStateMachine>::assign_assuming_AddRef (this=0x940ca70, newPtr=0x0) at nsCOMPtr.h:495
#10 0x040a067c in nsCOMPtr<nsOggDecodeStateMachine>::assign_with_AddRef (this=0x940ca70, rawPtr=0x0) at nsCOMPtr.h:1171
#11 0x040a0962 in nsCOMPtr<nsOggDecodeStateMachine>::operator= (this=0x940ca70, rhs=0x0) at nsCOMPtr.h:640
#12 0x040a0a1e in nsDestroyStateMachine::Run (this=0x940ca60) at /Users/roc/mozilla-checkin/content/media/video/src/nsOggDecoder.cpp:1448
#13 0x0052cb10 in nsThread::ProcessNextEvent (this=0xe169d0, mayWait=0, result=0xbfffd6f4) at /Users/roc/mozilla-checkin/xpcom/threads/nsThread.cpp:510

Comment 1

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

(It crashes when I close the window, not immediately on load)

Comment 2

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

The crash is

Program received signal EXC_BAD_ACCESS, Could not access memory.
Reason: KERN_PROTECTION_FAILURE at address: 0x00000024
0x040ce440 in _vorbis_block_ripcord (vb=0x86a80c0) at /Users/roc/mozilla-checkin/media/libvorbis/lib/vorbis_block.c:141
141	    struct alloc_chain *next=reap->next;

Looks like a pure null dereference.

Comment 3

[cajbir (:cajbir)]

This file plays in the liboggplay example player but does not exit. It does not play in the theora example player saying it cannot find the headers.

Comment 4

[Mats Palmgren (inactive)]

Attachment: Patch rev. 1

fs_vorbis_init() does not initialize the 'vd' or 'vb' members of
the FishSoundVorbisInfo struct.  fs_vorbis_delete() calls
vorbis_block_clear() and vorbis_dsp_clear() on random data.
http://mxr.mozilla.org/mozilla-central/source/media/libfishsound/src/libfishsound/fishsound_vorbis.c#419

Where do we put crash tests for media/* bugs?
Who does reviews?

Comment 5

[Mats Palmgren (inactive)]

Attachment: Patch rev. 1

Now with all files.

Comment 6

[cajbir (:cajbir)]

Conrad Parker is the libfishsound author, I've cc'd him on the bug.

Comment 7

[Conrad Parker]

The patch looks good; note that it is a patch to libvorbis, not just to libfishsound. It adds a new vorbis_dsp_init() function to libvorbis, and calls that from fs_vorbis_init().

This seems reasonable to me: these structures are normally initialized during one of the invocations of vorbis_synthesis_headerin(), but this file lacks a required header so that does not happen in this case.

(Technically the header data is present in the testcase, but is rejected by libogg -- perhaps the checksum is corrupt or something).

Comment 8

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

Comment on attachment 366150 [details] [diff] [review]
Patch rev. 1

I'll take that as an r+ from Conrad. In the future, I think it's fine for upstream library maintainers to r+ patches in our Bugzilla that apply to their libraries.

Chris, can you wrap this up as an in-tree libvorbis/libfishsound patch so we can check it in? Conrad, can you do whatever it takes to get this upstream? Thanks!!

Comment 9

[cajbir (:cajbir)]

Attachment: Packaged original bug with readme and update script

Comment 10

[:Gavin Sharp [email: [e-mail]]]

The following changes have been made to the user account [e-mail]:
    * The account has been added to the canconfirm, editbugs groups.

You should now be able to grant review and make other changes to attachments/bugs, Conrad.

Comment 11

[cajbir (:cajbir)]

This should be blocking1.9.1, yes?

Comment 12

[cajbir (:cajbir)]

Pushed to mozilla-central:
http://hg.mozilla.org/mozilla-central/rev/203815a88708

Comment 13

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

This shouldn't block 1.9.1 since it's not a security issue AFAIK and users wouldn't hit it in real life. Of course we should still take it on 1.9.1 though.

Comment 14

[cajbir (:cajbir)]

Can I get approval1.9.1+ on the patch to land on 1.9.1?

Comment 15

[Conrad Parker]

An update on this: there was resistance to the libvorbis changes upstream, so I've added the required vorbis_dsp_state initialization to libfishsound, in svn.annodex.net r3901. See the revision log for that for details:

https://trac.annodex.net/changeset/3901

Comment 16

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

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/209f931d0d75

Comment 17

[Aakash Desai [:aakashd]]

verified FIXED on builds: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090416 Minefield/3.6a1pre ID:20090416030845

and

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b4pre) Gecko/20090417 Shiretoko/3.5b4pre ID:20090417030722

Comment 18

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

This bug needs a test.