498855 - Crash in vorbis_synthesis

Status: VERIFIED FIXED

(Core :: Audio/Video, defect)

Description

[Lucas Adamski [:ladamski]]

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

Lots of crashes for this one.  I know we had a previous vorbis_synthesis issue but this one seems to be different / previously protected by the CRC checks.

#0	0x135828df in vorbis_synthesis at vorbis_synthesis.c:30
#1	0x1354fdec in fs_vorbis_decode at fishsound_vorbis.c:158
#2	0x1354f132 in fish_sound_decode at fishsound_decode.c:117
#3	0x13554b0f in oggplay_callback_audio at oggplay_callback.c:391
#4	0x13561a99 in oggz_read_deliver_packet at oggz_read.c:298
#5	0x13560ce1 in oggz_dlist_deliter at oggz_dlist.c:183
#6	0x13561f89 in oggz_read_sync at oggz_read.c:462
#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

[Lucas Adamski [:ladamski]]

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

Comment 3

[cajbir (:cajbir)]

This particular stack trace is bug 496684 which has langed on trunk. Applying this patch to 1.9.1 fixes two of these files but one still crashes. Looking into it.

Comment 4

[cajbir (:cajbir)]

Attachment: Fix

This patch fixes the crash and stops the files from playing by checking for a null value and returning an error. It requires bug 496684 to be landed (it's on trunk but not branch) for liboggplay to detect the return value and stop playing.

The underlying issue is due to problems that liboggplay has with Ogg headers. A complete fix is pending from the liboggplay maintainers for the header issues - this is a continuing issue from bug 496684 and the bug that was spawned from.

Comment 5

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

Null derefs, not sg:critical.

Comment 6

[Mike Shaver (:shaver emeritus)]

Updating sg: tag per roc's comment, also not a blocker for 1.9.1 if there isn't security exposure.

Comment 7

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

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

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

[cajbir (:cajbir)]

Pushed to mozilla-central:
http://hg.mozilla.org/mozilla-central/rev/1271d9a47bfb

Comment 11

[cajbir (:cajbir)]

Reviewed by monty on irc.

Comment 12

[cajbir (:cajbir)]

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

Comment 13

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 07/21 - 08/08)]

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

Comment 14

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

Attachment: Patch - add test

Patch - adds testcase which ensures that the 3 testcase files here don't load, they throw an error. This is rebased on top of the "Patch - add test" in bug 499519.

Comment 15

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

Testcase pushed to m-c:
http://hg.mozilla.org/mozilla-central/rev/0234cf1eee13

Comment 16

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 07/21 - 08/08)]

Chris, will those tests also be pushed to 1.9.1? And can we open this bug to the public now?

Comment 17

[Martijn Wargers (dead)]

Is the patch included in a public release? If it's not, the bug can't be opened up. And it seems to me the test shouldn't have been checked in yet, but probably Chris knows better and it's no problem the test has been checked in yet.

Comment 18

[Martijn Wargers (dead)]

Oh, this was a null deref, so no security problem, and apparently, the bug has become public now, at this time of writing. The tests can also be pushed to 1.9.1, but whether they will get pushed to 1.9.1 depends on if someone pushes them to that release.

Comment 19

[cajbir (:cajbir)]

Unfortunately 1.9.1 has a different test harness structure so the test will need to be redone for 1.9.1.