Baseline JITcode crash after changing Array.prototype.__proto__

RESOLVED FIXED in Firefox 23

Status

()

Core
JavaScript Engine
--
critical
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Jesse Ruderman, Assigned: djvj)

Tracking

(Blocks: 1 bug, {crash, regression, testcase})

Trunk
mozilla24
x86_64
Mac OS X
crash, regression, testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox23- fixed, firefox24 fixed, firefox-esr17 unaffected)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

5 years ago
./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

5 years ago
Created attachment 755985 [details] [diff] [review]
Fix.

We were forgetting to check if prototype could be null.
Attachment #755985 - Flags: review?(jdemooij)
(Assignee)

Comment 2

5 years ago
Created attachment 755997 [details] [diff] [review]
Better fix.

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

5 years ago
Assignee: general → kvijayan
(Reporter)

Comment 3

5 years ago
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+

Comment 5

5 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

5 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.
https://hg.mozilla.org/mozilla-central/rev/dccef416bb48
Status: NEW → RESOLVED
Last Resolved: 5 years ago
status-firefox24: --- → fixed
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla24

Comment 9

5 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

5 years ago
status-firefox23: --- → affected
status-firefox-esr17: --- → unaffected
tracking-firefox23: --- → ?

Comment 10

5 years ago
Could someone suggest a security rating as well?
(Assignee)

Comment 11

5 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?
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.
tracking-firefox23: ? → -
(Assignee)

Comment 16

5 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?
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.