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

VERIFIED FIXED

Status

()

defect
P2
critical
VERIFIED FIXED
11 years ago
10 years ago

People

(Reporter: Waldo, Assigned: Waldo)

Tracking

(5 keywords)

Dependency tree / graph
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, )

Attachments

(3 attachments)

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?
Whiteboard: [sg:critical?]
Posted patch PatchSplinter Review
Attachment #373485 - Flags: review?(jorendorff)
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.
Priority: -- → P2
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+
http://hg.mozilla.org/tracemonkey/rev/8466bcdbd346
Whiteboard: [sg:critical?] → [sg:critical?] fixed-in-tracemonkey
http://hg.mozilla.org/mozilla-central/rev/8466bcdbd346
Status: ASSIGNED → RESOLVED
Closed: 11 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+
Flags: in-testsuite+
Whiteboard: [sg:critical?] fixed-in-tracemonkey → [sg:critical?][1.9.0 patch in bug 465980] fixed-in-tracemonkey
Bob, can you verify this for 1.9.0.12?
v 1.9.0, 1.9.1, 1.9.2
Status: RESOLVED → VERIFIED
Flags: wanted1.8.1.x-
Group: core-security
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.