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

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jesse Ruderman]

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

Comment 1

[Jesse Ruderman]

Attachment: testcase

Comment 2

[Jesse Ruderman]

Attachment: stack

Comment 3

[Andrew McCreight [:mccr8]]

Possibly a regression from bug 810169? The stack is in the global read barrier, inside the JS memory reporter during xpc::XPCJSRuntimeStats::initExtraCompartmentStats.

Comment 4

[David Bolter [:davidb] (NeedInfo me for attention)]

Bill what do you think about comment 3?  (Also how bad is this?)

Comment 5

[Nicholas Nethercote [inactive]]

Can someone CC me on bug 810169?  Thanks.

Comment 6

[Andrew McCreight [:mccr8]]

(In reply to Nicholas Nethercote [:njn] from comment #5)
> Can someone CC me on bug 810169?  Thanks.

done

Comment 7

[Jesse Ruderman]

After the renaming in bug 751618, this appears as:

Assertion failure: !zone->rt->isHeapBusy(), at js/src/vm/ObjectImpl-inl.h:348

Comment 8

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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?

Comment 9

[Jesse Ruderman]

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.

Comment 10

[Andrew McCreight [:mccr8]]

Bill, can we unhide this if the assertion is bogus?

Comment 11

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Unassigning myself from old bugs. Don't worry, not quitting.

Comment 12

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

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.

Comment 13

[Jon Coppeard (:jonco)]

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.

Comment 14

[Jon Coppeard (:jonco)]

Attachment: bug822081-barrier-while-tracing

Patch to execute barriers if heap is in the tracing state.  I put this through try and it didn't seem to cause problems.

Comment 15

[Terrence Cole [:terrence]]

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.

Comment 16

[Jon Coppeard (:jonco)]

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.

Comment 17

[Al Billings [:abillings - ex-MoCo]]

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.

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a4cbf28c4d9e

Comment 19

[Jon Coppeard (:jonco)]

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.

Comment 20

[Jon Coppeard (:jonco)]

Attachment: bug822081-esr45

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

Comment 21

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8764192 [details] [diff] [review]
bug822081-barrier-while-tracing

Sec bug fix, Aurora49+, Beta48+

Comment 22

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8767911 [details] [diff] [review]
bug822081-esr45

Sec issue, ESR45+

Comment 23

[Jon Coppeard (:jonco)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/c6b972226566
https://hg.mozilla.org/releases/mozilla-beta/rev/1c9185a7df82

Comment 24

[Jon Coppeard (:jonco)]

https://hg.mozilla.org/releases/mozilla-esr45/rev/3a0deb9801ab

Comment 25

[Cristian Comorasu, QA [:ccomorasu], Releae Desktop QA - Please contact [e-mail]]

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?

Comment 26

[Terrence Cole [:terrence]]

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.