Boundary condition error in Array.prototype.push implementation for non-arrays near max-array-index limit

VERIFIED FIXED

Status

()

Core
JavaScript Engine
P2
critical
VERIFIED FIXED
9 years ago
9 years ago

People

(Reporter: Waldo, Assigned: Waldo)

Tracking

(5 keywords)

Trunk
assertion, crash, regression, verified1.9.0.12, verified1.9.1
Points:
---
Bug Flags:
blocking1.9.1 +
blocking1.9.0.12 +
wanted1.9.0.x +
wanted1.8.1.x -
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:critical?][1.9.0 patch in bug 465980] fixed-in-tracemonkey, URL)

Attachments

(3 attachments)

(Assignee)

Description

9 years ago
h-118:~/moz/shell-js/js/src jwalden$ dbg/js -j
js> javascript: var stack = { push: [].push }; stack.length = Math.pow(2, 37); stack.push(-2, -1, 0);
3
js> javascript: var stack = { push: [].push }; stack.length = Math.pow(2, 5); stack.push(-2, -1, 0);
35
js> javascript: var stack = { push: [].push }; stack.length = Math.pow(2, 32) - 2; stack.push(-2, -1, 0);
Assertion failure: OBJ_GET_CLASS(cx, obj) == &js_ArrayClass, at ../jsarray.cpp:1255
Trace/BPT trap

Fallout from bug 465980 and how property-set loops are optimized, looks to be a very quick crash afterward when we try to set properties and end up dereferencing the thoroughly bogus JSClass* associated with the "slowified array" -- but at least in the above we end up calling a resolve hook through that clasp, and that could go anywhere depending on the initial JSClass of |stack|.  Maybe we'd be lucky and the resolves would all go astray in non-exploitable ways for all the possible class pointers we might have here, but that's a dangerous assumption.  At least with the above:

(gdb) p resolve
$11 = (JSResolveOp) 0xbb7f0 <Decompile(SprintStack*, unsigned char*, int, JSOp)::exception_cookie+8332>

and then continuing into there we see real executable code (which thankfully hits a privileged instruction fairly quickly, but still mega scary), which counsels paranoia.

(gdb) disas $pc $pc+50
Dump of assembler code from 0xbb7f0 to 0xbb822:
0x000bb7f0 <_ZZ9DecompileP11SprintStackPhi4JSOpE16exception_cookie+8332>:       push   %esp
0x000bb7f1 <_ZZ9DecompileP11SprintStackPhi4JSOpE16exception_cookie+8333>:       push   %eax
0x000bb7f2 <_ZZ9DecompileP11SprintStackPhi4JSOpE16exception_cookie+8334>:       inc    %ebx
0x000bb7f3 <_ZZ9DecompileP11SprintStackPhi4JSOpE16exception_cookie+8335>:       add    %dh,105(%esi)
0x000bb7f6 <_ZZ9DecompileP11SprintStackPhi4JSOpE16exception_cookie+8338>:       add    %al,(%eax)
0x000bb7f8 <_ZZ9DecompileP11SprintStackPhi4JSOpE16exception_cookie+8340>:       jbe    0xbb86d <js_NaN_str+1>
0x000bb7fa <_ZZ9DecompileP11SprintStackPhi4JSOpE16exception_cookie+8342>:       add    %al,(%eax)
0x000bb7fc <_ZZ9DecompileP11SprintStackPhi4JSOpE16exception_cookie+8344>:       jae    0xbb863 <js_isFinite_str+11>
0x000bb7fe <_ZZ9DecompileP11SprintStackPhi4JSOpE16exception_cookie+8346>:       je     0xbb85f <js_isFinite_str+7>
0x000bb800 <_ZZ9DecompileP11SprintStackPhi4JSOpE16exception_cookie+8348>:       jae    0xbb86e <js_NaN_str+2>
0x000bb802 <_ZZ9DecompileP11SprintStackPhi4JSOpE16exception_cookie+8350>:       outsl  %ds:(%esi),(%dx)

This problem was originally discovered via these pages, the former directing to the latter:

http://hexmen.com/blog/2006/12/push-and-pop/
http://hexmen.com/tests/pushpop.html

There may be other failures here beyond this immediate one, but I haven't investigated any beyond this.  Note that that prior tests to the one that demonstrate this error contain a few bugs: there should be no ToUint32() performed when increasing the index variable in push, only when reading the intial length property value.  Consequently, take failure with a grain of salt until investigated.
Flags: blocking1.9.1?
(Assignee)

Comment 1

9 years ago
Created attachment 373482 [details]
Original tests as a shell testcase
(Assignee)

Updated

9 years ago
Whiteboard: [sg:critical?]
(Assignee)

Comment 2

9 years ago
Created attachment 373485 [details] [diff] [review]
Patch
Attachment #373485 - Flags: review?(jorendorff)
(Assignee)

Comment 3

9 years ago
I looked at the other failures, and they're all from push with an index that the test expects to wrap around but which doesn't, and then the push fails when the final length-set is attempted.

Updated

9 years ago
Priority: -- → P2

Updated

9 years ago
Flags: blocking1.9.1? → blocking1.9.1+
Comment on attachment 373485 [details] [diff] [review]
Patch

Someday, once we're out from under LiveConnect's dependence on C headers, I hope we'll have a JSArray type (derived from JSObject, no virtual methods, just for type safety), and this kind of thing will be rarer.
Attachment #373485 - Flags: review?(jorendorff) → review+
(Assignee)

Comment 5

9 years ago
http://hg.mozilla.org/tracemonkey/rev/8466bcdbd346
Whiteboard: [sg:critical?] → [sg:critical?] fixed-in-tracemonkey

Comment 6

9 years ago
http://hg.mozilla.org/mozilla-central/rev/8466bcdbd346
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
Needed in 1.9.0.x if we take bug 465980
Flags: wanted1.9.0.x+
Flags: blocking1.9.0.11?
Keywords: regression
Flags: blocking1.9.0.11? → blocking1.9.0.12?
Flags: blocking1.9.0.12? → blocking1.9.0.12+

Comment 9

9 years ago
Created attachment 381469 [details]
ecma_3/Array/regress-488989.js

Updated

9 years ago
Flags: in-testsuite+
Whiteboard: [sg:critical?] fixed-in-tracemonkey → [sg:critical?][1.9.0 patch in bug 465980] fixed-in-tracemonkey
Keywords: fixed1.9.0.12
Bob, can you verify this for 1.9.0.12?

Comment 11

9 years ago
v 1.9.0, 1.9.1, 1.9.2
Status: RESOLVED → VERIFIED
Keywords: fixed1.9.0.12, fixed1.9.1 → verified1.9.0.12, verified1.9.1
Flags: wanted1.8.1.x-
Group: core-security

Comment 12

9 years ago
http://hg.mozilla.org/tracemonkey/rev/2bedd496fa14
/cvsroot/mozilla/js/tests/ecma_3/Array/regress-488989.js,v  <--  regress-488989.js
initial revision: 1.1
You need to log in before you can comment on or make changes to this bug.