Closed Bug 452913 (CVE-2009-0353) Opened 16 years ago Closed 16 years ago

Crash [@ js_SetPropertyHelper]

Categories

(Core :: JavaScript Engine, defect, P1)

defect

Tracking

()

VERIFIED FIXED

People

(Reporter: gkw, Assigned: igor)

Details

(5 keywords, Whiteboard: [sg:critical?] post 1.8-branch)

Crash Data

Attachments

(7 files, 6 obsolete files)

2.85 KB, text/plain
Details
22.49 KB, text/plain
Details
2.36 KB, text/plain
Details
6.58 KB, patch
igor
: review+
Details | Diff | Splinter Review
6.71 KB, patch
brendan
: review+
Details | Diff | Splinter Review
3.53 KB, text/plain
Details
2.19 KB, text/plain
Details
Attached file crash log (obsolete) —
(this.__defineGetter__("x", function (x)'foo'.replace(/o/g, [1].push))); for(let y in [,,,]) for(let y in [,,,]) x = x


$ ./js-moz190-intelmac-debug
js> (this.__defineGetter__("x", function (x)'foo'.replace(/o/g, [1].push))); for(let y in [,,,]) for(let y in [,,,]) x = x
Assertion failure: JS_PROPERTY_CACHE(cx).disabled, at jsinterp.c:97
Trace/BPT trap


Blows opt builds at a possibly exploitable 0x0000000080303ae3 address, then asserts debug with the above assertion. Mac 10.5.4 Leopard.

Nominating security sensitive because it crashes at a scary address.
Flags: blocking1.9.0.3?
Flags: blocking1.9.0.3? → wanted1.9.0.x+
Whiteboard: [sg:critical?]
Adding sayrer for assignment help.
QA Contact: general → igor
Assignee: general → igor
QA Contact: igor → general
I am looking into it.
In the test case the code 'foo'.replace(/o/g, [1].push) will call the push method of the array with its this object set to Array.prototype. Each such invocation adds 3 elements to Array.prototype. Now, the for-in loop for(let y in [,,,]) will iterate through Array.prototype after it finishes with [,,,]. Each such iteration in the inner loop adds elements to the prototype leading to longer and longer array to iterate in the subsequent loops. Eventually this will exhaust the system memory.

Now, I cannot reproduce the crash on 64-bit Linux as the test correctly reports out-of-memory. So here is the question:

1. Does the crash happens reliably on the reported system?
2. If so, is it possible to get access to it to debug the problem there?
> with its this object set to Array.prototype

Why Array.prototype?  Where does that come from?
Attached file updated crash log
> 1. Does the crash happens reliably on the reported system?

Yes.

> 2. If so, is it possible to get access to it to debug the problem there?

Yes. (The question is, how should I debug it for you to get whatever info is needed?)
Attachment #336165 - Attachment is obsolete: true
I am on Mac 10.5.5 now. Latest 1.9.0.x shell checkout.

