Closed Bug 312278 Opened 19 years ago Closed 19 years ago

Access of GC-ed object in Array.prototype.toSource

Categories

(Core :: JavaScript Engine, defect, P1)

x86
Linux
defect

Tracking

()

VERIFIED FIXED
mozilla1.8rc1

People

(Reporter: igor, Assigned: brendan)

Details

(Keywords: verified1.7.13, verified1.8, Whiteboard: [sg:critical?])

Attachments

(8 files, 3 obsolete files)

443 bytes, text/html
Details
1.68 KB, patch
Details | Diff | Splinter Review
5.56 KB, patch
igor
: review+
shaver
: superreview+
Details | Diff | Splinter Review
3.22 KB, patch
brendan
: review+
Details | Diff | Splinter Review
1.84 KB, patch
brendan
: review+
Details | Diff | Splinter Review
6.57 KB, patch
igor
: review+
shaver
: superreview+
Details | Diff | Splinter Review
15.18 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
2.53 KB, text/plain
Details
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.
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).
Attachment #199400 - Attachment mime type: text/plain → text/html
Assignee: general → mrbkap
Flags: blocking1.9a1+
Flags: blocking1.8rc1+
Flags: blocking1.7.13?
Flags: blocking-aviary1.0.8?
Whiteboard: [sg:critical?]
Priority: -- → P1
Target Milestone: --- → mozilla1.8rc1
Related to or dup of bug 311161 ?

/be
Attached file js1_5/Array/regress-312278.js (obsolete) —
Flags: testcase+
(In reply to comment #2)
> Related to or dup of bug 311161 ?

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

(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.
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?
Attachment #199604 - Flags: review?(brendan)
(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

Attached patch targeted fixSplinter Review
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
Attachment #199634 - Flags: superreview?(shaver)
Attachment #199634 - Flags: review?(igor.bukanov)
(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());
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.
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*.
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).
(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?
(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.
(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 on attachment 200129 [details] [diff] [review]
fix exn_to{String,Source}

r+a=me.

/be
Attachment #200129 - Flags: review?(brendan)
Attachment #200129 - Flags: review+
Attachment #200129 - Flags: approval1.8rc1+
Attachment #199634 - Flags: review?(igor.bukanov) → review+
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?
(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
brendan, we're lost in patches here. What more needs to happen here for this to
be resolved for 1.5?
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 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
Attachment #199634 - Flags: approval1.8rc1?
Comment on attachment 199634 [details] [diff] [review]
targeted fix

sr=shaver
Attachment #199634 - Flags: superreview?(shaver) → superreview+
Attachment 200129 [details] [diff] is now checked in on trunk and branch. I'll fix the remaining
known problem in jsexn.
Local root scopes make everything OK.
Attachment #200293 - Flags: review?(brendan)
Comment on attachment 200293 [details] [diff] [review]
fix js_ErrorToException

r=me.

/be
Attachment #200293 - Flags: review?(brendan)
Attachment #200293 - Flags: review+
Attachment #200293 - Flags: approval1.8rc1?
Attachment #199634 - Flags: approval1.8rc1? → approval1.8rc1+
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
Attachment #200293 - Flags: approval1.8rc1? → approval1.8rc1+
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).
Status: NEW → RESOLVED
Closed: 19 years ago
Keywords: fixed1.8
Resolution: --- → FIXED
The attachment 200129 [details] [diff] [review] does not fix exn.toSource problem as it does not add
rooting of v during GetProperty/toSource.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
second-review? mrbkap is welcome too.

/be
Attachment #200320 - Flags: superreview?(shaver)
Attachment #200320 - Flags: review?(igor.bukanov)
Why wouldn't v be protected by newborn-ness through the ValueToSource calls?
Comment on attachment 200320 [details] [diff] [review]
fix to several problems including the one Igor pointed out

r=mrbkap, I think I see why...
Attachment #200320 - Flags: review+
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;
Attachment #200320 - Flags: review?(igor.bukanov) → review-
(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.

(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
Status: REOPENED → ASSIGNED
(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
(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. 

(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
(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.
Not accessible to reporter
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
Attachment #200320 - Attachment is obsolete: true
Attachment #200320 - Flags: superreview?(shaver)
Attachment #200320 - Flags: review+
Relieving mrbkap of this one, since I'm already patching.  Thanks to him and
Igor for help so far.

/be
Assignee: mrbkap → brendan
Status: ASSIGNED → NEW
Attached patch better patchSplinter Review
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
Attachment #200392 - Flags: superreview?(shaver)
Attachment #200392 - Flags: review?(igor.bukanov)
Accessible to reporter
Attachment #200392 - Flags: review?(igor.bukanov) → review+
Attachment #199604 - Flags: review?(brendan)
Comment on attachment 200392 [details] [diff] [review]
better patch

sr=shaver
Attachment #200392 - Flags: superreview?(shaver) → superreview+
Comment on attachment 200392 [details] [diff] [review]
better patch

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

/be
Attachment #200392 - Flags: approval1.8rc1+
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
Status: NEW → RESOLVED
Closed: 19 years ago19 years ago
Resolution: --- → FIXED
This bug's patch caused bug 313565 -- investigating.

/be
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(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
Status: REOPENED → ASSIGNED
Sigh, my checkin message for jsstr.c fingered this bug by mistake.  It should have identified bug 313276.

/be
Status: ASSIGNED → RESOLVED
Closed: 19 years ago19 years ago
Resolution: --- → FIXED
winxp,

1.8b5_2005102412: pass js1_5/Error/regress-312278.js
1.9a1_2005102413: crash js1_5/Error/regress-312278.js
(In reply to comment #50)
please ignore this comment.
Flags: blocking1.7.13?
Flags: blocking1.7.13+
Flags: blocking-aviary1.0.8?
Flags: blocking-aviary1.0.8+
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.
Attachment #200392 - Flags: approval1.7.13+
Attachment #200392 - Flags: approval-aviary1.0.8+
Attachment #199634 - Flags: approval1.7.13+
Attachment #199634 - Flags: approval-aviary1.0.8+
Attachment #200129 - Flags: approval1.7.13+
Attachment #200129 - Flags: approval-aviary1.0.8+
Attachment #211341 - Flags: review?(mrbkap) → review+
This has been fixed on the 1.7 branches.
Keywords: fixed1.8
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
Neither Blake nor I crash with these testcases.
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.
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
oops, forgot to mention verified on Mozilla 1.7.13/winxp/20060213 but not other platforms.
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
(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.
v ff on 1.7.13, 1.8.0.1, 1.8, 1.9a1 on windows/linux/mac 20060221 builds.
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.
Attachment #199507 - Attachment is obsolete: true
Attachment #199766 - Attachment is obsolete: true
Group: security
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
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: