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

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: 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)

Comment 1

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: another regression window

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?

Comment 2

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Sometimes js::types::TypeObject::print is on the stack.

Comment 3

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

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.

Comment 4

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: fix for lazy proto issue

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.

Comment 5

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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.

Comment 6

[Brian Hackett [Laid off!]]

Attachment: fix

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.

Comment 7

[Brian Hackett [Laid off!]]

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

Comment 8

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

My patch doesn't need sec-approval because it only affects DEBUG builds.

Comment 9

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

billm's patch:

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

bhackett's patch:

https://hg.mozilla.org/integration/mozilla-inbound/rev/7ada213f01bd

Comment 10

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/f77b007d48bf
https://hg.mozilla.org/mozilla-central/rev/7ada213f01bd

Comment 11

[Christian Holler (:decoder)]

JSBugMon: This bug has been automatically verified fixed.

Comment 12

[Brian Hackett [Laid off!]]

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

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/3d197fdb4b98
https://hg.mozilla.org/releases/mozilla-aurora/rev/3b1ce7ca16b2

https://hg.mozilla.org/releases/mozilla-beta/rev/e711dd161c8d
https://hg.mozilla.org/releases/mozilla-beta/rev/3c6da794f5c3

Comment 14

[bhavana bajaj [:bajaj]]

(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 15

[Brian Hackett [Laid off!]]

Comment on attachment 699389 [details] [diff] [review]
fix

This should apply clean to esr 17.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 17

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

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

Comment 18

[Brian Hackett [Laid off!]]

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.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr17/rev/7b5512da58d0

Can we check in a test for this?

Comment 20

[Lukas Blakk [:lsblakk] use ?needinfo]

Batch edit: Bugs marked status-b2g18: affected after 2/13 branching of v1.0.1 are now also status-b2g18-v1.0.1: affected

Comment 21

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

Can someone suggest a security rating here?

Comment 22

[bhavana bajaj [:bajaj]]

Can you please uplift the patch for b2g18 & land it on mozilla-b2g18_v1_0_1as well (should be identical fixes) asap. thank you !

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/e45eabeed16d
https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/a9e4f8912607