"Assertion failure: !comp->rt->isHeapBusy()" with about:memory and verifyprebarriers

RESOLVED FIXED in Firefox 48

Status

()

defect
--
critical
RESOLVED FIXED
7 years ago
3 years ago

People

(Reporter: jruderman, Assigned: jonco)

Tracking

({assertion, testcase})

Trunk
mozilla50
x86_64
macOS
Points:
---

Firefox Tracking Flags

(firefox47 wontfix, firefox48+ fixed, firefox49+ fixed, firefox-esr4548+ fixed, firefox50+ fixed)

Details

(Whiteboard: [adv-main48+][adv-esr45.3+])

Attachments

(4 attachments)

1. Create a new profile (mkdir -p ~/px/a; firefox -profile ~/px/a)
2. Install https://www.squarefree.com/extensions/domFuzzLite3.xpi
3. Load the testcase

Assertion failure: !comp->rt->isHeapBusy(), at js/src/vm/ObjectImpl-inl.h:395
Posted file testcase
Posted file stack
Possibly a regression from bug 810169? The stack is in the global read barrier, inside the JS memory reporter during xpc::XPCJSRuntimeStats::initExtraCompartmentStats.
Bill what do you think about comment 3?  (Also how bad is this?)
Flags: needinfo?(wmccloskey)
Can someone CC me on bug 810169?  Thanks.
(In reply to Nicholas Nethercote [:njn] from comment #5)
> Can someone CC me on bug 810169?  Thanks.

done
Assignee: general → wmccloskey
After the renaming in bug 751618, this appears as:

Assertion failure: !zone->rt->isHeapBusy(), at js/src/vm/ObjectImpl-inl.h:348
Sorry it's taken me so long to get to this. I'm pretty sure it's a bogus assertion. Do you hit this a lot, Jesse?
Flags: needinfo?(wmccloskey)
Not so much that it slows down fuzzing.  But it means I'm not testing the rest of what happens with this combination of features.
Bill, can we unhide this if the assertion is bogus?
Unassigning myself from old bugs. Don't worry, not quitting.
Assignee: wmccloskey → nobody
Jon, as you've touched some of the other heap/gc/barrier bugs recently, do you think you might be able to take a look here? Even if the assertion is bogus/no longer exists, it would be nice to have a resolution.
Flags: needinfo?(jcoppeard)
Currently our barriers do not fire if the heap is busy, a state that includes both collection and tracing.

The stack shows us accessing a read-barriered object from within an iteration callback, i.e. when we are in the tracing state.  It seems to me that we definitely do want the barrier to happen in this case.
Group: javascript-core-security
Flags: needinfo?(jcoppeard)
Patch to execute barriers if heap is in the tracing state.  I put this through try and it didn't seem to cause problems.
Attachment #8764192 - Flags: review?(terrence)
Assignee: nobody → jcoppeard
Comment on attachment 8764192 [details] [diff] [review]
bug822081-barrier-while-tracing

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

I guess the intention was that the liveness graph should not change if we trace during IGC marking. I agree that it's probably not safe though, so let's take this.
Attachment #8764192 - Flags: review?(terrence) → review+
Comment on attachment 8764192 [details] [diff] [review]
bug822081-barrier-while-tracing

[Security approval request comment]
How easily could an exploit be constructed based on the patch? Difficult, would require finding a caller of the changed code and then a way to exploit it.

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? Everything back to FF 41.

If not all supported branches, which bug introduced the flaw? Bug 1170717.

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

How likely is this patch to cause regressions; how much testing does it need? Unlikely.
Attachment #8764192 - Flags: sec-approval?
Comment on attachment 8764192 [details] [diff] [review]
bug822081-barrier-while-tracing

sec-approval+ for trunk.

We'll want branch patches, including ESR45, made and nominated as well so they can be checked in once this is on trunk.
Attachment #8764192 - Flags: sec-approval? → sec-approval+
https://hg.mozilla.org/mozilla-central/rev/a4cbf28c4d9e
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Comment on attachment 8764192 [details] [diff] [review]
bug822081-barrier-while-tracing

Approval Request Comment
[Feature/regressing bug #]: Bug 1170717.
[User impact if declined]: Possible UAF crash / security vulterability.
[Describe test coverage new/current, TreeHerder]: On m-c since 30th June.
[Risks and why]: Low.
[String/UUID change made/needed]: None.
Attachment #8764192 - Flags: approval-mozilla-beta?
Attachment #8764192 - Flags: approval-mozilla-aurora?
[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: Possible UAF crash / security vulterability.
Fix Landed on Version: 50.
Risk to taking this patch (and alternatives if risky): Low.
String or UUID changes made by this patch: None.

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.
Attachment #8767911 - Flags: review+
Attachment #8767911 - Flags: approval-mozilla-esr45?
Comment on attachment 8764192 [details] [diff] [review]
bug822081-barrier-while-tracing

Sec bug fix, Aurora49+, Beta48+
Attachment #8764192 - Flags: approval-mozilla-beta?
Attachment #8764192 - Flags: approval-mozilla-beta+
Attachment #8764192 - Flags: approval-mozilla-aurora?
Attachment #8764192 - Flags: approval-mozilla-aurora+
Comment on attachment 8767911 [details] [diff] [review]
bug822081-esr45

Sec issue, ESR45+
Attachment #8767911 - Flags: approval-mozilla-esr45? → approval-mozilla-esr45+
Group: javascript-core-security → core-security-release
Flags: qe-verify+
Whiteboard: [adv-main48+][adv-esr45.3+]
I could not reproduce this bug using Fx47.0, Fx 50.0a1 (from tinderbox link: http://goo.gl/EzpPiH) and Fx 44.0a1( also from tinderbox link: http://goo.gl/uGIOkY).

I managed to install  https://www.squarefree.com/extensions/domFuzzLite3.xpi on a clean profile. After that I ran the test case and searched in the terminal and in the console for "Assertion failure: !comp->rt->isHeapBusy()" or keywords but did not find anything.

Am I missing something?
Flags: qe-verify+ → needinfo?(jruderman)
The form of behavior that this bug elicited changed from a crash to over-aggressive filtering between when it was reported and when Jon investigated it. I don't think that you'd necessarily see anything wrong outside a debugger in 47.
Flags: needinfo?(jruderman)
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.