Attached are the updated backtraces, normal and full, and |list| output ten times, if it helps.
Attachment #336167 - Attachment is obsolete: true
(In reply to comment #5)
> > with its this object set to Array.prototype
> 
> Why Array.prototype?  Where does that come from?

string.replace(regex, f) will use the parent of f as its "this" to call the function f. In SpiderMonkey Array's natives have Array.prototype as their parent. The latter is not specified in EcmaScript and implementations are free to do what ever suits them since.
> The question is, how should I debug it for you to get whatever info is
> needed?

A possible reason for the bug is that after malloc fails on out-of-memory some code path does not handle that properly. So the question is if js_ReportOutOfMemory is ever called before the crash? If it is notr, then would the following examples causes the crash:

this.__defineGetter__("x", function () {
        Array.prototype.push("o", 1, "b");
        Array.prototype.push("o", 2, "b");
        return "";
    });

function f2(a, y, z) {
    for(y in a) {
        for(z in a) x;
    }
}

f2([0,1,2]);
print(Array.prototype.length);

The example takes out the regexp out of the loop.
$ ./js-opt-moz190-intelmac 
js> this.__defineGetter__("x", function () {
        Array.prototype.push("o", 1, "b");
        Array.prototype.push("o", 2, "b");
        return "";
    });
js> 
js> function f2(a, y, z) {
    for(y in a) {
        for(z in a) x;
    }
}
js> 
js> f2([0,1,2]);
Segmentation fault


Igor, yes, this causes the crash at the same location.
What about the following example? Does it crash?

function something() {
        Array.prototype.push("o", 1, "b");
        Array.prototype.push("o", 2, "b");
        return "";
}

function f2(a, y, z) {
    for(y in a) {
        for(z in a) something();
    }
}

f2([0,1,2]);
$ ./js-opt-moz190-intelmac 
js> function something() {
        Array.prototype.push("o", 1, "b");
        Array.prototype.push("o", 2, "b");
        return "";
}
js> 
js> function f2(a, y, z) {
    for(y in a) {
        for(z in a) something();
    }
}
js> 
js> f2([0,1,2]);
Segmentation fault


Yes.
Ok, so what about the following 3 examples?

The first:

function f2(a, y, z) {
    for(y in a) {
        for(z in a) 
            Array.prototype.push("o", 2, "b");
    }
}

f2([0,1,2]);


The second:

var a = [1];
while (true) {
    for (var i in a)
        Array.prototype.push(0);
}

The third:

while (true) {
    Array.prototype.push(0);
}
first:

$ ./js-opt-moz190-intelmac 
js> function f2(a, y, z) {
    for(y in a) {
        for(z in a) 
            Array.prototype.push("o", 2, "b");
    }
}
js> 
js> f2([0,1,2]);
Segmentation fault

Yes, at - Exception Type:  EXC_BAD_ACCESS (SIGSEGV)
Exception Codes: KERN_INVALID_ADDRESS at 0x000000002c0d9ef8

===

second:

$ ./js-opt-moz190-intelmac 
js> var a = [1];
js> while (true) {
    for (var i in a)
        Array.prototype.push(0);
}
Illegal instruction

Yes, at - Exception Type:  EXC_BAD_INSTRUCTION (SIGILL)
Exception Codes: 0x0000000000000001, 0x0000000000000000
Crashed Thread:  0

===

third:

 ./js-opt-moz190-intelmac 
js> while (true) {
    Array.prototype.push(0);
}
Segmentation fault

Yes, at - Exception Type:  EXC_BAD_ACCESS (SIGSEGV)
Exception Codes: KERN_INVALID_ADDRESS at 0x0000000000438280
Note: This causes the slow script dialog to come up -- but Fx 3.1 beta 1 then crashes if the script is continued.
I can reproduce the segfault with the third test case from the comment 14 on 64-bit Linux.
Attached patch fix for 1.9 branch (obsolete) — Splinter Review
To Gary Kwong:

Could you test this patch for 1.9.0 branch to see if it fixes the test cases?
(In reply to comment #18)
> Created an attachment (id=345914) [details]
> fix for 1.9 branch
> 
> To Gary Kwong:
> 
> Could you test this patch for 1.9.0 branch to see if it fixes the test cases?

Nope, it still crashes with the patch for both 1.9.0.x opt and debug:

opt:

$ ./js
js> while (true) {
    Array.prototype.push(0);
}
Segmentation fault

===

Exception Type:  EXC_BAD_ACCESS (SIGSEGV)
Exception Codes: 0x000000000000000d, 0x0000000000000000
Crashed Thread:  0

Thread 0 Crashed:
0   ???                           	0x000c21e4 0 + 795108
1   js                            	0x00048e26 js_SetPropertyHelper + 1333
2   js                            	0x00048ef9 js_SetProperty + 46
3   js                            	0x0000f297 SetArrayElement + 275
4   js                            	0x0000f627 InitArrayElements + 197
5   js                            	0x0000f866 array_push_slowly + 91

/snip
Attached patch fix for 1.9.0 v2 (obsolete) — Splinter Review
To Gary: could you test the new patch?
Attached patch fix for 1.9.0 v3 (obsolete) — Splinter Review
The previous version has some leftover from a preliminary fix, now it should be it for 1.9.0. Gary, could you check it?
Attachment #345914 - Attachment is obsolete: true
Attachment #347299 - Attachment is obsolete: true
(In reply to comment #20)
> Created an attachment (id=347299) [details]
> fix for 1.9.0 v2
> 
> To Gary: could you test the new patch?

Igor, this patch gives me 100% utilization for ~5 mins until I killed it, is it meant to be like that?
(In reply to comment #21)
> Created an attachment (id=347301) [details]
> fix for 1.9.0 v3
> 
> The previous version has some leftover from a preliminary fix, now it should be
> it for 1.9.0. Gary, could you check it?

Besides the above mentioned slowness in comment #22, there isn't a crash this time though. The mem usage maxes out at 738M RPRVT with 100% utilization.
(In reply to comment #23)
> Besides the above mentioned slowness in comment #22, there isn't a crash this
> time though. The mem usage maxes out at 738M RPRVT with 100% utilization.

So this fixes the crash that allows to overwrite freed memory. Good. I will make a patch for the trunk.
Attached patch trunk fix v1 (obsolete) — Splinter Review
Segmentation faults happen when a newly created  JSScopeProperty is GC-ed when js_GenerateShape detects shape overflow. There are 2 paths to it. 

The first is the js_GenerateShape call from GetPropertyTreeChild when the GC collects freshly allocated property. The patch fixes that via reordering the shape generation to come before the allocation.

The second path is when SCOPE_EXTEND_SHAPE in js_AddScopeProperty calls  js_GenerateShape due to shape overflow before. At that moment sprop is already a part of the property tree but no scopes point to it so it is unrooted. The solution in the patch is to root in this case the sprop. An alternative is to add sprop before calling SCOPE_EXTEND_SHAPE, but that means that the shape stored in the scope would come from lastProp->parent and not lastProp during the GC that SCOPE_EXTEND_SHAPE triggers. I am not sure that this is OK so I went with the tvr rooting approach. But comments are welcomend here.

I monitored the code for other instances of problematic js_GenerateShape calls, but so far the conclusion is that the problem exists only in the two above places.

The patch also fixes the assert in js_GenerateShape. The problem is that when the shape overflows, the GC may collect some properties and bring the shapeGen to SHAPE_OVERFLOW_BIT - 1. It will in turn enable the property cache. Then js_GenerateShape calls JS_ATOMIC_INCREMENT(&rt->shapeGen) bringing the counter to the overflow area with the cache enabled causing the assert. The patch uses a minimal fix for that and disables the property cache after the GC when shapeGen reaches SHAPE_OVERFLOW_BIT - 1, not SHAPE_OVERFLOW_BIT.
Attachment #347539 - Flags: review?(brendan)
Raising priority to P1 as this is a bad GC hazard that allows to free memory for a structure that contains a function pointer which code invokes later.
Priority: -- → P1
(In reply to comment #26)
> Raising priority to P1 as this is a bad GC hazard that allows to free memory
> for a structure that contains a function pointer which code invokes later.

As Igor mentioned, this is high priority, affects trunk too, nominating blocking1.9.1 because of this.
Flags: blocking1.9.1?
Version: 1.9.0 Branch → Trunk
(In reply to comment #17)
> I can reproduce the segfault with the third test case from the comment 14 on
> 64-bit Linux.

Also affects other platforms.
OS: Mac OS X → All
Hardware: PC → All
I will review this tonight.

/be
Comment on attachment 347539 [details] [diff] [review]
trunk fix v1

>Index: 31-ff/js/src/jsinterp.cpp
>===================================================================
>--- 31-ff.orig/js/src/jsinterp.cpp	2008-11-11 17:12:42.000000000 +0100
>+++ 31-ff/js/src/jsinterp.cpp	2008-11-11 17:15:23.000000000 +0100
>@@ -80,27 +80,33 @@
> #endif
> 
> #include "jsautooplen.h"
> 
> /* jsinvoke_cpp___ indicates inclusion from jsinvoke.cpp. */
> #if !JS_LONE_INTERPRET ^ defined jsinvoke_cpp___
> 
> uint32
>-js_GenerateShape(JSContext *cx, JSBool gcLocked)
>+js_GenerateShape(JSContext *cx, JSBool gcLocked,
>+                 JSScopeProperty *spropToRootIfGC)

Super-nit: just sprop as parameter name would work for me.

Looks great otherwise. Nice minimal fixes, thanks. Sorry for the bugs!

/be
Attachment #347539 - Flags: review?(brendan) → review+
Flags: blocking1.9.1? → blocking1.9.1+
Attached patch trunk fix v1bSplinter Review
The new version of the patch fixes the nits. I nominate it for beta 2 to increase the test coverage for the patch that almost as-is would go to the 1.9.0 branch.
Attachment #347539 - Attachment is obsolete: true
Attachment #347834 - Flags: review+
Comment on attachment 347834 [details] [diff] [review]
trunk fix v1b

nominating for b2 for real.
Attachment #347834 - Flags: approval1.9.1b2?
Attachment #347834 - Flags: approval1.9.1b2? → approval1.9.1b2+
landed - http://hg.mozilla.org/mozilla-central/rev/c287e29aeff1
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
(In reply to comment #26)
> Raising priority to P1 as this is a bad GC hazard that allows to free memory
> for a structure that contains a function pointer which code invokes later.

Upping to blocking1.9.0.6 nomination.
Flags: blocking1.9.0.6?
The trunk patch would not apply as-is to 1.9.0 branch due differences in jsgc.c shape overflow logic.
Attachment #348768 - Flags: review?(brendan)
Attachment #347301 - Attachment is obsolete: true
Comment on attachment 348768 [details] [diff] [review]
fix's backport to 1.9.0

r+ based on plain diff of patches.

/be
Attachment #348768 - Flags: review?(brendan) → review+
Attachment #348768 - Flags: approval1.9.0.6?
Flags: in-testsuite+
Flags: in-litmus-
Flags: blocking1.9.0.6? → blocking1.9.0.6+
Attachment #348768 - Flags: approval1.9.0.6? → approval1.9.0.6+
Comment on attachment 348768 [details] [diff] [review]
fix's backport to 1.9.0

Approved for 1.9.0.6, a=dveditz for release-drivers
verified fixed mozilla-central, tracemonkey.
Status: RESOLVED → VERIFIED
Checking in jsgc.c;
/cvsroot/mozilla/js/src/jsgc.c,v  <--  jsgc.c
new revision: 3.303; previous revision: 3.302
done
Checking in jsinterp.c;
/cvsroot/mozilla/js/src/jsinterp.c,v  <--  jsinterp.c
new revision: 3.502; previous revision: 3.501
done
Checking in jsinterp.h;
/cvsroot/mozilla/js/src/jsinterp.h,v  <--  jsinterp.h
new revision: 3.94; previous revision: 3.93
done
Checking in jsscope.c;
/cvsroot/mozilla/js/src/jsscope.c,v  <--  jsscope.c
new revision: 3.88; previous revision: 3.87
done
Checking in jsscope.h;
/cvsroot/mozilla/js/src/jsscope.h,v  <--  jsscope.h
new revision: 3.60; previous revision: 3.59
done
Keywords: fixed1.9.0.6
not an issue on 1.8 branches from what i see.
Flags: wanted1.8.1.x-
Flags: wanted1.8.0.x-
Bob or Gary, can one of you two verify this for 1.9.0.6 with last night's nightly 1.9.0 build? I expect that Gary should do this if he has time as the reporter. I'm not set up to verify this very easily.
js1_8/extensions/regress-452913.js still asserts in 1.9.0 mac os x debug shell.

Assertion failure: JS_PROPERTY_CACHE(cx).disabled == fp->pcDisabledSave, at jsinterp.c:6976

opt shell times out while browser opt/debug fails with
js1_8/extensions/regress-452913.js:49: setting a property that has only a getter.

I double checked on mac by clobbering and rebuilding.

This is different from the original issue, but I wouldn't say this is fixed1.9.0.6.

I didn't see this locally on linux although tomcat did on one of his linux machines earlier Wed. I'm rechecking there and am waiting on fresh winxp builds.
Keywords: fixed1.9.0.6
linux has the same issues as mac.
I forgot that this assertion was already filed as bug 472619. Whether this if fixed1.9.0.6 is open to debate I guess.
verified fixed 1.9.0.6 since this latest assertion is unrelated and predates this problem.
Keywords: verified1.9.0.6
Thanks, Bob.
v 1.9.1, 1.9.2
Whiteboard: [sg:critical?] → [sg:critical?] post 1.8-branch
Alias: CVE-2009-0353
Group: core-security
http://hg.mozilla.org/tracemonkey/rev/00f1f76e409b
/cvsroot/mozilla/js/tests/js1_8/extensions/regress-452913.js,v  <--  regress-452913.js
initial revision: 1.1
Crash Signature: [@ js_SetPropertyHelper]
You need to log in before you can comment on or make changes to this bug.