Closed Bug 877589 Opened 7 years ago Closed 7 years ago

Baseline JITcode crash after changing Array.prototype.__proto__

Categories

(Core :: JavaScript Engine, defect, critical)

x86_64
macOS
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla24
Tracking Status
firefox23 - fixed
firefox24 --- fixed
firefox-esr17 --- unaffected

People

(Reporter: jruderman, Assigned: djvj)

References

(Blocks 1 open bug)

Details

(Keywords: crash, regression, testcase)

Attachments

(1 file, 1 obsolete file)

./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
Attached patch Fix. (obsolete) — Splinter Review
We were forgetting to check if prototype could be null.
Attachment #755985 - Flags: review?(jdemooij)
Attached patch Better fix.Splinter Review
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: general → kvijayan
Is this a safe null-deref crash?
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+
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.
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.
https://hg.mozilla.org/mozilla-central/rev/dccef416bb48
Status: NEW → RESOLVED
Closed: 7 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
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)?
Could someone suggest a security rating as well?
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?
Thanks. Null pointer issues are not security bugs at all :) No rating required.
Group: core-security
Not a security critical bug so not tracking but go ahead with a nomination for uplift if this is low risk.
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?
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.