Crash [@ js::EncapsulatedPtr] or [@ js::types::TypeObject::print] or "Assertion failure: [infer failure] Missing type in object [0x10172f070] lastIndex: int,"

VERIFIED FIXED in Firefox 19

Status

()

defect
--
critical
VERIFIED FIXED
7 years ago
6 years ago

People

(Reporter: gkw, Assigned: bhackett)

Tracking

(Blocks 1 bug, 5 keywords)

Trunk
mozilla21
x86_64
macOS
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(firefox17 wontfix, firefox18- wontfix, firefox19+ fixed, firefox20+ fixed, firefox21 fixed, firefox-esr10 unaffected, firefox-esr1719+ fixed, b2g1819+ fixed, b2g18-v1.0.0 wontfix, b2g18-v1.0.1 fixed)

Details

(Whiteboard: [jsbugmon:update][adv-main19+][adv-esr1703+], crash signature)

Attachments

(4 attachments)

Posted file stack
RegExp.prototype.lastIndex = this
gc()
newGlobal()
eval("RegExp().lastIndex")

crashes js debug shell on m-c changeset fd8fd1a7aecd with -a at js::EncapsulatedPtr

During the process of reduction, the following assert showed up, also with -a, Assertion failure: [infer failure] Missing type in object [0x10172f070] lastIndex: int, with the testcase:

function g() {
    z = newGlobal('')
    return function(code) {
        evalcx(code, z)
    }
}
f = g()
f("this.RegExp.prototype.lastIndex=this")
gc()
f("RegExp().lastIndex")


s-s and sec-critical because it's a type inference failure and involves gc, feel free to change this when shown not to be s-s.


autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   108156:7228effb2e5b
user:        Bill McCloskey
date:        Mon Sep 03 16:42:22 2012 -0700
summary:     Bug 787856 - Use lazy protos for cross-compartment wrappers (r=bholley)
The regressing patch autoBisect pinpointed in comment 0 was for the crash in the first testcase.

Here's another regression window for the assert (2nd testcase).

Bill, I'm not sure which is correct, is this a GC or TI bug?
Flags: needinfo?(wmccloskey)
Assignee: general → wmccloskey
Sometimes js::types::TypeObject::print is on the stack.
Crash Signature: [@ js::EncapsulatedPtr] → [@ js::EncapsulatedPtr] [@ js::types::TypeObject::print]
Summary: Crash [@ js::EncapsulatedPtr] or "Assertion failure: [infer failure] Missing type in object [0x10172f070] lastIndex: int," → Crash [@ js::EncapsulatedPtr] or [@ js::types::TypeObject::print] or "Assertion failure: [infer failure] Missing type in object [0x10172f070] lastIndex: int,"
If 22c9c298a21f is the correct regressing changeset, then 18 and later is affected, and 17 ESR and prior are not. Setting flags based on this assumption, please feel free to change if necessary.
This fixes a problem that was introduced by the lazy proto patch. However, it's not the real fix, since we still assert after it's applied.
Attachment #699370 - Flags: review?(bhackett1024)
Flags: needinfo?(wmccloskey)
Brian said he would take this. It looks like RegExp objects aren't informing TI when they initialize properties like lastIndex. Brian says that StringObject may have a similar problem.
Assignee: wmccloskey → bhackett1024
Attachment #699370 - Flags: review?(bhackett1024) → review+
Posted patch fixSplinter Review
The blame revision is wrong for the underlying problem, this goes back to objshrink I think.  We add type information for the builtin properties of String and RegExp objects, but only do so during class initialization.  If the type of those default String/RegExp objects is later destroyed, that information is lost and is not regenerated.  The attached patch fixes this by ensuring that whenever the type for a RegExp or String object is created, type information for the builtin properties is added.
Attachment #699389 - Flags: review?(wmccloskey)
Attachment #699389 - Flags: review?(wmccloskey) → review+
Comment on attachment 699389 [details] [diff] [review]
fix

[Security approval request comment]
> How easily could an exploit be constructed based on the patch?

Rather difficult, requires knowledge of how the modified parts of the code interact with each other and GC.

> Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

No

> Which older supported branches are affected by this flaw?

esr 17

> Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

trivial

> How likely is this patch to cause regressions; how much testing does it need?

