Closed Bug 1253099 Opened 8 years ago Closed 8 years ago

Assertion failure: p, at js/src/vm/Shape.cpp:1568 with RegExp and GC

Categories

(Core :: JavaScript Engine, defect)

x86_64
Linux
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla48
Tracking Status
firefox45 --- wontfix
firefox46 + fixed
firefox47 + verified
firefox48 + verified
firefox-esr38 --- unaffected
firefox-esr45 46+ fixed

People

(Reporter: decoder, Assigned: Waldo)

Details

(4 keywords, Whiteboard: [jsbugmon:update][adv-main46+][adv-esr45.1+])

Attachments

(7 files, 1 obsolete file)

The following testcase crashes on mozilla-central revision e15383656900 (build with --enable-optimize --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --enable-debug, run with --fuzzing-safe --thread-count=2):

RegExp({ toString: fillHeap });
function fillHeap() {
  if (RegExp(toString))
    gc();
}



Backtrace:

Program received signal SIGSEGV, Segmentation fault.
0x0000000000b1d7c5 in js::EmptyShape::insertInitialShape (cx=cx@entry=0x7ffff6907800, shape=..., shape@entry=..., proto=proto@entry=...) at js/src/vm/Shape.cpp:1568
#0  0x0000000000b1d7c5 in js::EmptyShape::insertInitialShape (cx=cx@entry=0x7ffff6907800, shape=..., shape@entry=..., proto=proto@entry=...) at js/src/vm/Shape.cpp:1568
#1  0x0000000000af7534 in ensureInitialCustomShape<js::RegExpObject> (obj=..., cx=0x7ffff6907800) at js/src/vm/Shape-inl.h:130
#2  js::RegExpObject::init (this=<optimized out>, cx=cx@entry=0x7ffff6907800, source=..., flags=js::NoFlags) at js/src/vm/RegExpObject.cpp:289
#3  0x0000000000af7643 in js::RegExpObject::initFromAtom (cx=cx@entry=0x7ffff6907800, regexp=..., regexp@entry=..., source=..., source@entry=..., flags=<optimized out>) at js/src/vm/RegExpObject.cpp:182
#4  0x0000000000d2f545 in RegExpInitialize (cx=cx@entry=0x7ffff6907800, obj=..., obj@entry=..., patternValue=..., patternValue@entry=..., flagsValue=..., flagsValue@entry=..., staticsUse=staticsUse@entry=js::UseRegExpStatics) at js/src/builtin/RegExp.cpp:203
#5  0x0000000000d301aa in js::regexp_construct (cx=0x7ffff6907800, argc=<optimized out>, vp=<optimized out>) at js/src/builtin/RegExp.cpp:435
#6  0x0000000000ac0722 in js::CallJSNative (cx=0x7ffff6907800, native=0xd2f9a0 <js::regexp_construct(JSContext*, unsigned int, JS::Value*)>, args=...) at js/src/jscntxtinlines.h:235
[...]
#18 main (argc=<optimized out>, argv=<optimized out>, envp=<optimized out>) at js/src/shell/js.cpp:7244
rax	0x0	0
rbx	0x7ffff6907800	140737330051072
rcx	0x7ffff6ca588d	140737333844109
rdx	0x0	0
rsi	0x7ffff6f7a9d0	140737336814032
rdi	0x7ffff6f791c0	140737336807872
rbp	0x7fffffffc2f0	140737488339696
rsp	0x7fffffffc270	140737488339568
r8	0x7ffff7fdf7c0	140737354004416
r9	0x6372732f736a2f6c	7165916604736876396
r10	0x7fffffffc030	140737488338992
r11	0x7ffff6c27ee0	140737333329632
r12	0x7fffffffc340	140737488339776
r13	0x7ffff6907800	140737330051072
r14	0x7fffffffc360	140737488339808
r15	0x7ffff69491b0	140737330319792
rip	0xb1d7c5 <js::EmptyShape::insertInitialShape(js::ExclusiveContext*, JS::Handle<js::Shape*>, JS::Handle<JSObject*>)+405>
=> 0xb1d7c5 <js::EmptyShape::insertInitialShape(js::ExclusiveContext*, JS::Handle<js::Shape*>, JS::Handle<JSObject*>)+405>:	movl   $0x620,0x0
   0xb1d7d0 <js::EmptyShape::insertInitialShape(js::ExclusiveContext*, JS::Handle<js::Shape*>, JS::Handle<JSObject*>)+416>:	callq  0x4a6780 <abort()>


This test looks fishy to me. GC is being called while the RegExp object stringifies something, might be that something is collected there while it shouldn't be. 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 "20151022183103" and the hash "9a1e89df904b953006be78880dbe99ad262c5d6a".
The "bad" changeset has the timestamp "20151022190636" and the hash "6dc0a7e6f79178df535c0c55358cb0c51aa09c10".

