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)
Tracking
()
VERIFIED
FIXED
mozilla48
People
(Reporter: decoder, Assigned: Waldo)
Details
(4 keywords, Whiteboard: [jsbugmon:update][adv-main46+][adv-esr45.1+])
Attachments
(7 files, 1 obsolete file)
4.00 KB,
patch
|
arai
:
review+
|
Details | Diff | Splinter Review |
1.91 KB,
patch
|
efaust
:
review+
|
Details | Diff | Splinter Review |
14.43 KB,
patch
|
abillings
:
sec-approval+
|
Details | Diff | Splinter Review |
3.34 KB,
patch
|
Details | Diff | Splinter Review | |
14.40 KB,
patch
|
ritu
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
10.43 KB,
patch
|
ritu
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
10.35 KB,
patch
|
ritu
:
approval-mozilla-esr45+
|
Details | Diff | Splinter Review |
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.
Updated•8 years ago
|
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update]
Comment 1•8 years ago
|
||
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)
Assignee | ||
Comment 3•8 years ago
|
||
Hmm. I can reproduce this, but only with the particular specified compile options (and possibly only on the reported revision, for me). Tricksy. Investigating.
Assignee | ||
Comment 4•8 years ago
|
||
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)
Assignee | ||
Comment 5•8 years ago
|
||
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 | ||
Updated•8 years ago
|
Assignee: nobody → jwalden+bmo
Status: NEW → ASSIGNED
Assignee | ||
Comment 6•8 years ago
|
||
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 7•8 years ago
|
||
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+
Assignee | ||
Comment 8•8 years ago
|
||
Attachment #8727011 -
Flags: review?(arai.unmht)
Updated•8 years ago
|
Attachment #8727011 -
Flags: review?(arai.unmht) → review+
Assignee | ||
Comment 9•8 years ago
|
||
Attachment #8727038 -
Flags: review?(efaustbmo)
Assignee | ||
Comment 10•8 years ago
|
||
(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 11•8 years ago
|
||
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 12•8 years ago
|
||
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+
Assignee | ||
Comment 13•8 years ago
|
||
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.)
Assignee | ||
Updated•8 years ago
|
Attachment #8726655 -
Attachment is obsolete: true
Assignee | ||
Comment 14•8 years ago
|
||
Assignee | ||
Comment 15•8 years ago
|
||
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?
Comment 16•8 years ago
|
||
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.
status-firefox45:
--- → wontfix
status-firefox46:
--- → affected
status-firefox48:
--- → affected
status-firefox-esr38:
--- → unaffected
status-firefox-esr45:
--- → affected
tracking-firefox46:
--- → ?
tracking-firefox47:
--- → ?
tracking-firefox48:
--- → +
tracking-firefox-esr45:
--- → ?
Updated•8 years ago
|
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.
Assignee | ||
Comment 18•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/131a296d48a9
Assignee | ||
Comment 19•8 years ago
|
||
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?
Assignee | ||
Comment 20•8 years ago
|
||
Approval Request Comment: same as for the aurora patch.
Attachment #8733683 -
Flags: approval-mozilla-beta?
Assignee | ||
Comment 21•8 years ago
|
||
[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?
Comment 22•8 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/131a296d48a9
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Updated•8 years ago
|
Status: RESOLVED → VERIFIED
Comment 23•8 years ago
|
||
JSBugMon: This bug has been automatically verified fixed.
Updated•8 years ago
|
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+
Comment 27•8 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/b68ca8f1122c
Flags: in-testsuite?
Updated•8 years ago
|
Comment 28•8 years ago
|
||
JSBugMon: This bug has been automatically verified fixed on Fx47
Updated•8 years ago
|
Whiteboard: [jsbugmon:update] → [jsbugmon:update][adv-main46+][adv-esr45.1+]
Assignee | ||
Comment 31•8 years ago
|
||
Five months later, landed the possibly-incriminating comments and test: https://hg.mozilla.org/integration/mozilla-inbound/rev/65c59398a16037e0b9c8968551b10cd388f7afd9 https://hg.mozilla.org/integration/mozilla-inbound/rev/92f2732f1114decfdcd47d0d5e7c0b12f20415dc
Flags: in-testsuite? → in-testsuite+
Assignee | ||
Comment 32•8 years ago
|
||
And one final test: https://hg.mozilla.org/integration/mozilla-inbound/rev/cedbd5b95449d1566ca41bbac7e8016efa0ff0b6
Comment 33•8 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/65c59398a160 https://hg.mozilla.org/mozilla-central/rev/92f2732f1114 https://hg.mozilla.org/mozilla-central/rev/cedbd5b95449
Updated•8 years ago
|
Group: core-security-release
You need to log in
before you can comment on or make changes to this bug.
Description
•