Last Comment Bug 452913 - (CVE-2009-0353) Crash [@ js_SetPropertyHelper]
(CVE-2009-0353)
: Crash [@ js_SetPropertyHelper]
Status: VERIFIED FIXED
[sg:critical?] post 1.8-branch
: assertion, crash, testcase, verified1.9.0.6, verified1.9.1
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
: P1 critical (vote)
: ---
Assigned To: Igor Bukanov
:
: Jason Orendorff [:jorendorff]
Mentors:
Depends on:
Blocks: jsfunfuzz
  Show dependency treegraph
 
Reported: 2008-08-30 01:07 PDT by Gary Kwong [:gkw] [:nth10sd]
Modified: 2011-06-13 10:01 PDT (History)
13 users (show)
sayrer: blocking1.9.1+
dveditz: blocking1.9.0.6+
dveditz: wanted1.9.0.x+
asac: wanted1.8.1.x-
asac: wanted1.8.0.x-
bob: in‑testsuite+
bob: in‑litmus-
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
crash log (2.79 KB, text/plain)
2008-08-30 01:07 PDT, Gary Kwong [:gkw] [:nth10sd]
no flags Details
gdb backtraces, normal and full. (19.68 KB, text/plain)
2008-08-30 01:30 PDT, Gary Kwong [:gkw] [:nth10sd]
no flags Details
updated crash log (2.85 KB, text/plain)
2008-10-30 19:41 PDT, Gary Kwong [:gkw] [:nth10sd]
no flags Details
updated backtraces, normal and full, and |list| output ten times (22.49 KB, text/plain)
2008-10-30 19:46 PDT, Gary Kwong [:gkw] [:nth10sd]
no flags Details
list output from another instance at point of crash (2.36 KB, text/plain)
2008-10-30 19:50 PDT, Gary Kwong [:gkw] [:nth10sd]
no flags Details
fix for 1.9 branch (2.58 KB, patch)
2008-11-01 22:46 PDT, Igor Bukanov
no flags Details | Diff | Splinter Review
fix for 1.9.0 v2 (7.86 KB, patch)
2008-11-10 09:14 PST, Igor Bukanov
no flags Details | Diff | Splinter Review
fix for 1.9.0 v3 (6.66 KB, patch)
2008-11-10 09:20 PST, Igor Bukanov
no flags Details | Diff | Splinter Review
trunk fix v1 (6.53 KB, patch)
2008-11-11 08:54 PST, Igor Bukanov
brendan: review+
Details | Diff | Splinter Review
trunk fix v1b (6.58 KB, patch)
2008-11-12 13:18 PST, Igor Bukanov
igor: review+
sayrer: approval1.9.1b2+
Details | Diff | Splinter Review
fix's backport to 1.9.0 (6.71 KB, patch)
2008-11-18 08:13 PST, Igor Bukanov
brendan: review+
dveditz: approval1.9.0.6+
Details | Diff | Splinter Review
plain diff between trunk and 1.9.0 patches (3.53 KB, text/plain)
2008-11-18 08:14 PST, Igor Bukanov
no flags Details
js1_8/extensions/regress-452913.js (2.19 KB, text/plain)
2008-11-21 11:04 PST, Bob Clary [:bc:]
no flags Details

Description Gary Kwong [:gkw] [:nth10sd] 2008-08-30 01:07:51 PDT
Created attachment 336165 [details]
crash log

