Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now) Reporter
	Description • 16 years ago

I found this crash in latest js opt shell, and reduced it a little. Jesse Ruderman further reduced it till

js> var o = { __noSuchMethod__: Function }; new o.y;

This crash occurs in KERN_INVALID_ADDRESS at 0x0000000080080005. at js_ValueToString.

The line causes an assertion in debug js shell as well:

Assertion failure: OBJ_GET_CLASS(cx, obj)->flags & JSCLASS_HAS_PRIVATE, at jsapi.c:2876

	Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now) Reporter
	Comment 2 • 16 years ago

For notes:

This crash was found after running ~2-3 billion individual js testcases using jsfunfuzz.js on js opt builds after the report of bug 427191, which had been discovered after ~1 billion individual js testcases.

	Igor Bukanov Assignee
	Comment 3 • 16 years ago

The reason for the bug is that in 

   var o = { __noSuchMethod__: Function }; new o.y;

Function will be called as a constructor with "this" set to an instance of "Object" class. This is bad since the Function, when called as a constructor, expects its "this" parameter to have "Function" class.

This happens because NoSuchMethod just calls js_InternalInvoke even weh called with JSINVOKE_CONSTRUCT, not js_InvokeConstructor, bypassing the "this" object initialization done by the latter.

	Damon Sicore (:damons)
	Comment 4 • 16 years ago

Can we get a better explanation as to why this should block?  How often will
people hit this?  Exploitable? etc.  

Please re-nom once we have more info.

  

	Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now) Reporter
	Comment 5 • 16 years ago

Since the crash address is 0x0000000080080005, I think it's likely to be exploitable.

Re-nominating blocking-1.9, unless someone thinks otherwise and feels that it should end up wanted-1.9.0.x instead.

	Jesse Ruderman
	Comment 6 • 16 years ago

This bug is easy to find by fuzzing, too.

	Igor Bukanov Assignee
	Comment 7 • 16 years ago

(In reply to comment #4)
> Exploitable? etc.  

The consequence of the bug is writing of 5 words beyond the allocated memory when the code misrepresents JSObject as JSFunction and tries to initialize the extra JSFunction fields. The context of these words are partially controllable by a script. Since the objects are allocated sequentially from a special GC arena, most likely the overwritten bytes would belong to another object containing various pointers to function maps.

So I guess this is exploitable.

	Jesse Ruderman
	Comment 8 • 16 years ago

Igor, will you be able to fix this soon?  I told other browser vendors I'd be releasing the new version of jsfunfuzz around now, but I also don't want to delay Firefox 3 by turning this bug into a firedrill.

	Igor Bukanov Assignee
	Comment 9 • 16 years ago

I plan to have a patch ready today.

	Damon Sicore (:damons)
	Comment 10 • 16 years ago

There are other fuzzer bugs that are likely exploitable as well that we're not holding back the release for.  At this point, wouldn't hold back the release unless the exploit is public.

So, don't release any new fuzzers, Jessie!  :-)

	Jesse Ruderman
	Comment 11 • 16 years ago

This is the only security-sensitive dependency of the jsfunfuzz bug.

	Igor Bukanov Assignee
	Comment 12 • 16 years ago

The bug is regression from bug 420399.

	Damon Sicore (:damons)
	Comment 13 • 16 years ago

+'ing this per a direct request from Brendan.  Let's see a patch and how much of a risk it is.

	Igor Bukanov Assignee
	Comment 14 • 16 years ago

I added dependancy on bug 430871 as fixing that bug simplifies the patch here.

	Igor Bukanov Assignee
	Comment 15 • 16 years ago

[This patch depends on the patch from bug 430871 comment 1]

The essence of the patch is NoSuchMethod to call js_InvokeConstructor when the method is called in the constructor context. The rest is naming of anonymous constants and reodering js_InvokeConstructor signature to use (uintN argc, jsval *vp) ordering from the rest of code.

	Brendan Eich [:brendan]
	Comment 16 • 16 years ago

Comment on attachment 317783 [details] [diff] [review]
v1

>     JS_ASSERT(STOBJ_GET_CLASS(obj) == &js_NoSuchMethodClass);
> 
>+    invokevp[0] = OBJ_GET_SLOT(cx, obj, JSSLOT_FOUND_FUNCTION);
>+    invokevp[1] = vp[1];
>+    invokevp[2] = OBJ_GET_SLOT(cx, obj, JSSLOT_SAVED_ID);

The js_NoSuchMethodClass instance can't leak into script, IIRC, so it doesn't need to be thread-safe. Or does it? If not, then STOBJ_GET_SLOT, etc. Oh, but those slots are within obj->fslots, so you really don't need any veneer here. I think we prefer obj->fslots[JSSLOT_XXX] for maximum clarity and explicitude.

>+    if (flags & JSINVOKE_CONSTRUCT)
>+        ok = js_InvokeConstructor(cx, 2, invokevp);
>+    else
>+        ok = js_Invoke(cx, 2, invokevp, flags);

Uber-nit: house style commons LHS using ?:, still aligning multiline consequent and alternate -- expression language ftw.

r=me with these picked or addressed somehow.

/be

	Igor Bukanov Assignee
	Comment 17 • 16 years ago

In v1 I have early return in NoSuchMethod that bypassed a call to js_FreeStack. The new version fixes that besides addressing the nits. Here is a delta:

@@ -943,6 +943,6 @@ js_OnUnknownMethod(JSContext *cx, jsval 
             goto out;
         }
-        STOBJ_SET_SLOT(obj, JSSLOT_FOUND_FUNCTION, tvr.u.value);
-        STOBJ_SET_SLOT(obj, JSSLOT_SAVED_ID, vp[0]);
+        obj->fslots[JSSLOT_FOUND_FUNCTION] = tvr.u.value;
+        obj->fslots[JSSLOT_SAVED_ID] = vp[0];
         vp[0] = OBJECT_TO_JSVAL(obj);
     }
