342677 - select.options.add(null) crashes [@ JS_GetClass]

Status: VERIFIED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[David D. Kilzer (ddk)]

Calling select.options.add(null) crashes Firefox 1.5.0.4 hard.  I'm not sure if this is actually a security issue or not, but I'll assume it is until proven otherwise.

I found this while testing Firefox's behavior while implementing the options.add() function in Apple's WebKit.

http://bugzilla.opendarwin.org/show_bug.cgi?id=9179

I will post a test case next.

Comment 1

[David D. Kilzer (ddk)]

Attachment: Test case (crashes Firefox 1.5.0.4)

This is a test case that crashes Firefox 1.5.0.4.

See also Talkback Incident TB20250155X.

Comment 2

[José Jeria]

It also crashes latest trunk build on Windows XP SP2 as well, updating summary

Comment 3

[timeless]

review blame is jst.

Comment 4

[timeless]

expected results (from ie6) are of course:
Invalid argument.

Comment 5

[Ria Klaassen (not reading all bugmail)]

The crash happened for the first time somewhere in August 2005.

Comment 6

[Alex Vincent [:WeirdAl]]

###!!! ASSERTION: bad param: 'aJSObj', file m:/vc8/mozilla/js/src/xpconnect/src/nsXPConnect.cpp, line 643

 	ntdll.dll!7c901230() 	
 	[Frames below may be incorrect and/or missing, no symbols loaded for ntdll.dll]	
 	xpcom_core.dll!Break(const char * aMsg=0x0012da1c)  Line 471	C++
 	xpcom_core.dll!NS_DebugBreak_P(unsigned int aSeverity=1, const char * aStr=0x00d07864, const char * aExpr=0x00d0785c, const char * aFile=0x00d07828, int aLine=643)  Line 350 + 0xc bytes	C++
 	xpc3250.dll!nsXPConnect::GetWrappedNativeOfJSObject(JSContext * aJSContext=0x033579f8, JSObject * aJSObj=0x00000000, nsIXPConnectWrappedNative * * _retval=0x0012df38)  Line 643 + 0x22 bytes	C++
