1357462 - Assertion failure: !denseElementsAreFrozen(), at /home/andre/git/mozilla-central/js/src/vm/NativeObject.h:1055

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[André Bargull [:anba]]

Test case:
---
var p = Object.freeze(["old-value"]);
var a = Object.setPrototypeOf([], p);
print(p[0]);
print(Reflect.set(a, 0, "new-value", p))
print(p[0]);
---

SetExistingProperty (NativeObject.cpp) needs to check |pobj| is frozen instead of |obj|.


And related:
Why do we call the setter-op for inherited slotless properties with |obj| as the this-value? I'd expect either |pobj| or |receiver|.

Comment 1

[André Bargull [:anba]]

Attachment: bug1357462.patch

We need to check with the object on which the property was found instead of using the object which started the [[Set]] call.

Comment 2

[André Bargull [:anba]]

Attachment: bug1357462-testcase.patch

Test case from comment #0.

Comment 3

[André Bargull [:anba]]

Regression from bug 1283334. Similar to bug 1310744 (also allowed to modify a frozen array), rating probably sec-other.

Comment 4

[Jan de Mooij [:jandem]]

Comment on attachment 8859305 [details] [diff] [review]
bug1357462.patch

Review of attachment 8859305 [details] [diff] [review]:
-----------------------------------------------------------------

Good find, thanks!

Comment 5

[Daniel Veditz [:dveditz]]

calling this sec-moderate rather than sec-other on the off chance we rely on freezing objects we inject into content (maybe on android? certainly add-ons might).

Comment 6

[Ryan VanderMeulen [:RyanVM]]

Can this land?

Comment 7

[André Bargull [:anba]]

Do we want/need to backport this to beta/esr52? (Aurora is defunct, right?)

Comment 8

[Ryan VanderMeulen [:RyanVM]]

Aurora is gone, yes. And yes, I think we should nominate for Beta/ESR52 as well after it lands on m-c.

Comment 9

[André Bargull [:anba]]

Ok, then I'll need to prepare a new patch for esr52, because the current patch only applies to beta, but not esr52.

Comment 10

[André Bargull [:anba]]

Attachment: bug1357462-esr52.patch

Combined patch (bug fix + test case) for esr52. Patch is identical to the ones from comment #1 and comment #2, only needed to be updated to apply cleanly on esr52. Carrying r+ from jandem.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/27114f4cd6e7

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/27114f4cd6e7

Comment 13

[Ryan VanderMeulen [:RyanVM]]

I think we're good to nominate this for Beta/ESR52 approval now.

Comment 14

[André Bargull [:anba]]

Attachment: bug1357462-beta.patch

Combined bug1357462.patch + bug1357462-testcase.patch files for beta approval, carrying r+ because it contains no code changes.

Comment 15

[André Bargull [:anba]]

Comment on attachment 8864247 [details] [diff] [review]
bug1357462-beta.patch

Approval Request Comment
[Feature/Bug causing the regression]: bug 1283334
[User impact if declined]: Frozen (non-writable + non-configuable) properties in a JavaScript object can be modified.
[Is this code covered by automated tests?]: Yes.
[Has the fix been verified in Nightly?]: Yes.
[Needs manual test from QE? If yes, steps to reproduce]:  No.
[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: No.
[Why is the change risky/not risky?]: Only changes the lookup object in a single method.
[String changes made/needed]: None.

Comment 16

[André Bargull [:anba]]

Comment on attachment 8862108 [details] [diff] [review]
bug1357462-esr52.patch

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: Frozen (non-writable + non-configuable) properties in a JavaScript object can be modified.
Fix Landed on Version: mozilla55
Risk to taking this patch (and alternatives if risky): No risks are known.
String or UUID changes made by this patch: None.

Comment 17

[Gerry Chang [:gchang]]

Comment on attachment 8864247 [details] [diff] [review]
bug1357462-beta.patch

Fix a security issue and should be low risk. Beta54+ & ESR52+. Should be in 54 beta 5.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/ff1cbde07042

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/38273203b827

Comment 20

[Andrei Vaida [:avaida]]

(In reply to André Bargull from comment #15)
> [Is this code covered by automated tests?]: Yes.
> [Has the fix been verified in Nightly?]: Yes.
> [Needs manual test from QE? If yes, steps to reproduce]:  No.

Setting qe-verify- based on André's assessment on manual testing needs and the fact that this fix has automated coverage.