@@ -966,4 +966,5 @@ NoSuchMethod(JSContext *cx, uintN argc, 
         return JS_FALSE;
 
+    /* From this point the control must flow through the label out. */
     JS_ASSERT(!JSVAL_IS_PRIMITIVE(vp[0]));
     JS_ASSERT(!JSVAL_IS_PRIMITIVE(vp[1]));
@@ -971,16 +972,19 @@ NoSuchMethod(JSContext *cx, uintN argc, 
     JS_ASSERT(STOBJ_GET_CLASS(obj) == &js_NoSuchMethodClass);
 
-    invokevp[0] = OBJ_GET_SLOT(cx, obj, JSSLOT_FOUND_FUNCTION);
+    invokevp[0] = obj->fslots[JSSLOT_FOUND_FUNCTION];
     invokevp[1] = vp[1];
-    invokevp[2] = OBJ_GET_SLOT(cx, obj, JSSLOT_SAVED_ID);
+    invokevp[2] = obj->fslots[JSSLOT_SAVED_ID];
     argsobj = js_NewArrayObject(cx, argc, vp + 2);
-    if (!argsobj)
-        return JS_FALSE;
+    if (!argsobj) {
+        ok = JS_FALSE;
+        goto out;
+    }
     invokevp[3] = OBJECT_TO_JSVAL(argsobj);
-    if (flags & JSINVOKE_CONSTRUCT)
-        ok = js_InvokeConstructor(cx, 2, invokevp);
-    else
-        ok = js_Invoke(cx, 2, invokevp, flags);
+    ok = (flags & JSINVOKE_CONSTRUCT)
+         ? js_InvokeConstructor(cx, 2, invokevp)
+         : js_Invoke(cx, 2, invokevp, flags);
     vp[0] = invokevp[0];
+
+  out:
     js_FreeStack(cx, mark);
     return ok;

	Brendan Eich [:brendan]
	Comment 18 • 16 years ago

Comment on attachment 317799 [details] [diff] [review]
v2

Sorry, should have caught that failure to free stack on error.

>     argsobj = js_NewArrayObject(cx, argc, vp + 2);
>+    if (!argsobj) {
>+        ok = JS_FALSE;
>+        goto out;
>+    }
>+    invokevp[3] = OBJECT_TO_JSVAL(argsobj);
>+    ok = (flags & JSINVOKE_CONSTRUCT)
>+         ? js_InvokeConstructor(cx, 2, invokevp)
>+         : js_Invoke(cx, 2, invokevp, flags);
>+    vp[0] = invokevp[0];
>+
>+  out:
>+    js_FreeStack(cx, mark);
>+    return ok;

Avoid a goto, an angel gets its wings. Just else the success path and r=me.

/be

	Igor Bukanov Assignee
	Comment 19 • 16 years ago

The new version avoids goto, here is a delta against v2:

@@ -967,3 +967,2 @@ NoSuchMethod(JSContext *cx, uintN argc, 
 
-    /* From this point the control must flow through the label out. */
     JS_ASSERT(!JSVAL_IS_PRIMITIVE(vp[0]));
@@ -979,11 +978,9 @@ NoSuchMethod(JSContext *cx, uintN argc, 
         ok = JS_FALSE;
-        goto out;
+    } else {
+        invokevp[3] = OBJECT_TO_JSVAL(argsobj);
+        ok = (flags & JSINVOKE_CONSTRUCT)
+             ? js_InvokeConstructor(cx, 2, invokevp)
+             : js_Invoke(cx, 2, invokevp, flags);
+        vp[0] = invokevp[0];
     }
-    invokevp[3] = OBJECT_TO_JSVAL(argsobj);
-    ok = (flags & JSINVOKE_CONSTRUCT)
-         ? js_InvokeConstructor(cx, 2, invokevp)
-         : js_Invoke(cx, 2, invokevp, flags);
-    vp[0] = invokevp[0];
-
-  out:
     js_FreeStack(cx, mark);

	Igor Bukanov Assignee
	Comment 20 • 16 years ago

This is a normal CVS diff that I generated after landing the bug 430871.

	Damon Sicore (:damons)
	Comment 21 • 16 years ago

Comment on attachment 318115 [details] [diff] [review]
v3 (cvs diff)

a1.9+=damons

	Igor Bukanov Assignee
	Comment 22 • 16 years ago

I checked in the patch from the comment 20 to the trunk:

http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&branch=HEAD&cvsroot=%2Fcvsroot&date=explicit&mindate=1209503799&maxdate=1209503844&who=igor%25mir2.org

	Bob Clary [:bc] (inactive)
	Comment 23 • 16 years ago

no need to cc me on js bugs. I watch the component and see _all_. ;-) I also regularly check fixed bugs for unset in-testsuite and add tests as they are fixed. If you see a works for me or duplicate bug that you think needs a test though, please do add in-testsuite?

	Bob Clary [:bc] (inactive)
	Comment 25 • 16 years ago

v 1.9.0

	Daniel Veditz [:dveditz]
	Comment 26 • 16 years ago

We're not planning on taking perf bug 420399 on the 1.8 branch, doesn't look like we need this fix.

	Bob Clary [:bc] (inactive)
	Comment 27 • 15 years ago

test checked into 1.9.0, 1.9.1, 1.9.2, tracemonkey. 1.9.3 will get picked up in the next merge.