481921 - Crash [@ntdll:RtlpCoalesceFreeBlocks]

Status: VERIFIED FIXED

(Core :: Audio/Video, defect, P2)

Description

[Paul Nickerson]

Attachment: Corrupted file

This bug definitely looks like it may be exploitable:

Heap corruption detected at 059287D8
HEAP[firefox.exe]: HEAP: Free Heap block 59287d0 modified at 59287e0 after it was freed
(368.98): Break instruction exception - code 80000003 (first chance)
eax=059287d0 ebx=00c70000 ecx=7c91eb05 edx=0978f994 esi=059287d0 edi=059286f0
eip=7c901230 esp=0978fb94 ebp=0978fb98 iopl=0         nv up ei pl nz na po nc
cs=001b  ss=0023  ds=0023  es=0023  fs=003b  gs=0000             efl=00000202
ntdll!DbgBreakPoint:
7c901230 cc              int     3

I titled the bug after the function at the top of the stack, but a more accurate title may be Crash [@gklayout:oggplay_data_clean_list]. Somebody may change it if they see fit.

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20090306 Minefield/3.2a1pre

Comment 1

[Paul Nickerson]

Attachment: Stack trace

Comment 2

[Paul Nickerson]

Attachment: Open this

Comment 3

[Paul Nickerson]

This is a weird bug. If it doesn't crash right away when playing the video, use the browser normally for a bit and it will crash. I always crash right away, however, when using the debug build.

Comment 4

[Benjamin Smedberg]

Is the stack always the same? Crashes in RtlpCoalesceFreeBlocks generally indicate malloc heap corruption, and may not have anything to do with the current callsite.

If it is the same, we're likely double-freeing memory.

In any case, valgrind is probably the best way to diagnose the problem, if it's reproducable on Linux. cc'ing some people who have valgrind setups.

Comment 5

[Paul Nickerson]

The stack changes around. I pasted another stack trace at the bottom of this message, and it doesn't look like it has any useful information about the actual bug. Btw, does anybody think this is a duplicate of bug 480863?

