"Assertion failure: srcArgs.thisv().isMagic(JS_IS_CONSTRUCTING) || &srcArgs.thisv().toObject() == wrapper"

VERIFIED FIXED in Firefox 15

Status

()

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

People

(Reporter: Jesse Ruderman, Assigned: sfink)

Tracking

(Blocks: 2 bugs, {assertion, regression, testcase})

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

Firefox Tracking Flags

(firefox14 unaffected, firefox15 verified, firefox16 verified, firefox-esr10 unaffected)

Details

(Whiteboard: [advisory-tracking-])

Attachments

(1 attachment)

(Reporter)

Description

5 years ago
var w = newGlobal("new-compartment");
var b = (new Int8Array).buffer;
w.DataView(b);

Assertion failure: srcArgs.thisv().isMagic(JS_IS_CONSTRUCTING) || &srcArgs.thisv().toObject() == wrapper, at js/src/jswrapper.cpp:688

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/73b380d3edd8
user:        Steve Fink
date:        Wed Mar 28 14:43:02 2012 -0700
summary:     Bug 741041 - Use UnwrapObjectChecked, and ensure ArrayBufferViews and their buffers are in the same compartment. r=luke
(Assignee)

Comment 1

5 years ago
Oddly, I am seeing a different error:

Assertion failure: isObject(), at /home/sfink/src/MI-typedarray/js/src/jsapi.h:476

but it's almost the same thing, since the isObject() is being called by toObject() in srcArgs.thisv().toObject().

I think the problem is that my custom machinery for dealing with cross-compartment array buffer views doesn't do the right thing wrt constructor calls vs regular calls. If I change the 3rd line to |new w.DataView(b);| it works.
Assignee: general → sphink
Status: NEW → ASSIGNED
(Assignee)

Comment 2

5 years ago
Created attachment 629928 [details] [diff] [review]
Force |this| to be Magic(JS_IS_CONSTRUCTING) when constructing cross-compartment DataViews
Attachment #629928 - Flags: review?(luke)

Comment 3

5 years ago
Comment on attachment 629928 [details] [diff] [review]
Force |this| to be Magic(JS_IS_CONSTRUCTING) when constructing cross-compartment DataViews

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

I think this can be made non-s-s.

::: js/src/jstypedarray.cpp
@@ +2289,5 @@
>  
> +        // Bug 760904: |this| must be forced to JS_IS_CONSTRUCTING because the
> +        // DataView constructor may have been invoked as a normal call, not a
> +        // constructor.
> +        argv[1].setMagic(JS_IS_CONSTRUCTING);

I generally take bug references in comments to indicate "some awful hack" and so, naturally, I don't like to see them anywhere.  This isn't really an awful hack; it's just appeasing a righteous assertion.  I'd just say:

// Appease 'thisv' assertion in CrossCompartmentWrapper::nativeCall
Attachment #629928 - Flags: review?(luke) → review+
(Assignee)

Comment 4

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/91f9aa8d3ac5
Target Milestone: --- → mozilla16
https://hg.mozilla.org/mozilla-central/rev/91f9aa8d3ac5
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
status-firefox16: --- → fixed
Resolution: --- → FIXED
Does this turn out to be a security problem or just an over-eager assertion? Do we want this patch on Aurora(15) and Beta(14)?
status-firefox-esr10: --- → unaffected
status-firefox14: --- → affected
status-firefox15: --- → affected
(Assignee)

Comment 7

5 years ago
Comment on attachment 629928 [details] [diff] [review]
Force |this| to be Magic(JS_IS_CONSTRUCTING) when constructing cross-compartment DataViews

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 741041
User impact if declined: none in opt builds; crash with assertion in debug builds when using a particular pattern of DataView object creation
Testing completed (on m-c, etc.): has been on m-c for quite a while now
Risk to taking this patch (and alternatives if risky): very low
String or UUID changes made by this patch: none

Not a security problem. Not exactly an over-eager assertion, either.

Originally, the assertion was stronger but was never tripped because nothing proxied construction. I weakened the assertion so that I could proxy typed array construction, but failed to mark all instances of construction as being construction (that's the JS_IS_CONSTRUCTOR bit).

None of this landed in 14, so it is unaffected.

This *did* land in 15, and it is affected.
Attachment #629928 - Flags: approval-mozilla-aurora?
(Assignee)

Updated

5 years ago
status-firefox14: affected → unaffected

Comment 8

5 years ago
Comment on attachment 629928 [details] [diff] [review]
Force |this| to be Magic(JS_IS_CONSTRUCTING) when constructing cross-compartment DataViews

[Triage Comment]
New regression in FF15 with a low risk fix, let's land this on Aurora.
Attachment #629928 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(Assignee)

Comment 9

5 years ago
https://hg.mozilla.org/releases/mozilla-aurora/rev/d41d84397116
status-firefox15: affected → fixed
Whiteboard: [advisory-tracking-]
Keywords: verifyme
Confirmed the testcase crashes/asserts 2012-06-02 mozilla-central debug JS shell. Does not crash/assert 2012-06-07 mozilla-central debug JS shell, nor 2012-07-04 mozilla-aurora debug JS shell.
Status: RESOLVED → VERIFIED
status-firefox15: fixed → verified
status-firefox16: fixed → verified
Keywords: verifyme
QA Contact: anthony.s.hughes
Follow-up question, did we want this testcase in-testsuite?
Flags: in-testsuite?
(Assignee)

Comment 12

5 years ago
I included an equivalent testcase in the patch, so it's already in.
(In reply to Steve Fink [:sfink] from comment #12)
> I included an equivalent testcase in the patch, so it's already in.

Thanks Steve. Marking in-testsuite+.
Flags: in-testsuite? → in-testsuite+
Group: core-security
You need to log in before you can comment on or make changes to this bug.