452913 - (CVE-2009-0353) Crash [@ js_SetPropertyHelper]

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: 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] (NOT official MoCo now)]

Attachment: gdb backtraces, normal and full.

Comment 2

[Mike Shaver (:shaver emeritus)]

Adding sayrer for assignment help.

Comment 3

[Igor Bukanov]

I am looking into it.

Comment 4

[Igor Bukanov]

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]

> with its this object set to Array.prototype

Why Array.prototype?  Where does that come from?

Comment 6

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: 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] (NOT official MoCo now)]

Attachment: 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] (NOT official MoCo now)]

Attachment: list output from another instance at point of crash

Comment 9

[Igor Bukanov]

(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]

> 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] (NOT official MoCo now)]

$ ./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]

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] (NOT official MoCo now)]

$ ./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]

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] (NOT official MoCo now)]

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] (NOT official MoCo now)]

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]

I can reproduce the segfault with the third test case from the comment 14 on 64-bit Linux.

Comment 18

[Igor Bukanov]

Attachment: 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] (NOT official MoCo now)]

(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]

Attachment: fix for 1.9.0 v2

To Gary: could you test the new patch?

Comment 21

[Igor Bukanov]

Attachment: 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] (NOT official MoCo now)]

(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] (NOT official MoCo now)]

(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]

(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]

Attachment: 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]

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] (NOT official MoCo now)]

(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] (NOT official MoCo now)]

(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]]

I will review this tonight.

/be

Comment 30

[Brendan Eich [:brendan]]

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]

Attachment: 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]

Comment on attachment 347834 [details] [diff] [review]
trunk fix v1b

nominating for b2 for real.

Comment 33

[Igor Bukanov]

landed - http://hg.mozilla.org/mozilla-central/rev/c287e29aeff1

Comment 34

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

(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]

Attachment: 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]

Attachment: plain diff between trunk and 1.9.0 patches

Comment 37

[Brendan Eich [:brendan]]

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] (inactive)]

Attachment: js1_8/extensions/regress-452913.js

Comment 39

[Daniel Veditz [:dveditz]]

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] (inactive)]

verified fixed mozilla-central, tracemonkey.

Comment 41

[Robert Sayre]

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]

not an issue on 1.8 branches from what i see.

Comment 43

[Al Billings [:abillings - ex-MoCo]]

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] (inactive)]

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] (inactive)]

linux has the same issues as mac.

Comment 46

[Bob Clary [:bc] (inactive)]

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] (inactive)]

verified fixed 1.9.0.6 since this latest assertion is unrelated and predates this problem.

Comment 48

[Al Billings [:abillings - ex-MoCo]]

Thanks, Bob.

Comment 49

[Bob Clary [:bc] (inactive)]

v 1.9.1, 1.9.2

Comment 50

[Bob Clary [:bc] (inactive)]

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