1299098 - Assertion failure: [infer failure] Missing type in object [0x0x7ffff0671b20] lastIndex: int, at js/src/vm/TypeInference.cpp:251 with RegExp

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

The following testcase crashes on mozilla-central revision 4f72b1d05267 (build with --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --enable-debug --enable-optimize, run with --fuzzing-safe --ion-offthread-compile=off --ion-eager):

function AddTestCase(description, expect, actual) {}
AddRegExpCases(new RegExp("A"), "new RegExp('A')", "\x41", "\\x41", 1, 0, ["A"]);
AddRegExpCases(new class valueOf extends RegExp {}("^x"), "new RegExp('^x')", "x412", "x412", 1, 0, ["x"]);
function AddRegExpCases(regexp, str_regexp, pattern, str_pattern, length, index, matches_array) {
    for (var matches = 0; matches < matches_array.length; matches++) {
        AddTestCase(regexp.exec(pattern)[matches]);
    }
}



Backtrace:

 received signal SIGSEGV, Segmentation fault.
0x0000000000435f55 in TypeFailure (cx=cx@entry=0x7ffff695f000, fmt=0x109f098 "Missing type in object %s %s: %s", fmt=0x109f098 "Missing type in object %s %s: %s") at js/src/vm/TypeInference.cpp:252
#0  0x0000000000435f55 in TypeFailure (cx=cx@entry=0x7ffff695f000, fmt=0x109f098 "Missing type in object %s %s: %s", fmt=0x109f098 "Missing type in object %s %s: %s") at js/src/vm/TypeInference.cpp:252
#1  0x0000000000bb3fe6 in js::ObjectGroupHasProperty (cx=cx@entry=0x7ffff695f000, group=0x7ffff0671b20, id=..., value=...) at js/src/vm/TypeInference.cpp:301
#2  0x0000000000ad71ec in GetExistingProperty<(js::AllowGC)1> (cx=0x7ffff695f000, receiver=..., obj=..., shape=..., vp=...) at js/src/vm/NativeObject.cpp:1757
#3  0x0000000000ad9dde in NativeGetPropertyInline<(js::AllowGC)1> (cx=0x7ffff695f000, obj=..., receiver=..., id=..., nameLookup=nameLookup@entry=NotNameLookup, vp=...) at js/src/vm/NativeObject.cpp:2014
#4  0x0000000000ada410 in js::NativeGetProperty (cx=<optimized out>, obj=..., receiver=..., id=..., vp=...) at js/src/vm/NativeObject.cpp:2048
#5  0x00000000006a8264 in js::GetProperty (cx=<optimized out>, obj=..., receiver=..., id=..., vp=...) at js/src/vm/NativeObject.h:1479
#6  0x0000000000adb305 in js::GetProperty (vp=..., name=<optimized out>, receiver=..., obj=..., cx=0x7ffff695f000) at js/src/jsobj.h:838
#7  js::GetProperty (cx=cx@entry=0x7ffff695f000, v=..., name=..., name@entry=..., vp=..., vp@entry=...) at js/src/vm/Interpreter.cpp:4238
#8  0x00000000007dc389 in js::jit::ComputeGetPropResult (cx=cx@entry=0x7ffff695f000, frame=<optimized out>, op=op@entry=JSOP_GETPROP, name=..., name@entry=..., val=..., val@entry=..., res=..., res@entry=...) at js/src/jit/SharedIC.cpp:2669
#9  0x00000000007f5bfc in js::jit::DoGetPropFallback (cx=0x7ffff695f000, payload=<optimized out>, stub_=<optimized out>, val=..., res=...) at js/src/jit/SharedIC.cpp:2749
#10 0x00007ffff7e40cd6 in ?? ()
[...]
#30 0x0000000000000000 in ?? ()
rax	0x0	0
rbx	0x7fffffffb480	140737488336000
rcx	0x7ffff6c28a2d	140737333332525
rdx	0x0	0
rsi	0x7ffff6ef7770	140737336276848
rdi	0x7ffff6ef6540	140737336272192
rbp	0x7fffffffbd60	140737488338272
rsp	0x7fffffffb460	140737488335968
r8	0x7ffff6ef7770	140737336276848
r9	0x7ffff7fe4740	140737354024768
r10	0x0	0
r11	0x0	0
r12	0x7ffff695f000	140737330409472
r13	0x7fffffffb880	140737488337024
r14	0x7fffffffc628	140737488340520
r15	0x7ffff695f000	140737330409472
rip	0x435f55 <TypeFailure(JSContext*, char const*, char const*, ...)+303>
=> 0x435f55 <TypeFailure(JSContext*, char const*, char const*, ...)+303>:	movl   $0x0,0x0
   0x435f60 <TypeFailure(JSContext*, char const*, char const*, ...)+314>:	ud2    