(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.
Comment 1 Gary Kwong [:gkw] [:nth10sd] 2008-08-30 01:30:35 PDT
Created attachment 336167 [details]
gdb backtraces, normal and full.
Comment 2 Mike Shaver (:shaver -- probably not reading bugmail closely) 2008-09-11 21:06:51 PDT
Adding sayrer for assignment help.
Comment 3 Igor Bukanov 2008-10-29 17:49:14 PDT
I am looking into it.
Comment 4 Igor Bukanov 2008-10-30 16:37:44 PDT
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?
Comment 5 Jesse Ruderman 2008-10-30 18:48:12 PDT
> with its this object set to Array.prototype

Why Array.prototype?  Where does that come from?
Comment 6 Gary Kwong [:gkw] [:nth10sd] 2008-10-30 19:41:53 PDT
Created attachment 345651 [details]
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?)
Comment 7 Gary Kwong [:gkw] [:nth10sd] 2008-10-30 19:46:39 PDT
Created attachment 345652 [details]
updated backtraces, normal and full, and |list| output ten times

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.
Comment 8 Gary Kwong [:gkw] [:nth10sd] 2008-10-30 19:50:07 PDT
Created attachment 345653 [details]
list output from another instance at point of crash
Comment 9 Igor Bukanov 2008-10-31 11:05:39 PDT
(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.
Comment 10 Igor Bukanov 2008-10-31 11:39:07 PDT
> 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.
Comment 11 Gary Kwong [:gkw] [:nth10sd] 2008-10-31 16:25:26 PDT
$ ./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.
Comment 12 Igor Bukanov 2008-10-31 16:44:40 PDT
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]);
Comment 13 Gary Kwong [:gkw] [:nth10sd] 2008-10-31 16:49:19 PDT
$ ./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.
Comment 14 Igor Bukanov 2008-10-31 17:06:53 PDT
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);
}
Comment 15 Gary Kwong [:gkw] [:nth10sd] 2008-10-31 17:56:24 PDT
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
Comment 16 Gary Kwong [:gkw] [:nth10sd] 2008-10-31 18:00:09 PDT
Note: This causes the slow script dialog to come up -- but Fx 3.1 beta 1 then crashes if the script is continued.
Comment 17 Igor Bukanov 2008-11-01 20:47:29 PDT
I can reproduce the segfault with the third test case from the comment 14 on 64-bit Linux.
Comment 18 Igor Bukanov 2008-11-01 22:46:21 PDT
Created attachment 345914 [details] [diff] [review]
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?
Comment 19 Gary Kwong [:gkw] [:nth10sd] 2008-11-02 02:13:28 PST
(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
Comment 20 Igor Bukanov 2008-11-10 09:14:55 PST
Created attachment 347299 [details] [diff] [review]
fix for 1.9.0 v2

To Gary: could you test the new patch?
Comment 21 Igor Bukanov 2008-11-10 09:20:03 PST
Created attachment 347301 [details] [diff] [review]
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?
Comment 22 Gary Kwong [:gkw] [:nth10sd] 2008-11-10 09:35:27 PST
(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?
Comment 23 Gary Kwong [:gkw] [:nth10sd] 2008-11-10 09:42:32 PST
(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.
Comment 24 Igor Bukanov 2008-11-10 12:03:37 PST
(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.
Comment 25 Igor Bukanov 2008-11-11 08:54:33 PST
Created attachment 347539 [details] [diff] [review]
trunk fix v1

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.
Comment 26 Igor Bukanov 2008-11-11 09:00:58 PST
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.
Comment 27 Gary Kwong [:gkw] [:nth10sd] 2008-11-11 18:29:14 PST
(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.
Comment 28 Gary Kwong [:gkw] [:nth10sd] 2008-11-11 18:30:02 PST
(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.
Comment 29 Brendan Eich [:brendan] 2008-11-11 19:04:25 PST
I will review this tonight.

/be
Comment 30 Brendan Eich [:brendan] 2008-11-12 12:27:06 PST
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
Comment 31 Igor Bukanov 2008-11-12 13:18:35 PST
Created attachment 347834 [details] [diff] [review]
trunk fix v1b

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.
Comment 32 Igor Bukanov 2008-11-14 09:25:18 PST
Comment on attachment 347834 [details] [diff] [review]
trunk fix v1b

nominating for b2 for real.
Comment 33 Igor Bukanov 2008-11-17 05:02:19 PST
landed - http://hg.mozilla.org/mozilla-central/rev/c287e29aeff1
Comment 34 Gary Kwong [:gkw] [:nth10sd] 2008-11-17 08:36:46 PST
(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.
Comment 35 Igor Bukanov 2008-11-18 08:13:25 PST
Created attachment 348768 [details] [diff] [review]
fix's backport to 1.9.0

The trunk patch would not apply as-is to 1.9.0 branch due differences in jsgc.c shape overflow logic.
Comment 36 Igor Bukanov 2008-11-18 08:14:33 PST
Created attachment 348769 [details]
plain diff between trunk and 1.9.0 patches
Comment 37 Brendan Eich [:brendan] 2008-11-18 12:01:35 PST
Comment on attachment 348768 [details] [diff] [review]
fix's backport to 1.9.0

r+ based on plain diff of patches.

/be
Comment 38 Bob Clary [:bc:] 2008-11-21 11:04:47 PST
Created attachment 349456 [details]
js1_8/extensions/regress-452913.js
Comment 39 Daniel Veditz [:dveditz] 2008-12-05 11:51:41 PST
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
Comment 40 Bob Clary [:bc:] 2008-12-06 04:35:04 PST
verified fixed mozilla-central, tracemonkey.
Comment 41 Robert Sayre 2009-01-06 20:32:37 PST
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
Comment 42 Alexander Sack 2009-01-07 06:36:58 PST
not an issue on 1.8 branches from what i see.
Comment 43 Al Billings [:abillings] 2009-01-14 13:22:41 PST
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.
Comment 44 Bob Clary [:bc:] 2009-01-15 00:12:52 PST
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.
Comment 45 Bob Clary [:bc:] 2009-01-15 00:24:34 PST
linux has the same issues as mac.
Comment 46 Bob Clary [:bc:] 2009-01-15 00:42:54 PST
I forgot that this assertion was already filed as bug 472619. Whether this if fixed1.9.0.6 is open to debate I guess.
Comment 47 Bob Clary [:bc:] 2009-01-15 00:56:38 PST
verified fixed 1.9.0.6 since this latest assertion is unrelated and predates this problem.
Comment 48 Al Billings [:abillings] 2009-01-15 12:04:38 PST
Thanks, Bob.
Comment 49 Bob Clary [:bc:] 2009-01-16 00:29:05 PST
v 1.9.1, 1.9.2
Comment 50 Bob Clary [:bc:] 2009-03-14 15:50:36 PDT
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

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