Likely regression window: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=9a1e89df904b953006be78880dbe99ad262c5d6a&tochange=6dc0a7e6f79178df535c0c55358cb0c51aa09c10
Waldo, is bug 1215430 or bug 1217069 a likely regressor?
Flags: needinfo?(jwalden+bmo)
Hmm.  I can reproduce this, but only with the particular specified compile options (and possibly only on the reported revision, for me).  Tricksy.  Investigating.
It's bug 1215430, but the problem is kind of not so much with the changes made there as with the existing code.  Sort of.  (But don't quote me on that just yet, as I need to do more investigation to understand the consequences of this mistake for sec-* purposes.)

The problem basically appears to be that RegExp objects, which are created with a set of builtin properties from birth (now just the single lastIndex property, formerly also .multiline, .sticky, .global, and so on), are created in two steps: RegExpAlloc and RegExpInitialize.  The former creates the object *but* doesn't quite fully do the builtin-property initialization.  The latter does that initialization.  *If* the first RegExp in a compartment is created by the former method, then another RegExp is created and initialized by the two methods *before* the first RegExp can be initialized using RegExpInitialize, you hit issues.

I think the fix is going to be to add the shape-initialization to add the builtin properties to RegExpAlloc -- which also happens to be consistent with ES6's RegExpAlloc algorithm.  Patch tomorrow.
Flags: needinfo?(jwalden+bmo)
Attached patch Patch (no tests yet) (obsolete) — Splinter Review
Okay, this sort of spiraled.  A little.

Moving shape-assignment into RegExpAlloc isn't too bad, but at that point initFromAtom becomes nigh-infallible.  So then I decided to make it infallbile, but then I realized: it really *shouldn't* be infallible.  Its use in RegExpInitialize (a spec algorithm) is wrong, because RegExpInitialize can be called on RegExps that have been exposed to script.  And that could let someone overwrite a "non-writable" lastIndex property with a zero, when such writing should actually fail.

Given this overwriting is a Caja violation (and sec-highish for that reason), and I was halfway there with the patch already, and my halfway-work didn't really tee up .lastIndex setting for a proper fix, I decided it made sense to do everything in one patch.  We have to fix all this stuff everywhere, so it doesn't seem to make much difference that it's being done in a sec bug instead of a more public one.

Here's the patch, with some refactoring of algorithms, a bunch of function-renaming to make the spec-algorithm similarities/differences clear, and more assertions.

Tests, including for the .lastIndex setting/zeroing bug, coming later today, after sleep.  But for now it seems good to get this in front of people.
Attachment #8726655 - Flags: review?(arai.unmht)
Assignee: nobody → jwalden+bmo
Status: NEW → ASSIGNED
Comment on attachment 8726655 [details] [diff] [review]
Patch (no tests yet)

And an extra reviewer, seeing as we collaborated the regression into the tree in the first place.  :-)
Attachment #8726655 - Flags: review?(efaustbmo)
Comment on attachment 8726655 [details] [diff] [review]
Patch (no tests yet)

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

Looks good :)

::: js/src/vm/RegExpObject.h
@@ +358,5 @@
>      static const unsigned SOURCE_SLOT              = 1;
>      static const unsigned FLAGS_SLOT               = 2;
>  
> +    // This function requires LAST_INDEX_SLOT for an assertion.
> +    friend RegExpObject* RegExpAlloc(ExclusiveContext* cx, HandleObject proto);