>	gklayout.dll!nsHTMLOptionsCollectionSH::Add(JSContext * cx=0x033579f8, JSObject * obj=0x033efac8, unsigned int argc=1, long * argv=0x0365b104, long * rval=0x0012dfdc)  Line 9666 + 0x39 bytes	C++
 	js3250.dll!js_Invoke(JSContext * cx=0x033579f8, unsigned int argc=1, unsigned int flags=0)  Line 1328 + 0x20 bytes	C
 	js3250.dll!js_Interpret(JSContext * cx=0x033579f8, unsigned char * pc=0x0361cf1c, long * result=0x0012eb44)  Line 4021 + 0xf bytes	C
 	js3250.dll!js_Invoke(JSContext * cx=0x033579f8, unsigned int argc=1, unsigned int flags=2)  Line 1347 + 0x13 bytes	C
 	js3250.dll!js_InternalInvoke(JSContext * cx=0x033579f8, JSObject * obj=0x0334ec48, long fval=54463856, unsigned int flags=0, unsigned int argc=1, long * argv=0x0365b060, long * rval=0x0012ec98)  Line 1422 + 0x14 bytes	C
 	js3250.dll!JS_CallFunctionValue(JSContext * cx=0x033579f8, JSObject * obj=0x0334ec48, long fval=54463856, unsigned int argc=1, long * argv=0x0365b060, long * rval=0x0012ec98)  Line 4348 + 0x1f bytes	C
 	gklayout.dll!nsJSContext::CallEventHandler(nsISupports * aTarget=0x0350aca8, void * aScope=0x0334ec48, void * aHandler=0x033f0d70, nsIArray * aargv=0x0363a8e8, nsIVariant * * arv=0x0012ee0c)  Line 1655 + 0x21 bytes	C++
 	gklayout.dll!nsJSEventListener::HandleEvent(nsIDOMEvent * aEvent=0x033e4fb0)  Line 211 + 0x62 bytes	C++
 	gklayout.dll!nsEventListenerManager::HandleEventSubType(nsListenerStruct * aListenerStruct=0x034bba90, nsIDOMEventListener * aListener=0x034bb9e8, nsIDOMEvent * aDOMEvent=0x033e4fb0, nsISupports * aCurrentTarget=0x033576c8, unsigned int aSubType=1, unsigned int aPhaseFlags=6)  Line 1648 + 0x12 bytes	C++
 	gklayout.dll!nsEventListenerManager::HandleEvent(nsPresContext * aPresContext=0x035f2698, nsEvent * aEvent=0x0012f5b0, nsIDOMEvent * * aDOMEvent=0x0012f0d8, nsISupports * aCurrentTarget=0x033576c8, unsigned int aFlags=6, nsEventStatus * aEventStatus=0x0012f0dc)  Line 1752	C++
 	gklayout.dll!nsEventTargetChainItem::HandleEvent(nsEventChainPostVisitor & aVisitor={...}, unsigned int aFlags=6)  Line 335	C++
 	gklayout.dll!nsEventTargetChainItem::HandleEventTargetChain(nsEventChainPostVisitor & aVisitor={...}, unsigned int aFlags=6, nsDispatchingCallback * aCallback=0x00000000)  Line 455	C++
 	gklayout.dll!nsEventTargetChainItem::CreateChainAndHandleEvent(nsEventChainPreVisitor & aVisitor={...}, nsDispatchingCallback * aCallback=0x00000000)  Line 401 + 0x12 bytes	C++
 	gklayout.dll!nsEventTargetChainItem::CreateChainAndHandleEvent(nsEventChainPreVisitor & aVisitor={...}, nsDispatchingCallback * aCallback=0x00000000)  Line 392 + 0x10 bytes	C++
 	gklayout.dll!nsEventTargetChainItem::CreateChainAndHandleEvent(nsEventChainPreVisitor & aVisitor={...}, nsDispatchingCallback * aCallback=0x00000000)  Line 392 + 0x10 bytes	C++
 	gklayout.dll!nsEventTargetChainItem::CreateChainAndHandleEvent(nsEventChainPreVisitor & aVisitor={...}, nsDispatchingCallback * aCallback=0x00000000)  Line 392 + 0x10 bytes	C++
 	gklayout.dll!nsEventTargetChainItem::CreateChainAndHandleEvent(nsEventChainPreVisitor & aVisitor={...}, nsDispatchingCallback * aCallback=0x00000000)  Line 392 + 0x10 bytes	C++
 	gklayout.dll!nsEventTargetChainItem::CreateChainAndHandleEvent(nsEventChainPreVisitor & aVisitor={...}, nsDispatchingCallback * aCallback=0x00000000)  Line 392 + 0x10 bytes	C++
 	gklayout.dll!nsEventTargetChainItem::CreateChainAndHandleEvent(nsEventChainPreVisitor & aVisitor={...}, nsDispatchingCallback * aCallback=0x00000000)  Line 392 + 0x10 bytes	C++
 	gklayout.dll!nsEventTargetChainItem::CreateChainAndHandleEvent(nsEventChainPreVisitor & aVisitor={...}, nsDispatchingCallback * aCallback=0x00000000)  Line 392 + 0x10 bytes	C++
 	gklayout.dll!nsEventTargetChainItem::CreateChainAndHandleEvent(nsEventChainPreVisitor & aVisitor={...}, nsDispatchingCallback * aCallback=0x00000000)  Line 392 + 0x10 bytes	C++
 	gklayout.dll!nsEventTargetChainItem::CreateChainAndHandleEvent(nsEventChainPreVisitor & aVisitor={...}, nsDispatchingCallback * aCallback=0x00000000)  Line 392 + 0x10 bytes	C++
 	gklayout.dll!nsEventTargetChainItem::CreateChainAndHandleEvent(nsEventChainPreVisitor & aVisitor={...}, nsDispatchingCallback * aCallback=0x00000000)  Line 392 + 0x10 bytes	C++
 	gklayout.dll!nsEventTargetChainItem::CreateChainAndHandleEvent(nsEventChainPreVisitor & aVisitor={...}, nsDispatchingCallback * aCallback=0x00000000)  Line 392 + 0x10 bytes	C++
 	gklayout.dll!nsEventDispatcher::Dispatch(nsISupports * aTarget=0x033576c8, nsPresContext * aPresContext=0x035f2698, nsEvent * aEvent=0x0012f5b0, nsIDOMEvent * aDOMEvent=0x00000000, nsEventStatus * aEventStatus=0x0012f5ac, nsDispatchingCallback * aCallback=0x00000000, int aTargetIsChromeHandler=0)  Line 575 + 0x10 bytes	C++
 	gklayout.dll!DocumentViewerImpl::LoadComplete(unsigned int aStatus=0)  Line 1068 + 0x23 bytes	C++
 	docshell.dll!nsDocShell::EndPageLoad(nsIWebProgress * aProgress=0x0336735c, nsIChannel * aChannel=0x03638640, unsigned int aStatus=0)  Line 4859	C++
 	docshell.dll!nsWebShell::EndPageLoad(nsIWebProgress * aProgress=0x0336735c, nsIChannel * channel=0x03638640, unsigned int aStatus=0)  Line 975	C++
 	docshell.dll!nsDocShell::OnStateChange(nsIWebProgress * aProgress=0x0336735c, nsIRequest * aRequest=0x03638640, unsigned int aStateFlags=131088, unsigned int aStatus=0)  Line 4774	C++
 	docshell.dll!nsDocLoader::FireOnStateChange(nsIWebProgress * aProgress=0x0336735c, nsIRequest * aRequest=0x03638640, int aStateFlags=131088, unsigned int aStatus=0)  Line 1232	C++
 	docshell.dll!nsDocLoader::doStopDocumentLoad(nsIRequest * request=0x03638640, unsigned int aStatus=0)  Line 865	C++
 	docshell.dll!nsDocLoader::DocLoaderIsEmpty()  Line 761	C++
 	docshell.dll!nsDocLoader::OnStopRequest(nsIRequest * aRequest=0x03536c90, nsISupports * aCtxt=0x00000000, unsigned int aStatus=0)  Line 678	C++
 	necko.dll!nsLoadGroup::RemoveRequest(nsIRequest * request=0x03536c90, nsISupports * ctxt=0x00000000, unsigned int aStatus=0)  Line 685 + 0x2e bytes	C++
 	gklayout.dll!nsDocument::DoUnblockOnload()  Line 4965	C++
 	gklayout.dll!nsUnblockOnloadEvent::Run()  Line 4924	C++
 	xpcom_core.dll!nsThread::ProcessNextEvent(int mayWait=1, int * result=0x0012fe04)  Line 483	C++
 	xpcom_core.dll!NS_ProcessNextEvent_P(nsIThread * thread=0x003d9118, int mayWait=1)  Line 225 + 0x16 bytes	C++
 	gkwidget.dll!nsBaseAppShell::Run()  Line 153 + 0xc bytes	C++
 	appcomps.dll!nsAppStartup::Run()  Line 219	C++
 	seamonkey.exe!main1(int argc=1, char * * argv=0x003d2fb0, nsISupports * nativeApp=0x009cb3b0)  Line 1238 + 0x22 bytes	C++
 	seamonkey.exe!main(int argc=1, char * * argv=0x003d2fb0)  Line 1740 + 0x25 bytes	C++
 	seamonkey.exe!__tmainCRTStartup()  Line 586 + 0x19 bytes	C
 	seamonkey.exe!mainCRTStartup()  Line 403	C
 	kernel32.dll!7c816d4f() 	
 	kernel32.dll!7c8399f3() 	