0012b4b4 7c96c943 ntdll!DbgBreakPoint
0012b4bc 7c96cd80 ntdll!RtlpBreakPointHeap+0x28
0012b4d0 7c960af8 ntdll!RtlpValidateHeapEntry+0x113
0012b534 7c85e9cf ntdll!RtlValidateHeap+0xe0
0012b548 102115ba kernel32!HeapValidate+0x14
0012b570 102107a6 MSVCR80D!_CrtIsValidHeapPointer(void * pUserData = 0x0ae1ae58)+0x15a
0012b580 1021065e MSVCR80D!_free_dbg_nolock(void * pUserData = 0x0ae1ae58, int nBlockUse = 1)+0x116
0012b5b8 102527b7 MSVCR80D!_free_dbg(void * pUserData = 0x0ae1ae58, int nBlockUse = 1)+0x4e
0012b5f4 03340d30 MSVCR80D!operator delete(void * pUserData = 0x0ae1ae58)+0xb7
0012b604 02eac467 gklayout!nsHTMLDivElement::`scalar deleting destructor'(void)+0x20
0012b658 02e9b838 gklayout!nsNodeUtils::LastRelease(class nsINode * aNode = 0x0ae1ae58)+0x247
0012b670 03340ddd gklayout!nsGenericElement::Release(void)+0xc8
0012b680 00287048 gklayout!nsHTMLDivElement::Release(void)+0xd
0012b690 00303188 xpcom_core!nsXPCOMCycleCollectionParticipant::Unroot(void * p = 0x0ae1ae58)+0x18
0012b6b8 00303c51 xpcom_core!nsCycleCollector::CollectWhite(void)+0xc8
0012b6cc 00303eb8 xpcom_core!nsCycleCollector::FinishCollection(void)+0x11
0012b6d8 010fca58 xpcom_core!nsCycleCollector_finishCollection(void)+0x18
0012b6e8 004daa55 xpc3250!XPCCycleCollectGCCallback(struct JSContext * cx = 0x03ec8138, JSGCStatus status = JSGC_END (1))+0xb8
0012b7c0 00489def js3250!js_GC(struct JSContext * cx = 0x03ec8138, JSGCInvocationKind gckind = GC_NORMAL (0))+0xf75
0012b7d0 010fc8df js3250!JS_GC(struct JSContext * cx = 0x03ec8138)+0x5f
0012b888 0030396d xpc3250!nsXPConnect::Collect(void)+0xcf
0012f748 00303e2a xpcom_core!nsCycleCollector::Collect(unsigned int aTryCollections = 1)+0xfd
0012f758 0302759b xpcom_core!nsCycleCollector_collect(void)+0x1a
0012f760 030276a0 gklayout!nsJSContext::CC(void)+0x3b
0012f774 030276f3 gklayout!nsJSContext::MaybeCC(int aHigherProbability = 1)+0xd0
0012f780 0302774f gklayout!nsJSContext::CCIfUserInactive(void)+0x13
0012f788 002f2a7e gklayout!GCTimerFired(class nsITimer * aTimer = 0x0b2877d8, void * aClosure = 0x00000000)+0x4f
0012f7dc 002f2c61 xpcom_core!nsTimerImpl::Fire(void)+0x28e
0012f7f4 002f488a xpcom_core!nsTimerEvent::Run(void)+0xa1
0012f830 00286813 xpcom_core!nsThread::ProcessNextEvent(int mayWait = 1, int * result = 0x0012f848)+0x1fa
0012f84c 0198679d xpcom_core!NS_ProcessNextEvent_P(class nsIThread * thread = 0x00c7fe40, int mayWait = 1)+0x53
0012f860 01be41db gkwidget!nsBaseAppShell::Run(void)+0x5d
0012f874 1000cb75 tkitcmps!nsAppStartup::Run(void)+0x6b
0012fed0 00401ac2 xul!XRE_main(int argc = 1, char ** argv = 0x00c78558, struct nsXREAppData * aAppData = 0x00c78ad0)+0x3385
0012ff34 00401289 firefox!NS_internal_main(int argc = 1, char ** argv = 0x00c78558)+0x2b2
0012ff68 00402746 firefox!wmain(int argc = 1, unsigned short ** argv = 0x00c784c8)+0x119
0012ffb8 0040259d firefox!__tmainCRTStartup(void)+0x1a6
0012ffc0 7c816fd7 firefox!wmainCRTStartup(void)+0xd
0012fff0 00000000 kernel32!BaseProcessStart+0x23

Comment 6

[Robert Sayre]

Attachment: ugly valgrind stacks

I've never seen the first one before, it's in the ogg code

Comment 7

[Mats Palmgren (inactive)]

Attachment: WIP (diff -w)

Fixing the invalid write in oggplay_callback_info_prepare() fixed the
crash for me.  The problem is that 'count' is incremented for
"!has_been_presented" data:
http://mxr.mozilla.org/mozilla-central/source/media/liboggplay/src/liboggplay/oggplay_callback_info.c#86

'count' is then used to allocate records:
http://mxr.mozilla.org/mozilla-central/source/media/liboggplay/src/liboggplay/oggplay_callback_info.c#130

but in the loop at line 140 we iterate all data from 'q' (the first
which was "!has_been_presented") to the end, so if there are any that
was "has_been_presented" after 'q' then we will write outside the buffer.

I'm not familiar with this code so I don't know if this is the correct fix...

Comment 8

[cajbir (:cajbir)]

Added liboggplay maintainer to CC to take a look.

Comment 9

[cajbir (:cajbir)]

Test case in attachment 365966 [details] doesn't crash for me on trunk btw.

Comment 10

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

Any update here?

Comment 11

[Mats Palmgren (inactive)]

Attachment: crashtest.diff

Comment 12

[Mats Palmgren (inactive)]

Attachment: Patch rev. 1

I have prepared a local patch.  I think we should take this until it's
fixed upstream.

Comment 13

[Viktor Gal]

the patch in comment 12 will solve things, but it should be taken as a temporal fix, as the problem is that the file has some packages where the granule position is not growing monotonically, which is against the specs of Ogg. so this should be avoided by not even processing these packets.

but that patch, that implements this simple 'filtering' will require some hacking in seeking (backward).

none the less, this patch should be taken in, and I think even when the prev mentioned patch is going to be ready should be kept, as a double-check.

Comment 14

[cajbir (:cajbir)]

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

Based on Viktor's review r+ for the changes. Can you also please add a line to update.sh to apply the patch so it doesn't get lost in the next liboggplay update.

Comment 15

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

Chris, can you take this over and land Mats' patch with appropriate changes to update.sh, or maybe get an upstream oggplay fix?

Comment 16

[Damon Sicore (:damons)]

Poke.  How we doing here, Chris?

Comment 17

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

http://hg.mozilla.org/mozilla-central/rev/c4a6d23c2ac9

Comment 18

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

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/9727000fd58c

Comment 19

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

Verified fixed on trunk and 1.9.1 with:

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

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1pre) Gecko/20090526 Shiretoko/3.5pre ID:20090526031155

Comment 21

[Jesse Ruderman]

I pushed the crashtest.diff that Mats created:
http://hg.mozilla.org/mozilla-central/rev/4056a8bf3fe5

Comment 22

[Jesse Ruderman]

... which might have caused orange bug 569105.