877589 - Baseline JITcode crash after changing Array.prototype.__proto__

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jesse Ruderman]

./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

Comment 1

[Kannan Vijayan [:djvj]]

Attachment: Fix.

We were forgetting to check if prototype could be null.

Comment 2

[Kannan Vijayan [:djvj]]

Attachment: Better fix.

Noticed that the same problem exists in the SetProp_NativeAdd compiler.

Comment 3

[Jesse Ruderman]

Is this a safe null-deref crash?

Comment 4

[Jan de Mooij [:jandem]]

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.

Comment 5

[Jeff Walden [:Waldo]]

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.

Comment 6

[Kannan Vijayan [:djvj]]

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.

Comment 7

[Kannan Vijayan [:djvj]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/dccef416bb48

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/dccef416bb48

Comment 9

[Al Billings [:abillings - ex-MoCo]]

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)?

Comment 10

[Al Billings [:abillings - ex-MoCo]]

Could someone suggest a security rating as well?

Comment 11

[Kannan Vijayan [:djvj]]

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

[Christian Holler (:decoder)]

Thanks. Null pointer issues are not security bugs at all :) No rating required.

Comment 13

[Kannan Vijayan [:djvj]]

Added test case:
https://hg.mozilla.org/integration/mozilla-inbound/rev/728ba49f0550

Comment 14

[Lukas Blakk [:lsblakk] use ?needinfo]

Not a security critical bug so not tracking but go ahead with a nomination for uplift if this is low risk.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/728ba49f0550

Comment 16

[Kannan Vijayan [:djvj]]

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

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/9c2936a07d35