This leads directly to:
###!!! ASSERTION: bad param: 'obj', file m:/vc8/mozilla/js/src/xpconnect/src/xpcwrappednative.cpp, line 1157

Then we hit the crash as described in comment 1.

Just from reading the context, I'd guess we probably need at nsDOMClassInfo.cpp#9663:

if (!wrapper) {
  nsDOMClassInfo::ThrowJSException(cx, NS_ERROR_INVALID_POINTER);
  return JS_FALSE;
}

Comment 7

[Alex Vincent [:WeirdAl]]

Attachment: patch, v1

The returned error code is wrong, but this patch does fix the crash & assertions.

Opinions?

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 226991 [details] [diff] [review]
patch, v1

>   NS_DEFINE_CLASSINFO_DATA(HTMLOptionsCollection,
>                            nsHTMLOptionsCollectionSH,
>                            ARRAY_SCRIPTABLE_FLAGS |
>+

Remove this.

> 
>   if (argc < 1 || !JSVAL_IS_OBJECT(argv[0])) {
>     nsDOMClassInfo::ThrowJSException(cx, NS_ERROR_XPC_NOT_ENOUGH_ARGS);
>     return JS_FALSE;
>   }
> 
>+  if (JSVAL_IS_NULL(argv[0])) {
>+    nsDOMClassInfo::ThrowJSException(cx, NS_ERROR_INVALID_POINTER);
>+    return JS_FALSE;
>+  }
>+

Couldn't you just do something like
if (argc < 1) {...}
if (JSVAL_IS_PRIMITIVE(argv[0])) {...}
...

/me should not comment on code which he really doesn't know at all

Comment 9

[Johnny Stenback (:jst)]

Comment on attachment 226991 [details] [diff] [review]
patch, v1

   NS_DEFINE_CLASSINFO_DATA(HTMLOptionsCollection,
                            nsHTMLOptionsCollectionSH,
                            ARRAY_SCRIPTABLE_FLAGS |
+
                            nsIXPCScriptable::WANT_SETPROPERTY)

Why add an empty line there?

   if (argc < 1 || !JSVAL_IS_OBJECT(argv[0])) {
     nsDOMClassInfo::ThrowJSException(cx, NS_ERROR_XPC_NOT_ENOUGH_ARGS);
     return JS_FALSE;
   }
 
+  if (JSVAL_IS_NULL(argv[0])) {
+    nsDOMClassInfo::ThrowJSException(cx, NS_ERROR_INVALID_POINTER);
+    return JS_FALSE;
+  }
+

I'd say loose the !JSVAL_IS_OBJECT() check in the first if statement above and make the next one check JSVAL_IS_PRIMITIVE() and throw NS_ERROR_DOM_WRONG_TYPE_ERR if argv() is a primitive type (which null is as well as all other non-object types).

If you attach an updated patch I'll gladly r+sr...

Comment 10

[Alex Vincent [:WeirdAl]]

Attachment: patch answering review comments

Sorry it took me so long to post the new patch; I wanted to test it first.

smaug: I don't mind your comments one bit; I probably know less about this code than you do.

Comment 11

[Johnny Stenback (:jst)]

Comment on attachment 227145 [details] [diff] [review]
patch answering review comments

r+sr=jst

Comment 12

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

mozilla/dom/src/base/nsDOMClassInfo.cpp 	1.396

Comment 13

[Alex Vincent [:WeirdAl]]

Comment on attachment 227145 [details] [diff] [review]
patch answering review comments

Requesting approval for 1.8.1 and 1.8.0.x - this fixes a crash known to affect Firefox 1.5.0.4 and assertions without changing API.

Comment 14

[Stephen Donner [:stephend] Not actively reading bugmail]

Verified FIXED using build 2006-06-28-08 of SeaMonkey trunk on Windows XP with https://bugzilla.mozilla.org/attachment.cgi?id=226977

Comment 15

[Darin Fisher]

Comment on attachment 227145 [details] [diff] [review]
patch answering review comments

a=darin on behalf of drivers

Comment 16

[Martijn Wargers (dead)]

Checking in dom/src/base/nsDOMClassInfo.cpp;
/cvsroot/mozilla/dom/src/base/nsDOMClassInfo.cpp,v  <--  nsDOMClassInfo.cpp
new revision: 1.292.2.41; previous revision: 1.292.2.40
done

Checked into the 1.8.1 tree.

Comment 17

[Jay Patel [:jay]]

Comment on attachment 227145 [details] [diff] [review]
patch answering review comments

approved for 1.8.0 branch, a=jay for drivers.  please land asap, so we can respin and get RC2 out for testing.

Comment 18

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

mozilla/dom/src/base/nsDOMClassInfo.cpp 	1.292.2.18.2.8

Comment 19

[David D. Kilzer (ddk)]

*** Bug 343350 has been marked as a duplicate of this bug. ***

Comment 20

[David D. Kilzer (ddk)]

*** Bug 343362 has been marked as a duplicate of this bug. ***

Comment 21

[Adam Guthrie]

Verified FIXED using Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.0.5) Gecko/20060705 Firefox/1.5.0.5.