Closed Bug 1357462 Opened 3 years ago Closed 3 years ago

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

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox-esr45 --- unaffected
firefox-esr52 54+ fixed
firefox53 --- wontfix
firefox54 + fixed
firefox55 + fixed

People

(Reporter: anba, Assigned: anba)

References

Details

(Keywords: regression, sec-moderate, Whiteboard: [adv-main54+][adv-esr52.2+])

Attachments

(4 files)

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|.
Attached patch bug1357462.patchSplinter Review
We need to check with the object on which the property was found instead of using the object which started the [[Set]] call.
Assignee: nobody → andrebargull
Status: NEW → ASSIGNED
Attachment #8859305 - Flags: review?(jdemooij)
Test case from comment #0.
Attachment #8859577 - Flags: review?(jdemooij)
Regression from bug 1283334. Similar to bug 1310744 (also allowed to modify a frozen array), rating probably sec-other.
Comment on attachment 8859305 [details] [diff] [review]
bug1357462.patch

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

Good find, thanks!
Attachment #8859305 - Flags: review?(jdemooij) → review+
Attachment #8859577 - Flags: review?(jdemooij) → review+
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).
Blocks: 1283334
Group: core-security → javascript-core-security
Can this land?
Flags: needinfo?(andrebargull)
Flags: in-testsuite?
Do we want/need to backport this to beta/esr52? (Aurora is defunct, right?)
Aurora is gone, yes. And yes, I think we should nominate for Beta/ESR52 as well after it lands on m-c.
Ok, then I'll need to prepare a new patch for esr52, because the current patch only applies to beta, but not esr52.
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.
Flags: needinfo?(andrebargull)
Attachment #8862108 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/27114f4cd6e7
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Group: javascript-core-security → core-security-release
I think we're good to nominate this for Beta/ESR52 approval now.
Flags: needinfo?(andrebargull)
Combined bug1357462.patch + bug1357462-testcase.patch files for beta approval, carrying r+ because it contains no code changes.
Attachment #8864247 - Flags: review+
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.
Flags: needinfo?(andrebargull)
Attachment #8864247 - Flags: approval-mozilla-beta?
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.
Attachment #8862108 - Flags: approval-mozilla-esr52?
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.
Attachment #8864247 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8862108 - Flags: approval-mozilla-esr52? → approval-mozilla-esr52+
(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.
Flags: qe-verify-
Whiteboard: [adv-main54+][adv-esr52.2+]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.