Closed
Bug 1114058
Opened 9 years ago
Closed 9 years ago
Crash [@ js::RegExpShared::~RegExpShared]
Categories
(Core :: JavaScript Engine: JIT, defect)
Tracking
()
VERIFIED
FIXED
mozilla37
People
(Reporter: gkw, Assigned: terrence)
References
Details
(4 keywords, Whiteboard: [jsbugmon:update][adv-main36+])
Crash Data
Attachments
(4 files, 1 obsolete file)
8.90 KB,
text/plain
|
Details | |
6.78 KB,
patch
|
jonco
:
review+
Sylvestre
:
approval-mozilla-aurora+
abillings
:
sec-approval+
|
Details | Diff | Splinter Review |
1.38 KB,
patch
|
terrence
:
review+
RyanVM
:
checkin+
|
Details | Diff | Splinter Review |
1.47 KB,
patch
|
terrence
:
review+
RyanVM
:
checkin+
|
Details | Diff | Splinter Review |
// Randomly chosen test: js/src/jit-test/tests/auto-regress/bug812235.js gcslice(3) // Randomly chosen test: js/src/tests/test262/ch07/7.8/7.8.5/S7.8.5_A2.1_T2.js var hex = ["0", "1", "2", "3", "4", "5", "6", "7", "8", "9", "A", "B", "C", "D", "E", "F"]; for (var i1 = 0; i1 < 16; i1++) { for (var i2 = 0; i2 < 16; i2++) { for (var i3 = 0; i3 < 16; i3++) { for (var i4 = 0; i4 < 16; i4++) { try { var uu = hex[i1] + hex[i2] + hex[i3] + hex[i4]; eval("/" + String.fromCharCode("0x" + uu) + "/"); } catch (e) {} } } } } // Randomly chosen test: js/src/jit-test/tests/debug/bug-826669.js gczeal(9, 2) newGlobal(); crashes 32-bit js debug shell on m-c changeset 490f124d7dea with --fuzzing-safe --no-threads --ion-eager at js::RegExpShared::~RegExpShared. Debug configure options: LD=ld CROSS_COMPILE=1 CC="clang -Qunused-arguments -msse2 -mfpmath=sse -arch i386" RANLIB=ranlib CXX="clang++ -Qunused-arguments -msse2 -mfpmath=sse -arch i386" AS=$CC AR=ar STRIP="strip -x -S" HOST_CC="clang -Qunused-arguments -msse2 -mfpmath=sse" AUTOCONF=/usr/local/Cellar/autoconf213/2.13/bin/autoconf213 HOST_CXX="clang++ -Qunused-arguments -msse2 -mfpmath=sse" sh /Users/skywalker/trees/mozilla-central/js/src/configure --target=i386-apple-darwin9.2.0 --enable-macos-target=10.5 --enable-debug --enable-optimize --enable-nspr-build --enable-more-deterministic --with-ccache --enable-gczeal --enable-debug-symbols --disable-tests This was found by combining random js tests together with jsfunfuzz, the specific file(s) is/are: http://hg.mozilla.org/mozilla-central/file/490f124d7dea/js/src/jit-test/tests/auto-regress/bug812235.js http://hg.mozilla.org/mozilla-central/file/490f124d7dea/js/src/tests/test262/ch07/7.8/7.8.5/S7.8.5_A2.1_T2.js http://hg.mozilla.org/mozilla-central/file/490f124d7dea/js/src/jit-test/tests/debug/bug-826669.js autoBisect shows this is probably related to the following changeset: The first bad revision is: changeset: https://hg.mozilla.org/mozilla-central/rev/a57b5d713883 parent: 213775:4e0a21777c55 user: Terrence Cole date: Wed Oct 29 14:17:45 2014 -0700 summary: Bug 1074961 - Part 11: Use a ChunkPool to manage available Chunks list; r=sfink Setting s-s because this seems to involve gcslice and gczeal. Terrence, is bug 1074961 a likely regressor?
Flags: needinfo?(terrence)
Reporter | ||
Comment 1•9 years ago
|
||
(lldb) bt 5 * thread #1: tid = 0x46ba0, 0x00761a96 js-dbg-opt-32-dm-nsprBuild-darwin-490f124d7dea`js::RegExpShared::~RegExpShared() [inlined] JSString::isPermanentAtom() const at String.h:441, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x7e4b950) * frame #0: 0x00761a96 js-dbg-opt-32-dm-nsprBuild-darwin-490f124d7dea`js::RegExpShared::~RegExpShared() [inlined] JSString::isPermanentAtom() const at String.h:441 frame #1: 0x00761a96 js-dbg-opt-32-dm-nsprBuild-darwin-490f124d7dea`js::RegExpShared::~RegExpShared() [inlined] JSString::writeBarrierPre(JSString*) + 4 at String.h:504 frame #2: 0x00761a92 js-dbg-opt-32-dm-nsprBuild-darwin-490f124d7dea`js::RegExpShared::~RegExpShared() [inlined] js::InternalGCMethods<JSAtom*>::preBarrier(v=0x07e4b950) at Barrier.h:300 frame #3: 0x00761a92 js-dbg-opt-32-dm-nsprBuild-darwin-490f124d7dea`js::RegExpShared::~RegExpShared() [inlined] js::BarrieredBase<JSAtom*>::pre() + 2 at Barrier.h:452 frame #4: 0x00761a90 js-dbg-opt-32-dm-nsprBuild-darwin-490f124d7dea`js::RegExpShared::~RegExpShared() [inlined] js::BarrieredBase<JSAtom*>::~BarrieredBase() at Barrier.h:421 (lldb)
Assignee | ||
Comment 2•9 years ago
|
||
This also repros on normal linux32 debug build with --no-threads and --no-baseline. What's going on here is that we are cleaning up the RegExpShared's source atom one or more GC's before the RegExpShared itself gets cleaned up. As usual, the failure path is extremely complicated. The facts so far: * We keep a regexp shared if it is marked and if the source atom is about to die. From RegExpCompartment::sweep: bool keep = shared->marked() && !IsStringAboutToBeFinalizedFromAnyThread(shared->source.unsafeGet()); * When visiting the relevant table entry: shared->source->isMarked(BLACK) == false HOWEVER IsStringAboutToBeFinalized(shared->source) == false. * In IsAboutToBeFinalized, we only actually check the mark bits if zone->isGCSweeping(). * This was true at the re-introdution of IsAboutToBeFinalized in bug 790338 (Oct 2012) and differed from the existing IsMarked function, which continues to check zone->isCollecting(). Presumable, there was no breakage at that time. * The breaking edge is a cross zone edge from the regexp compartment to the atoms zone. * At the time of the IsStringAboutToBeFinalized call, the atoms zone state is MARK. * The call to IsStringAboutToBeFinalized that is miss-firing was introduced by bug 1010441 (May 2014), keeping jitcode for regexp around when keeping jitcode for a compartment. * The SCC contains an edge from the impacted zone to the atoms zone during the failing GC. However, the impacted zone-group does not contain the atoms zone. I think this is expected as we shouldn't have any edges out of the atoms zone. After reflection, I don't understand how the zone->isGCSweeping() test can be correct. I think the intent may have been along the lines of: SCC constructs zone groups which reflect all edges; if it is possible to visit an edge during sweeping, it should either be in the current zone group or be in an already-swept group; if the edge is in the current zone group, then the target zone state must have been set to sweeping; if the edge was already swept, then the zone state should still be SWEEP. However, this last is wrong as endMarkingZoneGroup resets the state to MARK for reasons that aren't really clear to me. If I swap the zone->isGCSweeping test out for zone->isGCScheduled, the crash, naturally, goes away. Jon, is this the right solution, or am I missing something critical to the way the SCC works?
Flags: needinfo?(terrence)
Assignee | ||
Comment 3•9 years ago
|
||
Attachment #8540390 -
Flags: review?(jcoppeard)
Assignee | ||
Comment 4•9 years ago
|
||
Comment on attachment 8540390 [details] [diff] [review] bug-1114058-v0.diff Oh, very interesting! With this patch applied, auto-regress/bug746376.js starts failing when some regex jit-code disappears under us.
Attachment #8540390 -
Flags: review?(jcoppeard)
Comment 5•9 years ago
|
||
(In reply to Terrence Cole [:terrence] from comment #2) > * The breaking edge is a cross zone edge from the regexp compartment to the > atoms zone. Ok this is interesting, I didn't think about the case of IsAboutToBeFinalized() being called on an object in a different zone group to the entity being swept. > After reflection, I don't understand how the zone->isGCSweeping() test can > be correct. The idea is to only sweep entities associated with the zone group currently being swept. E.g. if there is a table containing entries related to objects in several different groups, this would be swept once for each zone group but only the entries related to objects that were being swept in the current group would be removed each time. I think the solution here is probably to use |IsMarked(source)| rather than |!IsAboutToBeFinalized(source)|.
Assignee | ||
Comment 6•9 years ago
|
||
(In reply to Jon Coppeard (:jonco) (PTO until 29th Dec) from comment #5) > (In reply to Terrence Cole [:terrence] from comment #2) > The idea is to only sweep entities associated with the zone group currently > being swept. E.g. if there is a table containing entries related to objects > in several different groups, this would be swept once for each zone group > but only the entries related to objects that were being swept in the current > group would be removed each time. > > I think the solution here is probably to use |IsMarked(source)| rather than > |!IsAboutToBeFinalized(source)|. Yes, that would also work, but I thought it would be a too much of a hack. Maybe not, but there are still two things that I really do not understand. 1) In this case (and it seems in the case of any weak pointer), by looking at the target to choose whether to keep the pointer, we are effectively marking in the opposite direction of the pointer. The SCC does not currently take into account these reversed "edges". Should it? If not, why not? 2) Why does this bug not bite us constantly? Even if the SCC algorithm takes into account the forward weak edges, the SCC algorithm does not guarantee that these edges, if cross compartment, will land in the same ZoneGroup. Thus, it seems like calling IsAboutAboutToBeFinalized on any cross-compartment edge should assert so that we can switch them all to IsMarked?
Flags: needinfo?(jcoppeard)
Comment 7•9 years ago
|
||
(In reply to Terrence Cole [:terrence] from comment #6) > 1) In this case (and it seems in the case of any weak pointer), by looking > at the target to choose whether to keep the pointer, we are effectively > marking in the opposite direction of the pointer. The SCC does not currently > take into account these reversed "edges". Should it? If not, why not? The issue that grouping the zones into SCCs solves is to ensure that for all strong cross compartment edges A -> B we don't sweep A's zone after B's zone, which would allow the possibility of A being marked after B had been already been swept. So yes, thinking about weak edges it does seem like the SCC computation should reverse the edge, since we need to know if the target is marked before we know whether to sweep the source pointer. In this case if we use IsMarked() we could remove a RegExpShared unnecessarily if the source atom was not marked when we swept it, but became marked later. I don't know if this is actually a problem for RegExpShared in particular. (Also I'm not sure whether this one actually needs to be weak pointer at all or whether we could mark the source atom if the RegExpShared itself was marked.) In general we could reverse the SCC edges for weak pointers but this runs the risk of pulling everything into the same zone group and defeating incremental sweeping, especially if we have weak references into the atoms compartment as probably every compartment has strong edges into there. > 2) Why does this bug not bite us constantly? Even if the SCC algorithm takes > into account the forward weak edges, the SCC algorithm does not guarantee > that these edges, if cross compartment, will land in the same ZoneGroup. > Thus, it seems like calling IsAboutAboutToBeFinalized on any > cross-compartment edge should assert so that we can switch them all to > IsMarked? Well as in the example in my last comment, sometimes it's ok to call IsAboutToBeFinalized() on something in a zone we're not sweeping at the moment. So I'm not sure if we could assert this. I'm trying to think how many cross compartment weak pointers we actually have... For weakmaps with key delegates in a different zone we already add SCC edges to ensure that we finish marking the delegates' zones before the key zones, so that case is handled. Maybe these are the only cases?
Flags: needinfo?(jcoppeard)
Assignee | ||
Comment 8•9 years ago
|
||
(In reply to Jon Coppeard (:jonco) (PTO until 29th Dec) from comment #7) > I'm trying to think how many cross compartment weak pointers we actually > have... For weakmaps with key delegates in a different zone we already add > SCC edges to ensure that we finish marking the delegates' zones before the > key zones, so that case is handled. Maybe these are the only cases? Exactly! We're already jumping through hoops to make this case work for WeakMaps; could we introduce a new WeakPointer primitive that guarantees that weak edges get the same treatment? What we're doing with relying on manual IsAboutToBeFinalized and nulling during sweeping is only going to get more fragile as we incrementalize further. At the very least, it would be nice to have a linked-list in DEBUG so that we could add some solid verifications that the ad-hoc approach is working.
Assignee | ||
Comment 10•9 years ago
|
||
Let's switch this user to IsMarked and uplift to solve the proximate cause now and work on a more general fix elsewhere. I've verified that this fixes the crash and doesn't cause other issues.
Assignee: nobody → terrence
Attachment #8540390 -
Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8540982 -
Flags: review?(jcoppeard)
Updated•9 years ago
|
Attachment #8540982 -
Flags: review?(jcoppeard) → review+
Assignee | ||
Comment 11•9 years ago
|
||
Comment on attachment 8540982 [details] [diff] [review] bug-1114058-v1.diff [Security approval request comment] How easily could an exploit be constructed based on the patch? Not easily. Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? Nope. Which older supported branches are affected by this flaw? The potential for the bug to exist goes back to 2012, but I think the only actual instance is from May 2014. So I think we need to backport everywhere that sourced from m-c around that time. If not all supported branches, which bug introduced the flaw? Bug 1010441 Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? It's a one-liner and I don't think the code has changed much since May. How likely is this patch to cause regressions; how much testing does it need? It should be very low risk.
Attachment #8540982 -
Flags: sec-approval?
Updated•9 years ago
|
status-firefox34:
--- → wontfix
status-firefox35:
--- → affected
status-firefox36:
--- → affected
status-firefox-esr31:
--- → unaffected
Updated•9 years ago
|
status-b2g-v1.4:
--- → unaffected
status-b2g-v2.0:
--- → affected
status-b2g-v2.0M:
--- → affected
status-b2g-v2.1:
--- → affected
status-b2g-v2.2:
--- → affected
Comment 12•9 years ago
|
||
Comment on attachment 8540982 [details] [diff] [review] bug-1114058-v1.diff sec-approval+. We should get this checked into trunk and nominated for Beta and Aurora branches ASAP as it is running the risk of not getting into Beta (and therefore not into 35).
Attachment #8540982 -
Flags: sec-approval? → sec-approval+
Assignee | ||
Comment 13•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/f1c8fc215969
Comment 14•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/f1c8fc215969
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
Updated•9 years ago
|
Updated•9 years ago
|
Status: RESOLVED → VERIFIED
Comment 15•9 years ago
|
||
JSBugMon: This bug has been automatically verified fixed.
Comment 16•9 years ago
|
||
So this has almost-certainly missed Fx35 now (the RC build is due to be spun today). Can we at least get approval requests for Aurora?
Flags: needinfo?(terrence)
Updated•9 years ago
|
tracking-firefox36:
--- → +
Assignee | ||
Comment 17•9 years ago
|
||
Comment on attachment 8540982 [details] [diff] [review] bug-1114058-v1.diff Approval Request Comment [Feature/regressing bug #]: Bug 1010441. [User impact if declined]: Potential security risk. [Describe test coverage new/current, TBPL]: On m-i for a week. [Risks and why]: Low - this switches the caller to a different, but existing, primitive, so is unlikely to break other things or introduce new bugs. [String/UUID change made/needed]: None.
Flags: needinfo?(terrence)
Attachment #8540982 -
Flags: approval-mozilla-aurora?
Updated•9 years ago
|
Attachment #8540982 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 19•9 years ago
|
||
Terrence, can you please help rebase this for b2g34/b2g32?
Flags: needinfo?(terrence)
Assignee | ||
Comment 20•9 years ago
|
||
Attachment #8546830 -
Flags: review+
Attachment #8546830 -
Flags: checkin?(ryanvm)
Comment 21•9 years ago
|
||
Comment on attachment 8546830 [details] [diff] [review] backport-32.diff https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/e02e85b6a0c3 https://hg.mozilla.org/releases/mozilla-b2g32_v2_0m/rev/e02e85b6a0c3
Attachment #8546830 -
Flags: checkin?(ryanvm) → checkin+
Assignee | ||
Comment 22•9 years ago
|
||
Flags: needinfo?(terrence)
Attachment #8546841 -
Flags: review+
Attachment #8546841 -
Flags: checkin?(ryanvm)
Comment 23•9 years ago
|
||
Comment on attachment 8546841 [details] [diff] [review] backport-34.diff https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/273f24a1d1fe
Attachment #8546841 -
Flags: checkin?(ryanvm) → checkin+
Updated•9 years ago
|
Updated•9 years ago
|
Whiteboard: [jsbugmon:update] → [jsbugmon:update][adv-main36+]
Updated•9 years ago
|
Group: core-security → core-security-release
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
•