Last Comment Bug 312278 - Access of GC-ed object in Array.prototype.toSource
: Access of GC-ed object in Array.prototype.toSource
Status: VERIFIED FIXED
[sg:critical?]
: verified1.7.13, verified1.8
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: x86 Linux
: P1 major (vote)
: mozilla1.8rc1
Assigned To: Brendan Eich [:brendan]
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2005-10-13 01:19 PDT by Igor Bukanov
Modified: 2011-08-05 21:29 PDT (History)
4 users (show)
dveditz: blocking1.7.13+
dveditz: blocking‑aviary1.0.8+
dveditz: blocking1.8rc1+
dveditz: blocking1.9a1+
bob: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Test case to crash the browser (443 bytes, text/html)
2005-10-13 01:39 PDT, Igor Bukanov
no flags Details
js1_5/Array/regress-312278.js (2.41 KB, text/plain)
2005-10-13 20:22 PDT, Bob Clary [:bc:]
no flags Details
Something that fixes the problem (1.68 KB, patch)
2005-10-14 14:59 PDT, Igor Bukanov
no flags Details | Diff | Splinter Review
targeted fix (5.56 KB, patch)
2005-10-15 00:14 PDT, Brendan Eich [:brendan]
igor: review+
shaver: superreview+
dveditz: approval‑aviary1.0.8+
dveditz: approval1.7.13+
asa: approval1.8rc1+
Details | Diff | Splinter Review
js1_5/Error/regress-312278.js (2.36 KB, text/plain)
2005-10-16 16:07 PDT, Bob Clary [:bc:]
no flags Details
fix exn_to{String,Source} (3.22 KB, patch)
2005-10-19 13:16 PDT, Blake Kaplan (:mrbkap)
brendan: review+
dveditz: approval‑aviary1.0.8+
dveditz: approval1.7.13+
brendan: approval1.8rc1+
Details | Diff | Splinter Review
fix js_ErrorToException (1.84 KB, patch)
2005-10-20 16:38 PDT, Blake Kaplan (:mrbkap)
brendan: review+
asa: approval1.8rc1+
Details | Diff | Splinter Review
fix to several problems including the one Igor pointed out (4.39 KB, patch)
2005-10-20 23:39 PDT, Brendan Eich [:brendan]
igor: review-
Details | Diff | Splinter Review
better patch (6.57 KB, patch)
2005-10-21 15:46 PDT, Brendan Eich [:brendan]
igor: review+
shaver: superreview+
dveditz: approval‑aviary1.0.8+
dveditz: approval1.7.13+
brendan: approval1.8rc1+
Details | Diff | Splinter Review
hand-merged patch for AVIARY_1_0_1_01252004_BRANCH (15.18 KB, patch)
2006-02-09 19:22 PST, Brendan Eich [:brendan]
mrbkap: review+
Details | Diff | Splinter Review
js1_5/Array/regress-312278.js (2.53 KB, text/plain)
2006-05-09 02:18 PDT, Igor Bukanov
no flags Details

Description Igor Bukanov 2005-10-13 01:19:43 PDT
array_join_sub from jsarray.c uses unrooted jsval to hold a value of array
element to apply recursive toSource to the element later in the code. A
specially crafted script can take advantage of it to GC the element before it is
passed as "this" variable to js_InternalCall. It would allow the script to
replace on a 32 platform 8 bytes of JSObect that the unrooted jsval points to by
8 bytes from chosen double value.

This is not as bad compared with other reported unrooted access bugs since this
does not allow to subvert double reference to point to other objects to learn
their addresses through double value bits. Still theoretically on platforms were
the system heap is not randomized one can try to preallocate many strings where
string body when interpreted as JSObjectMap would allow for
JSObjectMap->ops->someOperation() to be successfully executed. Then one learn
the address this strings will most likely occupy on a particular platform, use
that address to build custom double value and replace via exploit JSObject->map
to point to that expected address. 

But I have no idea how feasible it would be.
Comment 1 Igor Bukanov 2005-10-13 01:39:18 PDT
Created attachment 199400 [details]
Test case to crash the browser

The test case proceeds as following:

1. Code defines array with getter for its single element. The getter would
return a newly allocated specially crafted object. The object would be unrooted
after array_join_sub calls JS_GetElement and stores its reference in jsval.

2. To construct that special object the code first creates an object "obj" with
a getter for toSource property. The getter forces GC when called and return a
function to be used as toSource implementation. Then code wraps "obj" into With
constructor and return that.

3. array_join_sub tries to call toSource method on the unrooted With object. As
a part of implementation the runtime calls OBJ_GET_PROPERTY that executes
with_GetProperty. That method calls in turn OBJ_GET_PROPERTY on that object
with toSource getter where GC is forced and With object is GC-ed. Then the
getter return toSource implementation.

4. Code calls custom toSource passing GC-ed With there as this parameter. That
crashes since JSObject for the original With is replaced by bits from
Math.sqrt(2).
Comment 2 Brendan Eich [:brendan] 2005-10-13 18:17:53 PDT
Related to or dup of bug 311161 ?

