Closed
Bug 877589
Opened 11 years ago
Closed 11 years ago
Baseline JITcode crash after changing Array.prototype.__proto__
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
mozilla24
Tracking | Status | |
---|---|---|
firefox23 | - | fixed |
firefox24 | --- | fixed |
firefox-esr17 | --- | unaffected |
People
(Reporter: jruderman, Assigned: djvj)
References
Details
(Keywords: crash, regression, testcase)
Attachments
(1 file, 1 obsolete file)
1.70 KB,
patch
|
jandem
:
review+
lsblakk
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
./js --baseline-eager --no-ti function x() { [1]; } Array.prototype.__proto__ = {}; x(); Array.prototype.__proto__ = null; x(); The first bad revision is: changeset: http://hg.mozilla.org/mozilla-central/rev/def96e89be7e user: Jan de Mooij date: Mon Mar 25 17:54:29 2013 +0100 summary: Bug 848743 - Change SetElem_DenseAdd stub to check all shapes on the proto chain. r=djvj
Assignee | ||
Comment 1•11 years ago
|
||
We were forgetting to check if prototype could be null.
Attachment #755985 -
Flags: review?(jdemooij)
Assignee | ||
Comment 2•11 years ago
|
||
Noticed that the same problem exists in the SetProp_NativeAdd compiler.
Attachment #755985 -
Attachment is obsolete: true
Attachment #755985 -
Flags: review?(jdemooij)
Attachment #755997 -
Flags: review?(jdemooij)
Assignee | ||
Updated•11 years ago
|
Assignee: general → kvijayan
Reporter | ||
Comment 3•11 years ago
|
||
Is this a safe null-deref crash?
Comment 4•11 years ago
|
||
Comment on attachment 755997 [details] [diff] [review] Better fix. Review of attachment 755997 [details] [diff] [review]: ----------------------------------------------------------------- Please add the testcase as well. ::: js/src/ion/BaselineIC.cpp @@ +4458,5 @@ > scratchReg = regs.takeAny(); > Register protoReg = regs.takeAny(); > for (size_t i = 0; i < protoChainDepth_; i++) { > masm.loadObjProto(i == 0 ? obj : protoReg, protoReg); > + masm.branchPtr(Assembler::Equal, protoReg, ImmWord((void*)NULL), &failureUnstow); Nit: branchTestPtr(Assembler::Zero, protoReg, protoReg, &failuresUnstow); is a bit more efficient on x64 where using branchPtr with an ImmWord may emit an extra move. @@ +6374,5 @@ > Register protoReg = regs.takeAny(); > // Check the proto chain. > for (size_t i = 0; i < protoChainDepth_; i++) { > masm.loadObjProto(i == 0 ? objReg : protoReg, protoReg); > + masm.branchPtr(Assembler::Equal, protoReg, ImmWord((void*)NULL), &failureUnstow); Same here.
Attachment #755997 -
Flags: review?(jdemooij) → review+
Comment 5•11 years ago
|
||
Why is [1] doing a setelem-ish thing anyway? [] initialization semantics completely ignore the prototype chain. We shouldn't need to be doing any prototype-chain testing here at all.
Assignee | ||
Comment 6•11 years ago
|
||
JSOP_INITELEM takes the same path as JSOP_SETELEM in baseline. It doesn't need to, but we haven't bothered writing a special dense INITELEM stub yet.
Assignee | ||
Comment 7•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/dccef416bb48
Comment 8•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/dccef416bb48
Status: NEW → RESOLVED
Closed: 11 years ago
status-firefox24:
--- → fixed
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
Comment 9•11 years ago
|
||
Firefox 23 is affected by this based on the March 23 regression date. Shouldn't we get this into Aurora (and shouldn't this have gone through sec-approval)?
Updated•11 years ago
|
Comment 10•11 years ago
|
||
Could someone suggest a security rating as well?
Assignee | ||
Comment 11•11 years ago
|
||
This is a pure null pointer read issue, and not controllable by the user. It's a DoS issue. What's the appropriate security rating for that?
Comment 12•11 years ago
|
||
Thanks. Null pointer issues are not security bugs at all :) No rating required.
Group: core-security
Assignee | ||
Comment 13•11 years ago
|
||
Added test case: https://hg.mozilla.org/integration/mozilla-inbound/rev/728ba49f0550
Comment 14•11 years ago
|
||
Not a security critical bug so not tracking but go ahead with a nomination for uplift if this is low risk.
Comment 15•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/728ba49f0550
Flags: in-testsuite? → in-testsuite+
Assignee | ||
Comment 16•11 years ago
|
||
Comment on attachment 755997 [details] [diff] [review] Better fix. [Approval Request Comment] Bug caused by (feature/regressing bug #): Bug 848743 User impact if declined: Attacker-controllable crashes. Testing completed (on m-c, etc.): On mozilla-central for weeks. Risk to taking this patch (and alternatives if risky): Very low. String or IDL/UUID changes made by this patch: N/A
Attachment #755997 -
Flags: approval-mozilla-aurora?
Updated•11 years ago
|
Attachment #755997 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in
before you can comment on or make changes to this bug.
Description
•