As a security precaution, we have turned on the setting "Require API key authentication for API requests" for everyone. If this has broken something, please contact bugzilla-admin@mozilla.org
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 User image 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 User image Gary Kwong [:gkw] [:nth10sd] 2008-08-30 01:30:35 PDT
Created attachment 336167 [details]
gdb backtraces, normal and full.
Comment 2 User image Mike Shaver (:shaver -- probably not reading bugmail closely) 2008-09-11 21:06:51 PDT
Adding sayrer for assignment help.
Comment 3 User image Igor Bukanov 2008-10-29 17:49:14 PDT
I am looking into it.
Comment 4 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image Brendan Eich [:brendan] 2008-11-11 19:04:25 PST
I will review this tonight.

/be
Comment 30 User image 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 User image 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 User image 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 User image Igor Bukanov 2008-11-17 05:02:19 PST
landed - http://hg.mozilla.org/mozilla-central/rev/c287e29aeff1
Comment 34 User image 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 User image 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 User image Igor Bukanov 2008-11-18 08:14:33 PST
Created attachment 348769 [details]
plain diff between trunk and 1.9.0 patches
Comment 37 User image 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 User image Bob Clary [:bc:] 2008-11-21 11:04:47 PST
Created attachment 349456 [details]
js1_8/extensions/regress-452913.js
Comment 39 User image 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 User image Bob Clary [:bc:] 2008-12-06 04:35:04 PST
verified fixed mozilla-central, tracemonkey.
Comment 41 User image 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 User image Alexander Sack 2009-01-07 06:36:58 PST
not an issue on 1.8 branches from what i see.
Comment 43 User image 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 User image 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 User image Bob Clary [:bc:] 2009-01-15 00:24:34 PST
linux has the same issues as mac.
Comment 46 User image 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 User image 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 User image Al Billings [:abillings] 2009-01-15 12:04:38 PST
Thanks, Bob.
Comment 49 User image Bob Clary [:bc:] 2009-01-16 00:29:05 PST
v 1.9.1, 1.9.2
Comment 50 User image 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.