/be
Comment 3 Bob Clary [:bc:] 2005-10-13 20:22:29 PDT
Created attachment 199507 [details]
js1_5/Array/regress-312278.js
Comment 4 Igor Bukanov 2005-10-14 03:04:20 PDT
(In reply to comment #2)
> Related to or dup of bug 311161 ?

I can not tell since I do not have access to it ;)

Comment 5 Igor Bukanov 2005-10-14 12:24:14 PDT
(In reply to comment #2)
> Related to or dup of bug 311161 ?

No, it is different one. That bug is another exposure of that "extra" problem
from bug 311497. (Well, I would not bet a fortune on it, but at least I can bet
500$ ;)

This bug is a reflection of the fact that OBJ_GET_PROPERTY/JS_GetElement etc.
return unrooted jsval if "jsval* vp" argument is itself unrooted and the value
is a result of getter call with no other references left.

It looks like most of the places where OBJ_GET_PROPERTY stores its results in
local C varaiables are not exploitable. Typically they are followed by
js_ValueToNumber/js_ValueToString etc. which roots the object before triggering
GC allocation and after the conversion the code does not access the original
unrooted value. So with one-threaded JS embeddings there is no bug. 

The problem with Array.prototype.toSource (and with Object.prototype.toSource as
well) is that via evil With there is a code path which does not root With
instance before calling JS code. It allows to trigger controllable GC to wipe
out the With instance.

To fix the problem I suggest to always store results of js_InternalInvoke in a
new rooted JSContext field. Then the code can continue to assume that
OBJ_GET_PROPERTY returns a rooted jsval. And it would close the bug for
multithreaded applications.
Comment 6 Igor Bukanov 2005-10-14 14:59:30 PDT
Created attachment 199604 [details] [diff] [review]
Something that fixes the problem

Here is a simple fix that stores results of js_InternalCall in an extra
JSContext field. It solves the problem for the price of a memory leak since the
field is always a part of GC root set until context is entered. 

Perhaps the field should be cleared when the last frame exits and cx->fp is set
to NULL?
Comment 7 Brendan Eich [:brendan] 2005-10-14 23:29:51 PDT
(In reply to comment #5)
> This bug is a reflection of the fact that OBJ_GET_PROPERTY/JS_GetElement etc.
> return unrooted jsval if "jsval* vp" argument is itself unrooted and the value
> is a result of getter call with no other references left.

But the GC model is not preemptive at arbitrary points -- requests (JS_Begin-
and JS_EndRequest) keep the GC from running on another thread, so the only GC
hazard is nesting a last-ditch GC on the current thread.  It's hard to see all
allocations in calls to subroutines, of course.  This model is fragile.

> It looks like most of the places where OBJ_GET_PROPERTY stores its results in
> local C varaiables are not exploitable. Typically they are followed by
> js_ValueToNumber/js_ValueToString etc. which roots the object before triggering
> GC allocation and after the conversion the code does not access the original
> unrooted value. So with one-threaded JS embeddings there is no bug. 

Right.

> The problem with Array.prototype.toSource (and with Object.prototype.toSource as
> well) is that via evil With there is a code path which does not root With
> instance before calling JS code. It allows to trigger controllable GC to wipe
> out the With instance.

Could we not fix just this case?

> To fix the problem I suggest to always store results of js_InternalInvoke in a
> new rooted JSContext field. Then the code can continue to assume that
> OBJ_GET_PROPERTY returns a rooted jsval. And it would close the bug for
> multithreaded applications.

The multithreaded case is handled by the request model.  In that light, we might
rather a more particular fix.  The risk of your patch is entrainment of garbage,
although your suggestion of clearing the lastInvokeResult root when popping the
last frame from cx should help.  But it's more code for a generalized solution
that we might avoid with a focused fix.

/be

Comment 8 Brendan Eich [:brendan] 2005-10-15 00:14:25 PDT
Created attachment 199634 [details] [diff] [review]
targeted fix

This is the fix I was thinking of.  It fixes the array testcase, but I haven't
been able to construct an obvious variation on that case using an object
instead of an array.  Tomorrow... someone please beat me to it.

/be
Comment 9 Igor Bukanov 2005-10-16 15:21:27 PDT
(In reply to comment #8)
> Created an attachment (id=199634) [edit]
> targeted fix
> 
> This is the fix I was thinking of.  It fixes the array testcase, but I haven't
> been able to construct an obvious variation on that case using an object
> instead of an array.  Tomorrow... someone please beat me to it.


I was wrong about Object.prototype.toSource case. It is safe from the problem as
the exploit needs a call to a getter. But toSource here does not call the getter
but rather converts it to a string for serialization. So unless there is way to
have an object with a getter where where OBJ_IS_NATIVE(object) would give false,
accessing unrooted value is safe there.

But that was not the last fish in the pond. Error.ptototype.toSource has exactly
the same problem as Array.prototype.toSource. That is, it stores results of
JS_GetProperty in an unrooted local. Here is a trivial modification of that evil
With example adopted to crash js shell:

function customToSource()
{
        return "customToSource "+this;
}

Error.prototype.__defineGetter__('message', function() {
	var obj = {
                toSource: "something"
        }
	obj.__defineGetter__('toSource', function() {
		gc();
		return customToSource;
        });
        return With(obj);
});

print(Error.prototype.toSource());
Comment 10 Bob Clary [:bc:] 2005-10-16 16:07:24 PDT
Created attachment 199766 [details]
js1_5/Error/regress-312278.js
Comment 11 Igor Bukanov 2005-10-17 03:59:49 PDT
Here is another low hanging fruit that exploits the fact that exn_toString from
jsexn.c does not root name and message. So just couple of getters are sufficient
for the demo:

Error.prototype.__defineGetter__('name', function() {
	return String.fromCharCode(40);  // 40 is code for '('
});

Error.prototype.__defineGetter__('message', function() {
	gc();
	for (var i = 0; i != 40000; ++i) {
		var tmp = Math.sqrt(2);
		tmp = null;
	}
	return String.fromCharCode(45); // 45 is code for '-'
});

print(Error.prototype.toString()); // should print "(: -"

This prints "abs: -" instead of expected "(: -" where that "abs" comes from
reading sqrt(2) as JSString.

Note that here that idea with storing results of InternalInvoke would not
prevent unrooted access since the code uses 2 unrooted values.
Comment 12 Blake Kaplan (:mrbkap) 2005-10-17 12:32:50 PDT
exn_toString is pretty simple to fix (as long as name is rooted, there's no
chance for message to get collected before its copied over to the result string).

It looks like an auditing of OBJ_GET_PROPERTY is necessary, *sigh*.
Comment 13 Blake Kaplan (:mrbkap) 2005-10-17 17:52:20 PDT
Igor, can you help determine the extent of this problem? We're really pushing
for safe spot-fixes to get into 1.8. I can fix exn_to{String,Source}. Are there
other places that need patching as well (I can help look).
Comment 14 Igor Bukanov 2005-10-18 01:55:37 PDT
(In reply to comment #13)
> Igor, can you help determine the extent of this problem?

What is "this problem" here?

> Are there other places that need patching as well (I can help look).

I wish I knew... 

My real concern is that when the bug would be known, people would start to look
for the pattern. Since unrooted bugs can be quite bad and I honestly I do not
see how one can be certain that all of them are spotted, I would prefer some
proactive solution. 

On the other hand to fix proactively problems like Error.prototype.toString bug
from comment 11 it would be necessary to add something like calls to
js_Enter/LeaveLocalRootScope before any call to a native function in
js_InternalInvoke. But that would probably lead to denial of service in trivial
cases through rooted garbage. 

Another proactive solution is to add js_EnterLocalRootScope lazily when new
object is created from JS code that was called from native code called which in
turn was called via InternalInvoke from JS code. Then corresponding
jsLeaveLocalRootScope would be executed in InternalInvoke if necessary. But that
also may produce too much rooted garbage. Or would it?
Comment 15 Igor Bukanov 2005-10-18 04:02:04 PDT
(In reply to comment #13)
> Are there other places that need patching as well (I can help look).

I would suggest to create a patch with js_Enter/LeaveLocalRootScope around calls
to native function in js_InternalInvoke and then try a browser with the patch
against reported and reproducible crashes in bugzilla. If the patch would fix
the crash, then one has unrooted access.
Comment 16 Igor Bukanov 2005-10-18 04:42:46 PDT
(In reply to comment #15)
> (In reply to comment #13)
> > Are there other places that need patching as well (I can help look).
> 
> I would suggest to create a patch with js_Enter/LeaveLocalRootScope around calls
> to native function in js_InternalInvoke and then try a browser with the patch
> against reported and reproducible crashes in bugzilla. If the patch would fix
> the crash, then one has unrooted access.

Or add a runtime option to disable GC when JS code calls native functions or
other potentially complex C code. In addition for providing extra debugging
facility such option would allow to verify for a bug reporter the nature of
potential bug without sending all the test cases and to prevent exploits if a
bad bug would be found and announced before a fix is ready. 
Comment 17 Blake Kaplan (:mrbkap) 2005-10-19 13:16:46 PDT
Created attachment 200129 [details] [diff] [review]
fix exn_to{String,Source}
Comment 18 Brendan Eich [:brendan] 2005-10-19 15:59:05 PDT
Comment on attachment 200129 [details] [diff] [review]
fix exn_to{String,Source}

r+a=me.

/be
Comment 19 Igor Bukanov 2005-10-20 01:40:18 PDT
AFAICS there is one more problem with unrooted locals in jsexn.c.
js_InitExceptionClasses there calls JS_NewStringCopyZ for messageStr and then
for filenameStr without rooting messageStr. 

After that JS_DefineProperty is called for messageStr which could trigger GC
during slot allocation before messageStr is rooted, right?
Comment 20 Brendan Eich [:brendan] 2005-10-20 08:47:41 PDT
(In reply to comment #19)
> AFAICS there is one more problem with unrooted locals in jsexn.c.
> js_InitExceptionClasses there calls JS_NewStringCopyZ for messageStr and then
> for filenameStr without rooting messageStr. 

You mean js_ErrorToException, not js_InitExceptionClasses, right?

> After that JS_DefineProperty is called for messageStr which could trigger GC
> during slot allocation before messageStr is rooted, right?

Yes, good catch -- the way this code was written violates the existing model
(http://www.mozilla.org/js/spidermonkey/gctips.html).  Once the exnObject is
rooted by cx->exception, each string should be constructed and defined in turn,
rather than making two or more strings and then adding them as property values.

/be
Comment 21 Asa Dotzler [:asa] 2005-10-20 11:45:53 PDT
brendan, we're lost in patches here. What more needs to happen here for this to
be resolved for 1.5?
Comment 22 Brendan Eich [:brendan] 2005-10-20 11:56:09 PDT
1.  shaver needs to review attachment 199634 [details] [diff] [review] ("targeted fix"), so we have two
reviews; I'm checking into the trunk now with Igor's r+.

2.  mrbkap needs to land attachment 200129 [details] [diff] [review] ("fix exn_to{String,Source}") on the
1.8 branch.

3.  It falls on mrbkap, I bet, to fix the remaining problem Igor points out in
comment 19 which I elaborate on in comment 20.

This is all I think we should jam in this bug for 1.8/fx1.5.  We may need
another bug or two, depending on what more testing and code inspection turns up,
but new problems should go in other bugs.  That will simplify the testing and
release managing by avoiding clouds of patches.

/be
Comment 23 Brendan Eich [:brendan] 2005-10-20 11:59:58 PDT
Comment on attachment 199634 [details] [diff] [review]
targeted fix

Checked into the trunk.  Pre-nominating so shaver can review today and we can
consider tomorrow based on expected zero-regression short bake cycle results on
the trunk.

/be
Comment 24 Mike Shaver (:shaver -- probably not reading bugmail closely) 2005-10-20 12:18:45 PDT
Comment on attachment 199634 [details] [diff] [review]
targeted fix

sr=shaver
Comment 25 Blake Kaplan (:mrbkap) 2005-10-20 12:45:55 PDT
Attachment 200129 [details] [diff] is now checked in on trunk and branch. I'll fix the remaining
known problem in jsexn.
Comment 26 Blake Kaplan (:mrbkap) 2005-10-20 16:38:34 PDT
Created attachment 200293 [details] [diff] [review]
fix js_ErrorToException

Local root scopes make everything OK.
Comment 27 Brendan Eich [:brendan] 2005-10-20 16:46:28 PDT
Comment on attachment 200293 [details] [diff] [review]
fix js_ErrorToException

r=me.

/be
Comment 28 Brendan Eich [:brendan] 2005-10-20 17:19:31 PDT
Comment on attachment 199634 [details] [diff] [review]
targeted fix

Fixed on the branch too.  Are we done with this bug, and ready to give it the
fixed1.8 keyword?

/be
Comment 29 Blake Kaplan (:mrbkap) 2005-10-20 17:45:22 PDT
As far as I know, all fixes have been checked into to trunk and
MOZILLA_1_8_BRANCH. Marking this bug as FIXED and fixed1.8 (also based on
comment 28).
Comment 30 Igor Bukanov 2005-10-20 23:07:16 PDT
The attachment 200129 [details] [diff] [review] does not fix exn.toSource problem as it does not add
rooting of v during GetProperty/toSource.
Comment 31 Brendan Eich [:brendan] 2005-10-20 23:39:18 PDT
Created attachment 200320 [details] [diff] [review]
fix to several problems including the one Igor pointed out

second-review? mrbkap is welcome too.

/be
Comment 32 Blake Kaplan (:mrbkap) 2005-10-21 00:02:01 PDT
Why wouldn't v be protected by newborn-ness through the ValueToSource calls?
Comment 33 Blake Kaplan (:mrbkap) 2005-10-21 00:07:21 PDT
Comment on attachment 200320 [details] [diff] [review]
fix to several problems including the one Igor pointed out

r=mrbkap, I think I see why...
Comment 34 Igor Bukanov 2005-10-21 00:28:37 PDT
Comment on attachment 200320 [details] [diff] [review]
fix to several problems including the one Igor pointed out


This is not enough since getter may return unrooted value without creating new
objects via 

var tmp = somePrviouslyInitializedReference;
somePrviouslyInitializedReference = null;
return tmp;
Comment 35 Igor Bukanov 2005-10-21 00:33:54 PDT
(In reply to comment #32)
> Why wouldn't v be protected by newborn-ness through the ValueToSource calls?

Because you can create a getter that would create many objects overwriting all
the newborn entities. Plus getter does not even need to create new objects to
return unrooted jsval, it may simply forget all rooted references to existing one.

Comment 36 Brendan Eich [:brendan] 2005-10-21 09:52:24 PDT
(In reply to comment #34)
> (From update of attachment 200320 [details] [diff] [review] [edit])
> 
> This is not enough since getter may return unrooted value without creating new
> objects via 
> 
> var tmp = somePrviouslyInitializedReference;
> somePrviouslyInitializedReference = null;
> return tmp;

But the result value of the getter is stored in a local root (in exn_toSource;
the other part of the patch uses a local root scope).  What precisely is the GC
hazard you see in the patch, at what line or lines?

/be
Comment 37 Brendan Eich [:brendan] 2005-10-21 10:19:26 PDT
(In reply to comment #36)
> But the result value of the getter is stored in a local root (in exn_toSource;

An explicit local root (rval, argv[argc], argv[argc + 1], argv[argc + 2]).  So
long as the GC can't run between the evil getter's return to JS_GetProperty and
JS_GetProperty's return to exn_toSource, there is no GC hazard.

/be
Comment 38 Igor Bukanov 2005-10-21 11:37:20 PDT
(In reply to comment #37)
So
> long as the GC can't run between the evil getter's return to JS_GetProperty and
> JS_GetProperty's return to exn_toSource, there is no GC hazard.

Sorry I did not paste the part of patch I was refering to. That was about the
fix for js_ReportUncaughtException. 

The code accesses "message" and "filename" properties of the exception object.
Since a script can define getter for both of them with the first returning
unrooted value without using "new" and the second triggering GC, it is not
enough to use js_EnterLocalRootScope there to protect message bytes from GC. 

Comment 39 Brendan Eich [:brendan] 2005-10-21 12:08:56 PDT
(In reply to comment #38)
> The code accesses "message" and "filename" properties of the exception object.
> Since a script can define getter for both of them with the first returning
> unrooted value without using "new" and the second triggering GC, it is not
> enough to use js_EnterLocalRootScope there to protect message bytes from GC. 

Local root scoping works across calls out of the C function that enters them. 
So this scope is open in subroutines called from js_ReportUncaughtException, and
it protects all newborns until that function leaves the local root scope.

Can you reverse your - on the patch?

/be
Comment 40 Igor Bukanov 2005-10-21 12:42:03 PDT
(In reply to comment #39)
> Local root scoping works across calls out of the C function that enters them. 
> So this scope is open in subroutines called from js_ReportUncaughtException, and
> it protects all newborns until that function leaves the local root scope.

But what about the following code that creates an object before local scope for
js_ReportUncaughtException is entered during normal script execution:

var error = new Error();
var prepared_message = String(Math.sqrt(2));
String(Math.sqrt(3)); // to remove JSString in prepared_message from newborn

error.__defineGetter__('message', function() {
    var tmp = prepred_message;
    prepared_message = null;
    return tmp;
});

error.__defineGetter__('filename', function() {
    for (var i = 0; i != 40000; ++i) { new Object(); }
    return "test";
});

throw error;


Now I can not check it right now, but I do not see how the patch would prevent
messageStr from GC.
Comment 41 Brendan Eich [:brendan] 2005-10-21 15:00:43 PDT
Argh, so much for local root stack -- explicit local roots (rval, argv) handle
already-born, otherwise unprotected things, but the local root stack saves only
newborns born within a local root scope.  New patch shortly.

/be
Comment 42 Brendan Eich [:brendan] 2005-10-21 15:10:52 PDT
Relieving mrbkap of this one, since I'm already patching.  Thanks to him and
Igor for help so far.

/be
Comment 43 Brendan Eich [:brendan] 2005-10-21 15:46:02 PDT
Created attachment 200392 [details] [diff] [review]
better patch

When rooting several values at once and no explicit local roots (no argv passed
to a native function), prefer to allocate scanned operand stack space.

I hacked on the test-sketch a bit:

var error = new Error();
var prepared_message = String(Math.sqrt(2));
String(Math.sqrt(3)); // to remove JSString in prepared_message from newborn

var mjunk = 2;
error.__defineGetter__('message', function() {
    if (--mjunk != 0) return "dumb";
print("blorp");
    var tmp = prepared_message;
    prepared_message = null;
    return tmp;
});

error.__defineGetter__('fileName', function() {
print("glorp");
    for (var i = 0; i != 4000000; ++i) { new Object(); }
    return "test";
});

throw error;

(Changes: fileName, not filename; and mjunk is because message is got twice,
once by the js_ValueToString(cx, exn), then in the (!reportp && exnObject &&
[error class matches])-conditioned block, where we want the get to cut all live
object graph connections to the prepared_message string.)

/be
Comment 44 Mike Shaver (:shaver -- probably not reading bugmail closely) 2005-10-22 13:30:13 PDT
Comment on attachment 200392 [details] [diff] [review]
better patch

sr=shaver
Comment 45 Brendan Eich [:brendan] 2005-10-22 21:53:23 PDT
Comment on attachment 200392 [details] [diff] [review]
better patch

This went in yesterday.  I'll close the bug.

/be
Comment 46 Brendan Eich [:brendan] 2005-10-22 22:01:01 PDT
Oops, that jsexn.c patch was not checked in already (I meant to, since Igor's r+ suffices for the trunk, but forgot to).  It's in now, and on the branch.

/be
Comment 47 Brendan Eich [:brendan] 2005-10-23 23:49:54 PDT
This bug's patch caused bug 313565 -- investigating.

/be
Comment 48 Brendan Eich [:brendan] 2005-10-24 00:06:22 PDT
(In reply to comment #47)
> This bug's patch caused bug 313565 -- investigating.

As Jesse pointed out over IRC, it was the jsstr.c patch.

/be
Comment 49 Brendan Eich [:brendan] 2005-10-24 00:08:47 PDT
Sigh, my checkin message for jsstr.c fingered this bug by mistake.  It should have identified bug 313276.

/be
Comment 50 Bob Clary [:bc:] 2005-10-25 00:21:21 PDT
winxp,

1.8b5_2005102412: pass js1_5/Error/regress-312278.js
1.9a1_2005102413: crash js1_5/Error/regress-312278.js
Comment 51 Bob Clary [:bc:] 2005-10-25 01:30:10 PDT
(In reply to comment #50)
please ignore this comment.
Comment 52 Daniel Veditz [:dveditz] 2006-02-06 12:38:52 PST
Comment on attachment 200392 [details] [diff] [review]
better patch

aviary101/moz17 landing approval: a=dveditz for drivers. Please add the fixed1.7.13 and fixed-aviary1.0.8 keywords when landed.
Comment 53 Brendan Eich [:brendan] 2006-02-09 19:22:47 PST
Created attachment 211341 [details] [diff] [review]
hand-merged patch for AVIARY_1_0_1_01252004_BRANCH
Comment 54 Blake Kaplan (:mrbkap) 2006-02-10 19:17:24 PST
This has been fixed on the 1.7 branches.
Comment 55 Bob Clary [:bc:] 2006-02-15 12:48:56 PST
removing fixed-aviary1.0.8, until someone decides if this crash is related or not. Probably same issue on mozilla 1.7.13, but I haven't tested it yet.

crash in shell 1.7.13/winxp from 2006-02-14

static JSBool
ComputeThis(JSContext *cx, JSObject *thisp, JSStackFrame *fp)
{
    JSObject *parent;

=>    if (thisp && OBJ_GET_CLASS(cx, thisp) != &js_CallClass) {
        /* Some objects (e.g., With) delegate 'this' to another object. */
        thisp = OBJ_THIS_OBJECT(cx, thisp);
        if (!thisp)
            return JS_FALSE;

+	cx	0x00037100
+	fp	0x0013e3d4
+	&js_CallClass	0x6108c710
+	parent	0x0000000c
+	thisp	0x00039600

ComputeThis(JSContext * 0x00037100, JSObject * 0x00039600, JSStackFrame * 0x0013e3d4) line 600 + 20 bytes
js_Invoke(JSContext * 0x00037100, unsigned int 0x00000000, unsigned int 0x00000002) line 860 + 23 bytes
js_InternalInvoke(JSContext * 0x00037100, JSObject * 0x00039600, long 0x00039230, unsigned int 0x00000000, unsigned int 0x00000000, long * 0x00000000, long * 0x0013e544) line 1049 + 20 bytes
js_TryMethod(JSContext * 0x00037100, JSObject * 0x00039600, JSAtom * 0x0003af90, unsigned int 0x00000000, long * 0x00000000, long * 0x0013e544) line 3827 + 31 bytes
js_ValueToSource(JSContext * 0x00037100, long 0x00039600) line 2723 + 36 bytes
exn_toSource(JSContext * 0x00037100, JSObject * 0x000394f8, unsigned int 0x00000000, long * 0x0042403c, long * 0x0013e664) line 691 + 44 bytes
js_Invoke(JSContext * 0x00037100, unsigned int 0x00000000, unsigned int 0x00000000) line 955 + 23 bytes
js_Interpret(JSContext * 0x00037100, long * 0x0013ee24) line 3020 + 15 bytes
js_Execute(JSContext * 0x00037100, JSObject * 0x00038750, JSScript * 0x0041e070, JSStackFrame * 0x00000000, unsigned int 0x00000000, long * 0x0013fecc) line 1173 + 13 bytes
JS_ExecuteScript(JSContext * 0x00037100, JSObject * 0x00038750, JSScript * 0x0041e070, long * 0x0013fecc) line 3545 + 25 bytes
Process(JSContext * 0x00037100, JSObject * 0x00038750, char * 0x00034da5) line 351 + 22 bytes
ProcessArgs(JSContext * 0x00037100, JSObject * 0x00038750, char * * 0x00034d3c, int 0x00000003) line 568 + 17 bytes
main(int 0x00000003, char * * 0x00034d3c, char * * 0x00033320) line 2431 + 21 bytes
JS! mainCRTStartup + 227 bytes
KERNEL32! 7c816d4f()

crash ff 1.0.8/winxp

JS_FRIEND_API(jsval)
js_GetSlotThreadSafe(JSContext *cx, JSObject *obj, uint32 slot)
{
    jsval v;
    JSScope *scope;
#ifndef NSPR_LOCK
    JSThinLock *tl;
    jsword me;
#endif

    /*
     * We handle non-native objects via JSObjectOps.getRequiredSlot, treating
     * all slots starting from 0 as required slots.  A property definition or
     * some prior arrangement must have allocated slot.
     *
     * Note once again (see jspubtd.h, before JSGetRequiredSlotOp's typedef)
     * the crucial distinction between a |required slot number| that's passed
     * to the get/setRequiredSlot JSObjectOps, and a |reserved slot index|
     * passed to the JS_Get/SetReservedSlot APIs.
     */
    if (!OBJ_IS_NATIVE(obj))
=>        return OBJ_GET_REQUIRED_SLOT(cx, obj, slot);

+	cx	0x03baed70
+	obj	0x03bae3b0
	slot	0x00000002

00000180()
js_GetSlotThreadSafe(JSContext * 0x03baed70, JSObject * 0x03bae3b0, unsigned long 0x00000002) line 554 + 37 bytes
ComputeThis(JSContext * 0x03baed70, JSObject * 0x03bae3b0, JSStackFrame * 0x0012e664) line 600 + 241 bytes
js_Invoke(JSContext * 0x03baed70, unsigned int 0x00000000, unsigned int 0x00000002) line 860 + 23 bytes
js_InternalInvoke(JSContext * 0x03baed70, JSObject * 0x03bae3b0, long 0x03bae240, unsigned int 0x00000000, unsigned int 0x00000000, long * 0x00000000, long * 0x0012e7d4) line 1049 + 20 bytes
js_TryMethod(JSContext * 0x03baed70, JSObject * 0x03bae3b0, JSAtom * 0x00ee1230, unsigned int 0x00000000, long * 0x00000000, long * 0x0012e7d4) line 3827 + 31 bytes
js_ValueToSource(JSContext * 0x03baed70, long 0x03bae3b0) line 2723 + 36 bytes
exn_toSource(JSContext * 0x03baed70, JSObject * 0x03bae2c0, unsigned int 0x00000000, long * 0x03d8f344, long * 0x0012e914) line 691 + 44 bytes
js_Invoke(JSContext * 0x03baed70, unsigned int 0x00000000, unsigned int 0x00000000) line 955 + 23 bytes
js_Interpret(JSContext * 0x03baed70, long * 0x0012f18c) line 3020 + 15 bytes
js_Execute(JSContext * 0x03baed70, JSObject * 0x03bac520, JSScript * 0x03d97a38, JSStackFrame * 0x00000000, unsigned int 0x00000000, long * 0x0012f2a4) line 1173 + 13 bytes
JS_EvaluateUCScriptForPrincipals(JSContext * 0x03baed70, JSObject * 0x03bac520, JSPrincipals * 0x02e29428, const unsigned short * 0x03db60e8, unsigned int 0x00000973, const char * 0x03dbc000, unsigned int 0x00000001, long * 0x0012f2a4) line 3654 + 25 bytes
nsJSContext::EvaluateString(const nsAString & {...}, void * 0x03bac520, nsIPrincipal * 0x02e29420, const char * 0x03dbc000, unsigned int 0x00000001, const char * 0x100ba430, nsAString & {...}, int * 0x0012f2f0) line 946 + 67 bytes
nsScriptLoader::EvaluateScript(nsScriptLoadRequest * 0x03db9750, const nsString & {...}) line 668
nsScriptLoader::ProcessRequest(nsScriptLoadRequest * 0x03db9750) line 581 + 22 bytes
nsScriptLoader::OnStreamComplete(nsScriptLoader * const 0x03d442c4, nsIStreamLoader * 0x03dbc700, nsISupports * 0x03db9750, unsigned int 0x00000000, unsigned int 0xffffffff, const char * 0x03db19a4) line 905
nsStreamLoader::OnStopRequest(nsStreamLoader * const 0x03dbc704, nsIRequest * 0x03dbc0a0, nsISupports * 0x03db9750, unsigned int 0x00000000) line 144
nsStreamListenerTee::OnStopRequest(nsStreamListenerTee * const 0x03db9fd0, nsIRequest * 0x03dbc0a0, nsISupports * 0x03db9750, unsigned int 0x00000000) line 66
nsHttpChannel::OnStopRequest(nsHttpChannel * const 0x03dbc0a8, nsIRequest * 0x03dba180, nsISupports * 0x00000000, unsigned int 0x00000000) line 3739
nsInputStreamPump::OnStateStop() line 499
nsInputStreamPump::OnInputStreamReady(nsInputStreamPump * const 0x03dba184, nsIAsyncInputStream * 0x03dba05c) line 339 + 11 bytes
nsInputStreamReadyEvent::EventHandler(PLEvent * 0x03dba264) line 119
PL_HandleEvent(PLEvent * 0x03dba264) line 673 + 10 bytes
PL_ProcessPendingEvents(PLEventQueue * 0x00ef3d90) line 608 + 9 bytes
_md_EventReceiverProc(HWND__ * 0x0011056a, unsigned int 0x0000c146, unsigned int 0x00000000, long 0x00ef3d90) line 1414 + 9 bytes
USER32! 77d48734()
USER32! 77d48816()
USER32! 77d489cd()
USER32! 77d48a10()
nsAppShell::Run(nsAppShell * const 0x02e4c368) line 135
nsAppShellService::Run(nsAppShellService * const 0x02e4c2a8) line 495
xre_main(int 0x00000004, char * * 0x003e6e38, const nsXREAppData * 0x0041e01c kAppData) line 1907 + 35 bytes
main(int 0x00000004, char * * 0x003e6e38) line 58 + 18 bytes
mainCRTStartup() line 338 + 17 bytes
KERNEL3
Comment 56 Daniel Veditz [:dveditz] 2006-02-15 18:42:51 PST
Neither Blake nor I crash with these testcases.
Comment 57 Marcia Knous [:marcia - use ni] 2006-02-16 10:27:09 PST
I do not crash using the testcase in the bug - Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7.13) Gecko/20060214 Firefox/1.0.8.
Comment 58 Bob Clary [:bc:] 2006-02-17 00:34:33 PST
verified with nightly builds.

firefox/1.7.13 : 20060216 win/linux/mac
firefox/1.8.0.1: 20060214 winxp/20060215 linux/20060216 mac
firefox/1.8    : 20060216 winxp/linux/mac
firefox/1.9a1  : 20060216 winxp/linux/mac
Comment 59 Bob Clary [:bc:] 2006-02-17 00:36:21 PST
oops, forgot to mention verified on Mozilla 1.7.13/winxp/20060213 but not other platforms.
Comment 60 Bob Clary [:bc:] 2006-02-18 01:06:04 PST
Ok, I don't know what to say except I am seeing failures on linux for ff 1.0.8 for the 20060217 nightly when run in the test framework. If you need help in trying to reproduce this, catch me on irc.

Here are the full list of failures for the test run on this test.

js1_5/Error/regress-312278.js:  EXIT CRASHED 5 (0.580000 seconds) : (browser) : ./peachssh/2006-02-17-12-02-44-firefox-1.0-nightly-win32-1.7.5_20041111XX.log

js1_5/Error/regress-312278.js:  EXIT CRASHED 5 (0.592000 seconds) : (browser) : ./prunessh/2006-02-17-12-03-29-firefox-1.0-nightly-win32-1.7.5_20041111XX.log

js1_5/Error/regress-312278.js:  EXIT CRASHED 5 (0.780000 seconds) : (browser) : ./peachssh/2006-02-17-12-50-08-firefox-1.0.7-nightly-win32-1.7.12_20050915XX.log

js1_5/Error/regress-312278.js:  EXIT CRASHED 5 (0.791000 seconds) : (browser) : ./prunessh/2006-02-17-12-53-12-firefox-1.0.7-nightly-win32-1.7.12_20050915XX.log

js1_5/Error/regress-312278.js:  EXIT CRASHED signal 11 (1.390734 seconds) : (browser) : ./plumssh/2006-02-17-11-55-04-firefox-1.0-nightly-linux-1.7.5_20041107XX.log

js1_5/Error/regress-312278.js:  EXIT CRASHED signal 11 (1.416192 seconds) : (browser) : ./plumssh/2006-02-17-14-31-14-firefox-1.0.x-nightly-linux-1.7.13_20060214XX.log

js1_5/Error/regress-312278.js:  EXIT CRASHED signal 11 (1.435361 seconds) : (browser) : ./plumssh/2006-02-17-13-05-53-firefox-1.0.7-nightly-linux-1.7.12_20050915XX.log

js1_5/Error/regress-312278.js:  EXIT CRASHED signal 11 (1.437397 seconds) : (browser) : ./pearssh/2006-02-17-13-07-05-firefox-1.0.7-nightly-linux-1.7.12_20050915XX.log

js1_5/Error/regress-312278.js:  EXIT CRASHED signal 11 (1.445457 seconds) : (browser) : ./pearssh/2006-02-17-14-29-29-firefox-1.0.x-nightly-linux-1.7.13_20060214XX.log

js1_5/Error/regress-312278.js:  EXIT CRASHED signal 11 (1.446330 seconds) : (browser) : ./pearssh/2006-02-17-11-53-48-firefox-1.0-nightly-linux-1.7.5_20041107XX.log

js1_5/Error/regress-312278.js:  EXIT CRASHED signal 11 (1.607750 seconds) : (browser) : ./pineapplessh/2006-02-17-12-15-42-firefox-1.0.7-nightly-mac-1.7.12_20050915XX.log

js1_5/Error/regress-312278.js:  EXIT CRASHED signal 11 (1.639956 seconds) : (browser) : ./papayassh/2006-02-17-12-14-10-firefox-1.0.7-nightly-mac-1.7.12_20050915XX.log
Comment 61 Bob Clary [:bc:] 2006-02-18 01:22:28 PST
(In reply to comment #60)

Note the linux builds are from 20060214 instead of 20060217. I filed bug 327704 on the linux builds not being updated.
Comment 62 Bob Clary [:bc:] 2006-02-22 02:47:32 PST
v ff on 1.7.13, 1.8.0.1, 1.8, 1.9a1 on windows/linux/mac 20060221 builds.
Comment 63 Igor Bukanov 2006-05-09 02:18:36 PDT
Created attachment 221433 [details]
js1_5/Array/regress-312278.js

This is a replacement for the test case that does not use explicit With constructor. That extension was removed from SpiderMonkey, see bug 336784.

As a replacement for With(obj) the new test case uses wrapInsideWith(obj) where the utlity function is defined as:

function wrapInsideWith(obj)
{
  var f;
  with (obj) {
    f = function() { }
  }
  return f.__parent__;
}

This uses yet another SpiderMonkey extension, namely __parent__ property, but that is not going to be removed yet.

I also changed the test to crash js shell that uses uptached js sources.
Comment 64 Bob Clary [:bc:] 2006-06-14 16:54:31 PDT
RCS file: /cvsroot/mozilla/js/tests/js1_5/GC/regress-312278.js,v
done
Checking in regress-312278.js;
/cvsroot/mozilla/js/tests/js1_5/GC/regress-312278.js,v  <--  regress-312278.js
initial revision: 1.1
done

Note You need to log in before you can comment on or make changes to this bug.