322045 - CVE-2006-0293 GC hazard during function allocation.

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Igor Bukanov]

js_NewFunction from jsfun.c contains the following code:

    /* Allocate a function struct. */
     fun = (JSFunction *) js_NewGCThing(cx, GCX_PRIVATE,
sizeof(JSFunction));
     if (!fun)
         return NULL;

     /* If funobj is null, allocate an object for it. */
     if (funobj) {
         OBJ_SET_PARENT(cx, funobj, parent);
     } else {
         funobj = js_NewObject(cx, &js_FunctionClass, NULL, parent);

Since js_NewObject also allocates slots using GCX_PRIVATE, it will remove
"fun" from "cx->newborn[GCX_PRIVATE]". If JS embedding installs JSRuntime.objectHook which triggers GC during "funobj" allocation, "fun" would be GC-ed when js_NewObject returns to js_NewFunction.

This is one possible reason for bug 317815.

Comment 1

[Igor Bukanov]

Attachment: Fix via code rearangment

Comment 2

[Igor Bukanov]

Lets see if the fix helps to address bug 317815 or that was caused by something else.

Comment 3

[Brendan Eich [:brendan]]

Marking up bug to get on release tracks, hope you don't mind.

/be

Comment 4

[Brendan Eich [:brendan]]

Comment on attachment 207296 [details] [diff] [review]
Fix via code rearangment

>+    /*
>+     * Allocate a function struct after allocating funobj so slot allocation
>+     * in js_NewObject would not wipe out fun from cx->newborn[GCX_PRIVATE].
>+     */
>+

Nit: no need for blank line after major comment.

Great, simple fix for likely bug.  Nominating for branches.  Thanks,

/be

Comment 5

[Bob Clary [:bc] (inactive)]

with the patch from bug 322001 and this patch I no longer crash using a dbg trunk build on winxp without venkman with the test URL in bug 317815.

I don't know if this is related to this patch, but installing venkman and shutting down causes the following 

###!!! ASSERTION: CheckForDeactivation called from wrong thread!: 'Error', file c:/work/mozilla/builds/ff/trunk-test/mozilla/xpcom/t
hreads/nsEventQueue.cpp, line 255

        /*
         * If this ASSERT fails, a bad pointer has been passed in. It may be
         * totally bogus, or it may have been allocated from another heap.
         * The pointer MUST come from the 'local' heap.
         */
        _ASSERTE(_CrtIsValidHeapPointer(pUserData));

_free_dbg_lk(void * 0x03651420, int 0x00000001) line 1044 + 48 bytes
_free_dbg(void * 0x03651420, int 0x00000001) line 1001 + 13 bytes
free(void * 0x03651420) line 956 + 11 bytes
js_FinalizeStringRT(JSRuntime * 0x00fb0068, JSString * 0x02d8c538) line 2715 + 13 bytes
js_atom_uninterner(JSHashEntry * 0x03651550, int 0x00000a72, void * 0x0012fb00) line 403 + 21 bytes
JS_HashTableEnumerateEntries(JSHashTable * 0x0104d538, int (JSHashEntry *, int, void *)* 0x00438c20 js_atom_uninterner(JSHashEntry *, int, void *), void * 0x0012fb00) line 362 + 15 bytes
js_FinishAtomState(JSAtomState * 0x00fb0204) line 418 + 21 bytes
JS_Finish(JSRuntime * 0x00fb0068) line 737 + 15 bytes
nsJSRuntimeServiceImpl::~nsJSRuntimeServiceImpl() line 93 + 13 bytes
nsJSRuntimeServiceImpl::`scalar deleting destructor'(unsigned int 0x00000001) + 15 bytes
nsJSRuntimeServiceImpl::Release(nsJSRuntimeServiceImpl * const 0x01030000) line 103 + 141 bytes
nsJSRuntimeServiceImpl::FreeSingleton() line 126 + 26 bytes
xpcModuleDtor(nsIModule * 0x0102e430) line 135
nsGenericModule::Shutdown() line 339 + 10 bytes
nsGenericModule::~nsGenericModule() line 242
nsGenericModule::`scalar deleting destructor'(unsigned int 0x00000001) + 15 bytes
nsGenericModule::Release(nsGenericModule * const 0x0102e430) line 244 + 139 bytes
nsCOMPtr<nsIModule>::assign_assuming_AddRef(nsIModule * 0x00000000) line 569
nsCOMPtr<nsIModule>::assign_with_AddRef(nsISupports * 0x00000000) line 1225
nsCOMPtr<nsIModule>::operator=(nsIModule * 0x00000000) line 714
nsNativeModuleLoader::ReleaserFunc(nsIHashable * 0x0102e17c, nsNativeModuleLoader::NativeLoadData & {...}, void * 0x00000000) line 211
nsBaseHashtable<nsHashableHashKey,nsNativeModuleLoader::NativeLoadData,nsNativeModuleLoader::NativeLoadData>::s_EnumStub(PLDHashTable * 0x003fb720, PLDHashEntryHdr * 0x01d47ee8, unsigned int 0x00000015, void * 0x0012fc7c) line 346 + 28 bytes
PL_DHashTableEnumerate(PLDHashTable * 0x003fb720, int (PLDHashTable *, PLDHashEntryHdr *, unsigned int, void *)* 0x002e2310 nsBaseHashtable<nsHashableHashKey,nsNativeModuleLoader::NativeLoadData,nsNativeModuleLoader::NativeLoadData>::s_EnumStub(PLDHashTable *, PLDHashEntryHdr *, unsigned int, void *), void * 0x0012fc7c) line 621 + 34 bytes
nsBaseHashtable<nsHashableHashKey,nsNativeModuleLoader::NativeLoadData,nsNativeModuleLoader::NativeLoadData>::Enumerate(PLDHashOperator (nsIHashable *, nsNativeModuleLoader::NativeLoadData &, void *)* 0x002e1e50 nsNativeModuleLoader::ReleaserFunc(nsIHashable *, nsNativeModuleLoader::NativeLoadData &, void *), void * 0x00000000) line 221 + 18 bytes
nsNativeModuleLoader::UnloadLibraries() line 250
nsComponentManagerImpl::Shutdown() line 800
NS_ShutdownXPCOM_P(nsIServiceManager * 0x00000000) line 826 + 11 bytes
ScopedXPCOMStartup::~ScopedXPCOMStartup() line 553 + 12 bytes
XRE_main(int 0x00000003, char * * 0x003f79d8, const nsXREAppData * 0x0040301c kAppData) line 2337
main(int 0x00000003, char * * 0x003f79d8) line 61 + 19 bytes
mainCRTStartup() line 338 + 17 bytes
KERNEL32! 7c816d4f()

in js_FinalizeStringRT(JSRuntime * 0x00fb0068, JSString * 0x02d8c538) line 2715 + 13 bytes

+	str->chars	0x03651420 "file:///C:/work/mozilla/builds/ff/trunk-test/mozilla/extensions/venkman/xpi/venkman-0.9.85.xpi"
	valid	0x00000001

with venkman installed, I now longer crash immediately nor on shutdown in bug 317815.

Comment 6

[Brendan Eich [:brendan]]

Bob: can you test with just this bug's patch, and not the patch for bug 322001?  The crash you show looks like it might be a regression from that bug's patch.  This bug's patch should not cause any such crash.

/be

Comment 7

[Bob Clary [:bc] (inactive)]

(In reply to comment #6)

I cleaned out my tree and still see the heap assert when closing the browser after installing venkman so this is an existing issue not caused by bug 322001. I tried to reproduce with another extension and could not although the ASSERTION: CheckForDeactivation called from wrong thread! was still present. I looked for other bugs on this but couldn't find an exact match but Bug 155245 has the same mozilla assert.

Comment 8

[Brendan Eich [:brendan]]

(In reply to comment #7)
> (In reply to comment #6)
> 
> I cleaned out my tree and still see the heap assert when closing the browser
> after installing venkman so this is an existing issue not caused by bug 322001.

Please file a new bug.

> I tried to reproduce with another extension and could not although the
> ASSERTION: CheckForDeactivation called from wrong thread! was still present. I
> looked for other bugs on this but couldn't find an exact match but Bug 155245
> has the same mozilla assert.

Could you comment in that bug?  It seems old and stale, perhaps something has changed to make it bite anew, or possibly its symptom that you see deserves a new bug, but for now that one will do.  Timeless may be able to help.

Just to confirm: with only this bug's patch applied, you can't reproduce the crash reported in bug 317815?  And without this bug's patch, you can?

/be

Comment 9

[Bob Clary [:bc] (inactive)]

(In reply to comment #8)

> 
> Please file a new bug.
> 

Bug 322131 

> 
> Just to confirm: with only this bug's patch applied, you can't reproduce the
> crash reported in bug 317815?  And without this bug's patch, you can?

Yes and Yes.

Comment 10

[Brendan Eich [:brendan]]

Bob: great, thanks -- and thanks to eagle-eyed Igor.  This is a no-brainer for 1.8.0.1.  I'm checking Igor's patch into the trunk immediately.

/be

Comment 11

[Brendan Eich [:brendan]]

Attachment: version of Igor's patch that I committed

Comment 12

[Brendan Eich [:brendan]]

Fixed on trunk.  Nominations for the patch should keep this bug on the radar until the patch is approved for landing.

/be

Comment 13

[Brendan Eich [:brendan]]

Anyone see any js_NewGCThing talkback from trunk builds newer than the day this patch went into the trunk?

/be

Comment 14

[Bob Clary [:bc] (inactive)]

(In reply to comment #13)

No. I see 4 with js_NewGCThing in the signature for builds from 20051229 and 20051218, but the small number does not yet make me comfortable. I'll try to keep up with it.

Comment 15

[Brendan Eich [:brendan]]

This patch fixed bug 312588.  It's a pure fix for real bugs.  I say it's baked.

/be

Comment 16

[Daniel Veditz [:dveditz]]

Comment on attachment 207390 [details] [diff] [review]
version of Igor's patch that I committed

a=dveditz for drivers.
do we need this for the 1.7 branch as well?

Comment 17

[Brendan Eich [:brendan]]

(In reply to comment #16)
> (From update of attachment 207390 [details] [diff] [review] [edit])
> a=dveditz for drivers.
> do we need this for the 1.7 branch as well?

1.7 and Aviary 1.0.1 branches, you mean? :-P

No, this depends on allocation of JSFunction and object slots via the GC, which came in during 1.8.

/be

Comment 18

[Brendan Eich [:brendan]]

Fixed on branches.

/be

Comment 19

[Marcia Knous [:marcia]]

Is there a way for QA to verify this on the branch?

Comment 20

[Bob Clary [:bc] (inactive)]

from talkback: last trunk builds with js_NewGCThing signatures are 20051230 or earlier, last 1.5.x builds from 2006 with js_NewGCThing signatures are 20060109 or earlier.

Comment 21

[Bob Clary [:bc] (inactive)]

v by bonsai:

2006-01-02 17:41	brendan%mozilla.org 	mozilla/js/src/jsfun.c 	3.131 	8/5  	Fix from Igor Bukanov <[e-mail]> to reorder allocations to avoid pigeon-hole problem (322045, r=me).

2006-01-10 17:15	brendan%mozilla.org 	mozilla/js/src/jsfun.c 	3.117.2.9 	MOZILLA_1_8_BRANCH  	8/5  	Fix for 322045, a=dveditz.

2006-01-10 17:15	brendan%mozilla.org 	mozilla/js/src/jsfun.c 	3.117.2.7.2.2 	MOZILLA_1_8_0_BRANCH  	8/5 

Comment 22

[Rich Painter]

Advisory 2006-01 http://www.mozilla.org/security/announce/mfsa2006-01.html indicates that this flaw is also in Mozilla but no references seem to be here about fixing this.  This Advisory does indicate to disable javascript in Mail which is commonly done and prudent.  However, it does not address this for Browser and javascript is needed by just about everyone to access at least one web site.  Hence, disabling this in Browser to work around this flaw is impractical.

Is someone fixing this for Mozilla Suite and when will it be available?

thanks,
r