Closed Bug 481601 Opened 13 years ago Closed 13 years ago

Crash in [@ _vorbis_block_ripcord - vorbis_block_clear]

Categories

(Core :: Audio/Video, defect)

defect
Not set
critical

Tracking

()

VERIFIED FIXED

People

(Reporter: roc, Unassigned)

Details

(Keywords: crash, testcase, verified1.9.1)

Crash Data

Attachments

(2 files, 2 obsolete files)

Attached video 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
(It crashes when I close the window, not immediately on load)
Severity: normal → critical
Summary: Crash in vorbis_block_clear → Crash in [@ _vorbis_block_ripcord - vorbis_block_clear]
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.
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.
Attached patch Patch rev. 1 (obsolete) — Splinter Review
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?
Attachment #366147 - Flags: review?
Flags: blocking1.9.2?
Flags: blocking1.9.1?
OS: Mac OS X → All
Hardware: x86 → All
Attached patch Patch rev. 1 (obsolete) — Splinter Review
Now with all files.
Attachment #366147 - Attachment is obsolete: true
Attachment #366150 - Flags: review?
Attachment #366147 - Flags: review?
Conrad Parker is the libfishsound author, I've cc'd him on the bug.
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 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!!
Attachment #366150 - Attachment is obsolete: true
Attachment #366217 - Flags: review+
The following changes have been made to the user account conrad@metadecks.org:
    * 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.
This should be blocking1.9.1, yes?
Pushed to mozilla-central:
http://hg.mozilla.org/mozilla-central/rev/203815a88708
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Whiteboard: [baking for 1.9.1]
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.
Flags: wanted1.9.1+
Flags: blocking1.9.1?
Flags: blocking1.9.1-
Can I get approval1.9.1+ on the patch to land on 1.9.1?
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
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
Status: RESOLVED → VERIFIED
This bug needs a test.
Flags: in-testsuite?
Crash Signature: [@ _vorbis_block_ripcord - vorbis_block_clear]
You need to log in before you can comment on or make changes to this bug.