Closed Bug 1299098 Opened 3 years ago Closed 3 years ago

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


(Core :: JavaScript Engine, defect, critical)

Not set



Tracking Status
firefox49 --- wontfix
firefox-esr45 50+ fixed
firefox50 + verified
firefox51 + verified
firefox52 + verified
firefox53 + verified


(Reporter: decoder, Assigned: jandem)


(Blocks 1 open bug)


(5 keywords, Whiteboard: [jsbugmon:update][adv-main50.1+][adv-esr45.6+])


(1 file, 2 obsolete files)

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++) {


 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.
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update]
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:
Arai-san, bug 1207922 is in the regression window, is it a likely regressor?
Blocks: 1207922
Flags: needinfo?(arai.unmht)
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:

apparently from Bug 1055472.
Assignee: nobody → arai.unmht
Flags: needinfo?(arai.unmht)
Attachment #8788821 - Flags: review?(hv1989)
forgot to add simplified testcase.
Attachment #8788821 - Attachment is obsolete: true
Attachment #8788821 - Flags: review?(hv1989)
Attachment #8788823 - Flags: review?(hv1989)
Blocks: 1055472
No longer blocks: 1207922
Marking affected flags. Please request sec-approval before landing.
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?
Attachment #8788823 - Flags: review?(hv1989) → review?(bhackett1024)
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.
Attachment #8788823 - Flags: review?(bhackett1024)
Thanks :)

it sounds like I'm not capable of fixing this in short term.
unassigning for now.
anyone feel free to take this over.
Assignee: arai.unmht → nobody
might be better ni?-ing efaust.
can you take a look?
Flags: needinfo?(efaustbmo)
Jan, there's been no activity here for over a month. Do you know who else might be able to fix this? Thanks.
Flags: needinfo?(efaustbmo) → needinfo?(jdemooij)
Attached patch PatchSplinter Review
Fix type information implicitly defined properties by looking at the group's clasp instead of the proto's clasp.
Assignee: nobody → jdemooij
Attachment #8788823 - Attachment is obsolete: true
Flags: needinfo?(jdemooij)
Attachment #8803320 - Flags: review?(bhackett1024)
TI bugs are bad.
Duplicate of this bug: 1308022
Comment on attachment 8803320 [details] [diff] [review]

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.
Attachment #8803320 - Flags: review?(bhackett1024) → review+
Comment on attachment 8803320 [details] [diff] [review]

[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?

> 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.
Attachment #8803320 - Flags: sec-approval?
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.
Whiteboard: [jsbugmon:update] → [jsbugmon:update][checkin on 11/14]
Comment on attachment 8803320 [details] [diff] [review]

Sec-approval+ for November 14, 2016 checkin.
Attachment #8803320 - Flags: sec-approval? → sec-approval+
Should we consider taking this for 50.1?
Flags: needinfo?(abillings)
Whiteboard: [jsbugmon:update][checkin on 11/14] → [jsbugmon:update][checkin on 11/21]
Whiteboard: [jsbugmon:update][checkin on 11/21] → [jsbugmon:update][checkin on 11/29]
We could but I'm not sure it is an emergency for 50.1
Flags: needinfo?(abillings)
Duplicate of this bug: 1316841
evilpie, note how the patch here compares to your patch in bug 1213341.
Whiteboard: [jsbugmon:update][checkin on 11/29] → [jsbugmon:update]
Comment on attachment 8803320 [details] [diff] [review]

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.
Attachment #8803320 - Flags: approval-mozilla-esr45?
Attachment #8803320 - Flags: approval-mozilla-beta?
Attachment #8803320 - Flags: approval-mozilla-aurora?
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
JSBugMon: This bug has been automatically verified fixed.
Comment on attachment 8803320 [details] [diff] [review]

Fix a sec-high. Beta51+ and Aurora52+. Should be 51 beta 6.
Attachment #8803320 - Flags: approval-mozilla-beta?
Attachment #8803320 - Flags: approval-mozilla-beta+
Attachment #8803320 - Flags: approval-mozilla-aurora?
Attachment #8803320 - Flags: approval-mozilla-aurora+
JSBugMon: This bug has been automatically verified fixed on Fx51
JSBugMon: This bug has been automatically verified fixed on Fx52
Given the low risk of this fix, I think it's worth nominating for 50.1.
Flags: needinfo?(jdemooij)
Comment on attachment 8803320 [details] [diff] [review]

Nominating for 50.1 as suggested, see comment 23.
Flags: needinfo?(jdemooij)
Attachment #8803320 - Flags: approval-mozilla-release?
Comment on attachment 8803320 [details] [diff] [review]

Sec-high, meets the triage bar for inclusion in 50.1.0, ESR45.6.0
Attachment #8803320 - Flags: approval-mozilla-release?
Attachment #8803320 - Flags: approval-mozilla-release+
Attachment #8803320 - Flags: approval-mozilla-esr45?
Attachment #8803320 - Flags: approval-mozilla-esr45+
Needs rebasing for esr45.
Flags: needinfo?(jdemooij)
JSBugMon: This bug has been automatically verified fixed on Fx50
Whiteboard: [jsbugmon:update] → [jsbugmon:update][adv-main50.1+]
Whiteboard: [jsbugmon:update][adv-main50.1+] → [jsbugmon:update][adv-main50.1+][adv-esr45.6+]
Group: javascript-core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.