Infer failures used to be sec-high or critical, marking s-s.

Comment 1

[Fuzzing Team]

JSBugMon: Bisection requested, result:
=== Treeherder Build Bisection Results by autoBisect ===

The "good" changeset has the timestamp "20160106232707" and the hash "4f4ef7073bb53185edc3a208446c687b291abe61".
The "bad" changeset has the timestamp "20160106235308" and the hash "a0c3bdd0559b936e5b2865783411231181c84d3c".

Likely regression window: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=4f4ef7073bb53185edc3a208446c687b291abe61&tochange=a0c3bdd0559b936e5b2865783411231181c84d3c

Comment 2

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

Arai-san, bug 1207922 is in the regression window, is it a likely regressor?

Comment 3

[Tooru Fujisawa [:arai]]

Attachment: Add lastIndex type on RegExp object allocation.

Looks like we need to add TI information for lastIndex property when initializing RegExp object, or maybe when setting lastIndex to zero first time?

anyway, the assertion failure can be hit with the following code before Bug 1207922:
  function test(re) {
    return re.lastIndex;
  }
  test(new RegExp("A"));
  test(new class valueOf extends RegExp {}("^x"));

Bug 1207922 changes RegExp#exec to use self-hosted code, and it hits the same thing.


Here's regression range:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=cb2462db0b8653b4a42131a4d2a8ea92c534d4fb&tochange=ca6084eaafbfb041a9bc081228cdb8c7e879eb38

apparently from Bug 1055472.

Comment 4

[Tooru Fujisawa [:arai]]

Attachment: Add lastIndex type on RegExp object allocation.

forgot to add simplified testcase.

Comment 5

[Andrew McCreight [:mccr8]]

Marking affected flags. Please request sec-approval before landing.

Comment 6

[Hannes Verschore [:h4writer]]

Comment on attachment 8788823 [details] [diff] [review]
Add lastIndex type on RegExp object allocation.

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

Forwarding to Brian. I'm a bit puzzled why we don't have a type for "lastIndex". Shouldn't that get inherited from the proto?

Comment 7

[Brian Hackett [Laid off!]]

Comment on attachment 8788823 [details] [diff] [review]
Add lastIndex type on RegExp object allocation.

There is some logic in ObjectGroup::defaultNewGroup which is supposed to add the lastIndex property, as well as some other builtin properties for other kinds of objects, to the new group.  I'm guessing that this logic isn't working here because of this 'new class valueOf extends RegExp' thing?  If testing the class of the proto is not sufficient anymore then these tests should be fixed for all the kinds of objects being special cased here, probably by looking at the class of the new group rather than that of the proto.

Comment 8

[Tooru Fujisawa [:arai]]

Thanks :)

it sounds like I'm not capable of fixing this in short term.
unassigning for now.
anyone feel free to take this over.

Comment 9

[Tooru Fujisawa [:arai]]

might be better ni?-ing efaust.
can you take a look?

Comment 10

[Andrew McCreight [:mccr8]]

Jan, there's been no activity here for over a month. Do you know who else might be able to fix this? Thanks.

Comment 11

[Jan de Mooij [:jandem]]

Attachment: Patch

Fix type information implicitly defined properties by looking at the group's clasp instead of the proto's clasp.

Comment 12

[Jan de Mooij [:jandem]]

TI bugs are bad.

Comment 14

[Jan de Mooij [:jandem]]

Comment on attachment 8803320 [details] [diff] [review]
Patch

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

