Bug 452913 (CVE-2009-0353)

Crash [@ js_SetPropertyHelper]

VERIFIED FIXED

Status

()

Core
JavaScript Engine
P1
critical
VERIFIED FIXED
9 years ago
6 years ago

People

(Reporter: gkw, Assigned: Igor Bukanov)

Tracking

(Blocks: 1 bug, 5 keywords)

Trunk
assertion, crash, testcase, verified1.9.0.6, verified1.9.1
Points:
---
Bug Flags:
blocking1.9.1 +
blocking1.9.0.6 +
wanted1.9.0.x +
wanted1.8.1.x -
wanted1.8.0.x -
in-testsuite +
in-litmus -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:critical?] post 1.8-branch, crash signature)

Attachments

(7 attachments, 6 obsolete attachments)

(Reporter)

Description

9 years ago
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.
Flags: blocking1.9.0.3?
(Reporter)

Comment 1

9 years ago
Created attachment 336167 [details]
gdb backtraces, normal and full.
Flags: blocking1.9.0.3? → wanted1.9.0.x+
Whiteboard: [sg:critical?]
Adding sayrer for assignment help.

Updated

9 years ago
QA Contact: general → igor

Updated

9 years ago
Assignee: general → igor
QA Contact: igor → general
(Assignee)

Comment 3

9 years ago
I am looking into it.
(Assignee)

Comment 4

9 years ago
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

9 years ago
> with its this object set to Array.prototype

Why Array.prototype?  Where does that come from?
(Reporter)

Comment 6

9 years ago
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?)
Attachment #336165 - Attachment is obsolete: true
(Reporter)

Comment 7

9 years ago
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.
Attachment #336167 - Attachment is obsolete: true
(Reporter)

Comment 8

9 years ago
Created attachment 345653 [details]
list output from another instance at point of crash
(Assignee)

Comment 9

9 years ago
(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.
(Assignee)

Comment 10

9 years ago
> 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.
(Reporter)

Comment 11

9 years ago
$ ./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.
(Assignee)

Comment 12

9 years ago
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]);
(Reporter)

Comment 13

9 years ago
$ ./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.
(Assignee)

Comment 14

9 years ago
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);
}
(Reporter)

Comment 15

9 years ago
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
(Reporter)

Comment 16

9 years ago
Note: This causes the slow script dialog to come up -- but Fx 3.1 beta 1 then crashes if the script is continued.
(Assignee)

Comment 17

9 years ago
I can reproduce the segfault with the third test case from the comment 14 on 64-bit Linux.
(Assignee)

Comment 18

9 years ago
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?
(Reporter)

Comment 19

9 years ago
(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
(Assignee)

Comment 20

9 years ago
Created attachment 347299 [details] [diff] [review]
fix for 1.9.0 v2

To Gary: could you test the new patch?
(Assignee)

Comment 21

9 years ago
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?
Attachment #345914 - Attachment is obsolete: true
Attachment #347299 - Attachment is obsolete: true
(Reporter)

Comment 22

9 years ago
(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?
(Reporter)

Comment 23

9 years ago
(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.
(Assignee)

Comment 24

9 years ago
(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.
(Assignee)

Comment 25

9 years ago
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.
Attachment #347539 - Flags: review?(brendan)
(Assignee)

Comment 26

9 years ago
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
(Reporter)

Comment 27

9 years ago
(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
(Reporter)

Comment 28

9 years ago
(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+

Updated

9 years ago
Flags: blocking1.9.1? → blocking1.9.1+
(Assignee)

Comment 31

9 years ago
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.
Attachment #347539 - Attachment is obsolete: true
Attachment #347834 - Flags: review+
(Assignee)

Comment 32

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

nominating for b2 for real.
Attachment #347834 - Flags: approval1.9.1b2?

Updated

9 years ago
Attachment #347834 - Flags: approval1.9.1b2? → approval1.9.1b2+
(Assignee)

Comment 33

9 years ago
landed - http://hg.mozilla.org/mozilla-central/rev/c287e29aeff1
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
(Reporter)

Comment 34

9 years ago
(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?
(Assignee)

Comment 35

9 years ago
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.
Attachment #348768 - Flags: review?(brendan)
(Assignee)

Comment 36

9 years ago
Created attachment 348769 [details]
plain diff between trunk and 1.9.0 patches
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+
(Assignee)

Updated

9 years ago
Attachment #348768 - Flags: approval1.9.0.6?

Comment 38

9 years ago
Created attachment 349456 [details]
js1_8/extensions/regress-452913.js

Updated

9 years ago
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

Comment 40

9 years ago
verified fixed mozilla-central, tracemonkey.
Status: RESOLVED → VERIFIED
Keywords: fixed1.9.1

Comment 41

9 years ago
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

Comment 42

9 years ago
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.

Comment 44

9 years ago
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

Comment 45

9 years ago
linux has the same issues as mac.

Comment 46

9 years ago
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

9 years ago
verified fixed 1.9.0.6 since this latest assertion is unrelated and predates this problem.
Keywords: verified1.9.0.6
Thanks, Bob.

Comment 49

9 years ago
v 1.9.1, 1.9.2
Keywords: fixed1.9.1 → verified1.9.1
Whiteboard: [sg:critical?] → [sg:critical?] post 1.8-branch
Alias: CVE-2009-0353
Group: core-security

Comment 50

8 years ago
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.