Assertion failure: IsIdle(oldState), at xpcom/glue/PLDHashTable.h:140

VERIFIED FIXED in Firefox 45

Status

()

defect
VERIFIED FIXED
3 years ago
3 years ago

People

(Reporter: dholbert, Assigned: smaug)

Tracking

({sec-critical})

unspecified
mozilla47
Points:
---

Firefox Tracking Flags

(firefox44 wontfix, firefox45+ verified, firefox46+ fixed, firefox47+ verified, firefox-esr38 unaffected)

Details

(Whiteboard: [adv-main45+])

Attachments

(5 attachments, 2 obsolete attachments)

STR:
 1. Load attached testcase (coming up) in a debug build.

(The testcase has an iframe that has an event handler which removes the iframe from its parent document.)

ACTUAL RESULTS:
Assertion failure: IsIdle(oldState), at /scratch/work/builds/mozilla-central/mozilla-central-11-11-01.13-56/mozilla/xpcom/glue/PLDHashTable.h:140


The backtrace (attached) shows this is happening in EventListener::HandleEvent calling into JS, unsurprisingly.

Marking this as security-sensitive because:
 (1) even though we're not crashing, this seems like we might be on the edge of using freed memory.
 (2) I hit this when trying to come up with a testcase for bug 1243555 (and this might be close to something usable as an attack-vector for that bug), so we probably shouldn't open this up until bug 1243555 is no longer a threat.
Attachment #8712949 - Attachment filename: frame2.html → frame1.html
<tangent>
With a slightly-different testcase (using a click handler instead of a transitionend handler), I instead triggered this nonfatal assertion (without any crash/abort):
###!!! ASSERTION: Null pres shell: 'mShell', file $OBJ/dist/include/nsPresContext.h, line 171

That's inside the "PresShell()" accessor (which, by the lack of a "Get" in its name, promises to return something non-null).  So, we're failing to live up to that promise, with my other testcase.
</tangent>
Attachment #8712950 - Attachment description: testcase 1 → testcase 1 (asserts fatally in debug builds)
Here's the testcase I'm talking about in comment 3 (which asserts non-fatally about "Null pres shell")
(Assignee)

Comment 5

3 years ago
Not really an events bug, but something else.
I wonder if this is a regression.
Component: DOM: Events → DOM: Core & HTML
(Assignee)

Comment 6

3 years ago
Ah, this is an issue in animation code.
DispatchAnimationEventsOnSubDocuments is scary. enumerates through the child documents and while doing that dispatches events.
Component: DOM: Core & HTML → DOM: Animation
(Assignee)

Comment 7

3 years ago
So a regression form bug 1180125 I think.
(Assignee)

Comment 8

3 years ago
And the fix is possible to collected first all the relevant documents to an array which keeps strong references to the documents, and then dispatch events.
Comment on attachment 8712952 [details]
testcase 2 (asserts non-fatally in debug builds)

(Thanks for the diagnosis, smaug!  I spun off testcase 2 into a new bug -- bug 1243628 -- since it doesn't involve DispatchAnimationEventsOnSubDocuments & hence seems to need a different fix.)
Attachment #8712952 - Attachment is obsolete: true
Flags: needinfo?(bbirtles)
(Assignee)

Comment 10

3 years ago
I guess I could take this. Looks rather obvious case.
Assignee: nobody → bugs
(Assignee)

Comment 11

3 years ago
Posted patch patchSplinter Review
Attachment #8713741 - Flags: review?(dholbert)
(Assignee)

Comment 12

3 years ago
I went through other cases where we enumerate subdocuments and couldn't see other scary cases.
But we should make that API less error prone.
Comment on attachment 8713741 [details] [diff] [review]
patch

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

r=me with 'i' nit addressed.

::: layout/base/nsRefreshDriver.cpp
@@ +1486,5 @@
>  
> +  nsCOMArray<nsIDocument> documents;
> +  CollectDocuments(mPresContext->Document(), &documents);
> +
> +  for (int32_t i = 0; i < documents.Length(); ++i) {

nsCOMArray::Length() returns an uint32_t value, so 'i' should have type uint32_t as well. (Otherwise I expect this will trigger a signed/unsigned comparison compiler warning.)

@@ +1506,5 @@
> +    // below animations in terms of compositing order.
> +    context->TransitionManager()->DispatchEvents();
> +    // Check that the presshell has not been destroyed
> +    if (context->GetPresShell()) {
> +      context->AnimationManager()->DispatchEvents();

I'm confused by the GetPresShell() check here, but it's orthogonal to this change (it exists in the old & the new code, and is partially-explained in bug 1211334).  I'll spin off a separate bug for my concerns/confusion about this chunk.
Attachment #8713741 - Flags: review?(dholbert) → review+
(Assignee)

Comment 14

3 years ago
oh right, Count() returns int32_t, Length() uint32_t. /me kicks the API.
(Assignee)

Comment 15

3 years ago
Posted patch uint32_t (obsolete) — Splinter Review
(Assignee)

Comment 16

3 years ago
Just use the saner old style nsCOMArray API.
Attachment #8713753 - Attachment is obsolete: true
That works too. Thanks.
(Assignee)

Comment 18

3 years ago
Comment on attachment 8713758 [details] [diff] [review]
int32_t everywhere

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
At least crash is relatively easily constructible.

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


Which older supported branches are affected by this flaw?
44, 45, 46

If not all supported branches, which bug introduced the flaw?
bug 1180125

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
The patch seems to apply cleanly to beta, and fixes the bug there, so the same patch should work everywhere.

How likely is this patch to cause regressions; how much testing does it need?
Should be safe. We just collect documents to an array before doing anything with those documents.

Bug 1244235 even adds a new testcase related to this code.
Attachment #8713758 - Flags: sec-approval?
Attachment #8713758 - Flags: approval-mozilla-beta?
Attachment #8713758 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 19

3 years ago
(In reply to Olli Pettay [:smaug] from comment #18)
> Do comments in the patch, the check-in comment, or tests included in the
> patch paint a bulls-eye on the security problem?

Forgot to fill this. 
The commit message could be
"Bug 1243583, ensure transition events are dispatched to all the relevant subdocuments, r=dholbert"
sec-approval+ for checkin on February 9. 

Can we get branch nominations to have it go onto branches after trunk?
Whiteboard: [checkin on 2/9]
Comment on attachment 8713758 [details] [diff] [review]
int32_t everywhere

Nevermind. I see the nominations now. Approving.
Attachment #8713758 - Flags: sec-approval?
Attachment #8713758 - Flags: sec-approval+
Attachment #8713758 - Flags: approval-mozilla-beta?
Attachment #8713758 - Flags: approval-mozilla-beta+
Attachment #8713758 - Flags: approval-mozilla-aurora?
Attachment #8713758 - Flags: approval-mozilla-aurora+
(Assignee)

Comment 22

3 years ago
We probably want to land Bug 1244250 at the same time, since it is dealing with same stuff as this
(dispatching transition/animation events).
(Canceling needinfo for birtles, since smaug ended up taking this [thanks!])
Flags: needinfo?(bbirtles)
Group: core-security → dom-core-security
https://hg.mozilla.org/mozilla-central/rev/2d13bd207e3d
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Group: dom-core-security → core-security-release
Whiteboard: [adv-main45+]
Reproduced the crash and the assertion failure using Nightly debug build 47.0a1 2016-01-27 under Win 7 64-bit.

Verified as fixed using latest Nightly 47.0a1 and Firefox 45.0 2016-03-03.
Status: RESOLVED → VERIFIED
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.