::: js/src/vm/ObjectGroup.cpp
@@ +576,5 @@
> +    if (clasp == &RegExpObject::class_) {
> +        AddTypePropertyId(cx, group, nullptr, NameToId(names.lastIndex), TypeSet::Int32Type());
> +    } else if (clasp == &StringObject::class_) {
> +        AddTypePropertyId(cx, group, nullptr, NameToId(names.length), TypeSet::Int32Type());
> +    } else if (ErrorObject::isErrorClass((clasp))) {

Ugh, removed the unnecessary parentheses around clasp locally.

Comment 15

[Jan de Mooij [:jandem]]

Comment on attachment 8803320 [details] [diff] [review]
Patch

[Security approval request comment]
> How easily could an exploit be constructed based on the patch?
Not easily, maybe with some effort.

> 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?
All (regression from bug 1055472 probably).

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

> How likely is this patch to cause regressions; how much testing does it need?
The patch is pretty straight-forward, unlikely to cause regressions.

Comment 16

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

This missed the final beta for Firefox 50. I'd rather not take this with no betas. We should delay until November 14, a week into the new cycle. At that point, we should get ESR45, Beta and Aurora patches nominated as well.

Comment 17

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

Comment on attachment 8803320 [details] [diff] [review]
Patch

Sec-approval+ for November 14, 2016 checkin.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

Should we consider taking this for 50.1?

Comment 19

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

We could but I'm not sure it is an emergency for 50.1

Comment 21

[Jan de Mooij [:jandem]]

evilpie, note how the patch here compares to your patch in bug 1213341.

Comment 22

[Jan de Mooij [:jandem]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9d270fe2298bc780b33b76093bc0e87a4bd94fae

Comment 23

[Jan de Mooij [:jandem]]

Comment on attachment 8803320 [details] [diff] [review]
Patch

Approval Request Comment
[Feature/Bug causing the regression]: ES6 classes, bug 1055472 probably.
[User impact if declined]: Security issues, crashes.
[Is this code covered by automated tests?]: Yes.
[Has the fix been verified in Nightly?]: Patch fixes the tests the fuzzers found.
[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?]: Low risk.
[Why is the change risky/not risky?]: It changes a single function that's used a lot, and the change is well understood. I won't say "No risk" but it's unlikely.
[String changes made/needed]: None.

Comment 24

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/9d270fe2298b

Comment 25

[Fuzzing Team]

JSBugMon: This bug has been automatically verified fixed.

Comment 26

[Gerry Chang [:gchang]]

Comment on attachment 8803320 [details] [diff] [review]
Patch

Fix a sec-high. Beta51+ and Aurora52+. Should be 51 beta 6.

Comment 27

[Jan de Mooij [:jandem]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/d27c9c1d6beb

Comment 28

[Jan de Mooij [:jandem]]

https://hg.mozilla.org/releases/mozilla-beta/rev/5e2593f1cbeb

Comment 29

[Fuzzing Team]

JSBugMon: This bug has been automatically verified fixed on Fx51
JSBugMon: This bug has been automatically verified fixed on Fx52

Comment 30

[Ryan VanderMeulen [:RyanVM]]

Given the low risk of this fix, I think it's worth nominating for 50.1.

Comment 31

[Jan de Mooij [:jandem]]

Comment on attachment 8803320 [details] [diff] [review]
Patch

Nominating for 50.1 as suggested, see comment 23.

Comment 32

[Marcia Knous [:marcia]]

Tracking 53+.

Comment 33

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8803320 [details] [diff] [review]
Patch

Sec-high, meets the triage bar for inclusion in 50.1.0, ESR45.6.0

Comment 34

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-release/rev/0e371cdea9f2

Comment 35

[Ryan VanderMeulen [:RyanVM]]

Needs rebasing for esr45.

Comment 36

[Fuzzing Team]

JSBugMon: This bug has been automatically verified fixed on Fx50

Comment 37

[Jan de Mooij [:jandem]]

https://hg.mozilla.org/releases/mozilla-esr45/rev/a46a9f16823c61bfc99b414ea63639df4aee842d