the value of LAST_INDEX_SLOT can be obtained by public static lastIndexSlot() method.
maybe we could use it there and remove friend declaration here?
Attachment #8726655 - Flags: review?(arai.unmht) → review+
Attachment #8727011 - Flags: review?(arai.unmht) → review+
(In reply to Tooru Fujisawa [:arai] from comment #7)
> the value of LAST_INDEX_SLOT can be obtained by public static
> lastIndexSlot() method.
> maybe we could use it there and remove friend declaration here?

Good call.  Fixed locally.

Bug 1242810 and this bug touch the same areas of code, so they'll have to have somewhat-coordinated landing, once both have sec-approval.  So no posting an updated version of the patch, because I've based it atop the patch for that bug.
Comment on attachment 8726655 [details] [diff] [review]
Patch (no tests yet)

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

LGTM.

::: js/src/builtin/RegExp.cpp
@@ +161,5 @@
> + * whose "lastIndex" property might have been made non-writable: here, zeroing
> + * "lastIndex" can fail.
> + *
> + * We efficiently solve this problem by completely removing "lastIndex" zeroing
> + * from the provided function.  Callers *must* handle "lastIndex" zeroing

This *must* deserves its own line with a NB: or similar alerting syntax. Despite the asterisks, I fear that it will blend into the surrounding paragraph.
Attachment #8726655 - Flags: review?(efaustbmo) → review+
Comment on attachment 8727038 [details] [diff] [review]
Test for reentrant creation of the first RegExp in a global, during argument processing

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

This looks like a pretty easy thing to replicate. Should we hold off on landing this, or is the case more complicated?

::: js/src/tests/ecma_6/extensions/reentrant-RegExp-creation-and-gc-during-new-RegExp-pattern-ToString.js
@@ +11,5 @@
> +var summary =
> +  "Don't assert when, in |new RegExp(pat)|, stringifying |pat| creates " +
> +  "another RegExp and then performs a GC";
> +
> +print(BUGNUMBER + ": " + summary);

This boiler plate is optionally unnecessary.
Attachment #8727038 - Flags: review?(efaustbmo) → review+
I made the requested comment-change, but some of these comments have me a little leery, so I removed them to their own patch.  I'll post those comments in a separate patch, to land after the main one.  (Say, a week or so after releases with the bug fixed have happened, maybe.)
Attachment #8726655 - Attachment is obsolete: true
Comment on attachment 8729321 [details] [diff] [review]
Final patch, no tests, no too-incriminating comments

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

For the Caja-esque parts of this, wrt letting RegExp.prototype.compile modify a non-writable property -- pretty easy to invoke that.

For the assertion failure originally triggering the bug-filing: I'm not at all certain whether subsequent execution might be exploitable; I don't know exactly how our HashTable code works in the case in play there.  But it's definitely trivial to trigger that failing case.

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

Tests will land at their usual delay after this patch lands.  The possibly-incriminating comments, I've moved to a separate patch, that I'm going to land a week or so after releases with these problems fixed.  (efaust and) I think there's cause for concern about correctly using these APIs, so it's desirable not to let things go uncommented for too long -- but no need to make it easier to understand the problem solved, right off the bat.

* Which older supported branches are affected by this flaw?  If not all supported branches, which bug introduced the flaw?

44 onward, so ESR45 and aurora/beta.

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

Shouldn't be too hard, or riskier than this is.  I can always re-review if trunk's diverged from the original regressed state more than I think it has.

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

Relatively unlikely, but there's enough moving parts here that probably landing a few weeks out, rather than closer to release, would probably be best.
Attachment #8729321 - Flags: sec-approval?
sec-approval+ to land this on trunk. I think we'll want to backport this to Aurora, Beta, and ESR45. I'll let release management approve that.
Attachment #8729321 - Flags: sec-approval? → sec-approval+
Tracked for 46, 47, 48 and esr 45.1 as it's a sec-high issue. Please nominate patches for uplift when ready.
Attached patch Aurora patchSplinter Review
Approval Request Comment
[Feature/regressing bug #]: bug 1215430
[User impact if declined]: sec-high for the ability to mutate a non-writable .lastIndex, wrt Caja; unclear what the severity is for the original assertion -- don't know exactly what the contents of the misbegotten HashTable ptr (*not* pointer, it's a HashTable structure) are, when no match is found, or the consequences of writing into that bad ptr
[Describe test coverage new/current, TreeHerder]: trunk patch tested on tryserver, landed on m-i earlier today
[Risks and why]: this refactors/touches a moderate-ish bit of code in pursuit of fixing two issues, so a little riskier -- a little extra experience landing this now, rather than 1-2 weeks later, will be valuable
[String/UUID change made/needed]: N/A
Attachment #8733682 - Flags: approval-mozilla-aurora?
Attached patch Beta patchSplinter Review
Approval Request Comment: same as for the aurora patch.
Attachment #8733683 - Flags: approval-mozilla-beta?
Attached patch esr45 patchSplinter Review
[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: sec-high for the Caja overwrite-immutable-property aspect, unclear for the rest
User impact if declined: sec-high, maybe worse
Fix Landed on Version: trunk is 48, right?
Risk to taking this patch (and alternatives if risky): slightly risky as it touches a bit of code, but I think that's addressable by landing this earlier in a cycle
String or UUID changes made by this patch: N/A
Attachment #8733692 - Flags: approval-mozilla-esr45?
https://hg.mozilla.org/mozilla-central/rev/131a296d48a9
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Status: RESOLVED → VERIFIED
JSBugMon: This bug has been automatically verified fixed.
Group: javascript-core-security → core-security-release
Comment on attachment 8733682 [details] [diff] [review]
Aurora patch

Sec-high issue, Aurora47+
Attachment #8733682 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8733683 [details] [diff] [review]
Beta patch

Sec-high issue, Beta46+
Attachment #8733683 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment on attachment 8733692 [details] [diff] [review]
esr45 patch

Sec-high issue, ESR45+
Attachment #8733692 - Flags: approval-mozilla-esr45? → approval-mozilla-esr45+
JSBugMon: This bug has been automatically verified fixed on Fx47
Whiteboard: [jsbugmon:update] → [jsbugmon:update][adv-main46+][adv-esr45.1+]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.