none
Attachment #699389 - Flags: sec-approval?
No longer blocks: 787856
Attachment #699389 - Flags: sec-approval? → sec-approval+
My patch doesn't need sec-approval because it only affects DEBUG builds.
https://hg.mozilla.org/mozilla-central/rev/f77b007d48bf
https://hg.mozilla.org/mozilla-central/rev/7ada213f01bd
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
Status: RESOLVED → VERIFIED
Crash Signature: [@ js::EncapsulatedPtr] [@ js::types::TypeObject::print] → [@ js::EncapsulatedPtr] [@ js::types::TypeObject::print]
JSBugMon: This bug has been automatically verified fixed.
Comment on attachment 699389 [details] [diff] [review]
fix

[Approval Request Comment]
Bug caused by (feature/regressing bug #): objshrink
User impact if declined: potential security vulnerability
Testing completed (on m-c, etc.): on m-c
Risk to taking this patch (and alternatives if risky): none
String or UUID changes made by this patch: none
Attachment #699389 - Flags: approval-mozilla-beta?
Attachment #699389 - Flags: approval-mozilla-aurora?
Crash Signature: [@ js::EncapsulatedPtr] [@ js::types::TypeObject::print] → [@ js::EncapsulatedPtr] [@ js::types::TypeObject::print]
Attachment #699389 - Flags: approval-mozilla-beta?
Attachment #699389 - Flags: approval-mozilla-beta+
Attachment #699389 - Flags: approval-mozilla-aurora?
Attachment #699389 - Flags: approval-mozilla-aurora+
Keywords: checkin-needed
(In reply to Brian Hackett (:bhackett) from comment #12)
> Comment on attachment 699389 [details] [diff] [review]
> fix
> 
> [Approval Request Comment]
> Bug caused by (feature/regressing bug #): objshrink
> User impact if declined: potential security vulnerability
> Testing completed (on m-c, etc.): on m-c
> Risk to taking this patch (and alternatives if risky): none
> String or UUID changes made by this patch: none

Can we please get a esr patch ready here ? Thank you
Comment on attachment 699389 [details] [diff] [review]
fix

This should apply clean to esr 17.
Attachment #699389 - Flags: approval-mozilla-esr17?
Attachment #699389 - Flags: approval-mozilla-esr17? → approval-mozilla-esr17+
The first patch doesn't apply cleanly to esr17. Looks like some function name changes, but also I don't see TaggedProto defined. The second patch has some differences in the surrounding code, so I'll let you make sure that it's applied correctly.
(In reply to Ryan VanderMeulen from comment #16)
> The first patch doesn't apply cleanly to esr17. Looks like some function
> name changes, but also I don't see TaggedProto defined. The second patch has
> some differences in the surrounding code, so I'll let you make sure that
> it's applied correctly.

Brian, looks like the patch needs unbitrotting for ESR17.
Flags: needinfo?(bhackett1024)
Only the second patch has approval.  The first is just fixing some debugging code.  For the second, getNewType() has change a little, but using this code instead:

        if (self->isRegExp()) {
            AddTypeProperty(cx, type, "source", types::Type::StringType());
            AddTypeProperty(cx, type, "global", types::Type::BooleanType());
            AddTypeProperty(cx, type, "ignoreCase", types::Type::BooleanType());
            AddTypeProperty(cx, type, "multiline", types::Type::BooleanType());
            AddTypeProperty(cx, type, "sticky", types::Type::BooleanType());
            AddTypeProperty(cx, type, "lastIndex", types::Type::Int32Type());
        }

        if (self->isString())
            AddTypeProperty(cx, type, "length", Type::Int32Type());

Before the self->getClass()->ext.equality test should be fine.
Flags: needinfo?(bhackett1024)
Batch edit: Bugs marked status-b2g18: affected after 2/13 branching of v1.0.1 are now also status-b2g18-v1.0.1: affected
Can someone suggest a security rating here?
Whiteboard: [jsbugmon:update] → [jsbugmon:update][adv-main19+][adv-esr1703+]
Can you please uplift the patch for b2g18 & land it on mozilla-b2g18_v1_0_1as well (should be identical fixes) asap. thank you !
Keywords: sec-high
Group: core-security
You need to log in before you can comment on or make changes to this bug.