Xidorn Quan [:xidorn] UTC+11 Reporter
	Description • 10 years ago

Per bug 1075336 comment 11, there is some other bug other than the lifetime management problem of CounterStyle, which causes bug 1075336.

In normal case, when any change of styles affect any CounterStyle object, style structs should be reconstructed so that all style structs will use the latest information of counter styles. Hence, when that happens, no style struct should refer to removed CounterStyle objects anymore.

However, bug 1075336 uncovers that there is cases that after a CounterStyle object is remove by the CounterStyleManager, someone could still be using it.

	Mats Palmgren (inactive) Assignee
	Comment 1 • 10 years ago

With bug 1077718 fixed I think any use-after-free here would be protected by
the poisoning that the arena does.

Xidorn, do you think that the derived class that we didn't arena-allocate
(AnonymousCounterStyle), b/c it's ref-counted anyway, poses any risk for
use-after-free here?

	Xidorn Quan [:xidorn] UTC+11 Reporter
	Comment 2 • 10 years ago

If the ref-count mechanism is reliable, I don't think AnonymousCounterStyle poses use-after-free risk here. But as what David said in bug 1075336 comment 11, that bug indicates some other unknown bug.

	Mats Palmgren (inactive) Assignee
	Comment 3 • 10 years ago

OK, it seems to me sec-high is a bit over-pessimistic then, but I guess we should
assume the worst until we know exactly what the problem is.

This bug is rather vague to me; Xidorn, could you debug this and provide more
details about what the error condition is and how to reproduce that error
condition, with relevant details about the sequence of events that leads
us there and why the style system is at fault for that condition to occur?

	Xidorn Quan [:xidorn] UTC+11 Reporter
	Comment 4 • 10 years ago

The very simplified testcase of this bug.

The conditions to trigger this bug are:
1. <style>#1 with "@counter-style disc {...}" and "li { float: left/right; }"
2. <style>#1 is in <li>#1
2. another non-empty <li> exists
3. the <li>#1 is removed dynamically

setTimeout in the file is not necessary, but it seems that, without a delay, sometimes this bug won't be triggered.

	Xidorn Quan [:xidorn] UTC+11 Reporter
	Comment 5 • 10 years ago

Apply this patch to crash the application when the bug appears.

	Xidorn Quan [:xidorn] UTC+11 Reporter
	Comment 6 • 10 years ago

BTW, I think attachment 8511799 [details] could be the crashtest for bug 1075336.

	Mats Palmgren (inactive) Assignee
	Comment 7 • 10 years ago

Thanks, that makes it clear what the issue is.

In nsPresContext::FlushCounterStyles()
http://mxr.mozilla.org/mozilla-central/source/layout/base/nsPresContext.cpp?rev=968aa79b1200#2203
we first call 1. NotifyCounterStylesAreDirty() which is what removes the
style from mCacheTable in the CounterStyleManager (synchronously) and
then 2. PostRebuildAllStyleDataEvent which posts a request to rebuild
all style data at the next refresh driver tick (asynchronously) .

This is the stack with Xidorn's patch above, indicating the bad access.
We entered ProcessPendingRestyles with mRebuildAllStyleData == true
which indicates that 2 hasn't happened yet, so we're operating with
stale style data.

	Mats Palmgren (inactive) Assignee
	Comment 8 • 10 years ago

I don't think ProcessPendingRestyles should process individual restyles when
there's a pending request to rebuild all style data.  It should just rebuild
all style data directly.

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=ee12645c45f6

	Mats Palmgren (inactive) Assignee
	Comment 9 • 10 years ago

Interesting, the patch triggers the following assertion on Android 4.0 (but nowhere else?!):

ASSERTION: NS_BLOCK_HAS_FIRST_LETTER_STYLE state out of sync: 'haveFirstLetterStyle == ((mState & NS_BLOCK_HAS_FIRST_LETTER_STYLE) != 0)', file /builds/slave/try-and-d-00000000000000000000/build/layout/generic/nsBlockFrame.cpp, line 6540

layout/reftests/forms/button/first-letter-1.html  (4 assertions)
layout/reftests/forms/button/first-letter-1-ref.html (2 assertions)

I believe this is an existing bug that is uncovered by the patch.
Not really a layout bug though, just a false positive assertion.
New Try run with that fixed:

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=5518af68d629

	Mats Palmgren (inactive) Assignee
	Comment 10 • 10 years ago

(I've submitted a fix for the first-letter assertions separately in bug 399262.)

FWIW, the patch does cause a log warning on Android to occur more often
than before: "W/GeckoFaviconDecoder( 2100): Can't decode null data: URI."
I have no explanation for that currently; it's investigated/fixed in bug 1089940.

It also tickles the underlying bug behind the assertions in bug 1019192
slightly differently which is the reason for the assert count changes.

Green on Try:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=62388857169b
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=f74fe4424784

	Xidorn Quan [:xidorn] UTC+11 Reporter
	Comment 11 • 10 years ago

Comment on attachment 8512703 [details] [diff] [review]
fix

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

::: layout/base/RestyleManager.cpp
@@ +1533,5 @@
> +    RebuildAllStyleData(nsChangeHint(0), nsRestyleHint(0));
> +    MOZ_ASSERT(mPendingRestyles.Count() == 0);
> +    return;
> +  }
> +

It seems that there is effectively same code at the very end of this function, should we delete that code? If not, I guess we may need some comments about that?

	Mats Palmgren (inactive) Assignee
	Comment 12 • 10 years ago

BTW, the "W/GeckoFaviconDecoder" thing turned out to be an unrelated issue - my Try push
was based on a rev that was buggy.  I'll provide the details in bug 1089940.

	Mats Palmgren (inactive) Assignee
	Comment 13 • 10 years ago

(In reply to Xidorn Quan [:xidorn] (UTC+11) from comment #11)
> It seems that there is effectively same code at the very end of this
> function, should we delete that code? 

No, because 'mRebuildAllStyleData' could have been set true during the
course of this method.

> If not, I guess we may need some comments about that?

What do you think it should say?  The code itself already implies the
above (even more so after my patch) so adding a comment that just points
that out seems redundant.

	Mats Palmgren (inactive) Assignee
	Comment 14 • 10 years ago

Comment on attachment 8512703 [details] [diff] [review]
fix

>[Security approval request comment]
>How easily could an exploit be constructed based on the patch?

I don't think the patch reveals anything specific, other than "restyling".

>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?

Gecko 33 and later.

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

bug 966166.

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

Same patch should apply on all affected branches.

>How likely is this patch to cause regressions; how much testing does it need?

Hard to say; I'm guessing medium risk for regressions.  No special testing needed,
this path is well covered by our tests.

A note on the sec rating: I'm pretty sure it's not exploitable in v36 where we
have bug 1077718 because the arena poisoning will protect us.  We might want to
uplift that patch too as a belt-and-suspenders fix, because I have some doubts
that my patch here fixes the problem completely.  I'm pretty sure though, that
it would be a non-exploitable crash with that poisoning.

	Mats Palmgren (inactive) Assignee
	Comment 15 • 10 years ago

> because I have some doubts that my patch here fixes the problem completely

The reason for that is that, as described in comment 7, we're setting the
style data in an invalid state and then waiting for an asynchronous event
to correct that later.  My patch here restores the valid state in
ProcessPendingRestyles(), which probably is the next thing that happens
after setting the invalid state, but I'm not entirely sure something else
couldn't slip in and use this invalid data before that.  It seems unlikely,
but I can't prove it's impossible.

It seems to me we should take another look at the fragile design here and
see if we can avoid the invalid state altogether.  I think the patch here
is good in any case.

	Daniel Veditz [:dveditz]
	Comment 16 • 10 years ago

> A note on the sec rating: I'm pretty sure it's not exploitable in v36 where we
> have bug 1077718 because the arena poisoning will protect us.

Fair enough, but in terms of tracking severity for our advisory writing about bugs affecting Firefox 33 users the lack of both bugs equals sec-high. I'd rather leave this one sec-high than move it to the mitigation bug (which is public).

> We might want to uplift that patch too as a belt-and-suspenders fix

Looks like this was done earlier today -- thanks for pushing for it.

	Daniel Veditz [:dveditz]
	Comment 17 • 10 years ago

Comment on attachment 8512703 [details] [diff] [review]
fix

sec-approval+
a=dveditz for landing on aurora and beta

	Mats Palmgren (inactive) Assignee
	Comment 18 • 10 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/d2edb76fa2c0

@sheriff I think we should let this bake for a few days before landing on branches

	Carsten Book [:Tomcat]
	Comment 19 • 10 years ago

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

	Ryan VanderMeulen [:RyanVM]
	Comment 20 • 10 years ago

(In reply to Mats Palmgren (:mats) from comment #18)
> @sheriff I think we should let this bake for a few days before landing on
> branches

Long enough now?

	Mats Palmgren (inactive) Assignee
	Comment 21 • 10 years ago

Yeah, go ahead and land it please.  Thanks.

	Ryan VanderMeulen [:RyanVM]
	Comment 22 • 10 years ago

https://hg.mozilla.org/releases/mozilla-aurora/rev/cb4e1d969e8d
https://hg.mozilla.org/releases/mozilla-beta/rev/fdb8b52bea5c

	Ryan VanderMeulen [:RyanVM]
	Comment 23 • 10 years ago

Backed out from beta for bustage.
https://hg.mozilla.org/releases/mozilla-beta/rev/5b4bac2ebf6c

https://treeherder.mozilla.org/ui/logviewer.html#?job_id=126309&repo=mozilla-beta

	Mats Palmgren (inactive) Assignee
	Comment 24 • 10 years ago

Sorry about that, here's the correct patch for beta.
(trivial change, so no need for re-review)

	Ryan VanderMeulen [:RyanVM]
	Comment 25 • 10 years ago

https://hg.mozilla.org/releases/mozilla-beta/rev/4e78f69ca4a9

	Ryan VanderMeulen [:RyanVM]
	Comment 26 • 10 years ago

https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/4e78f69ca4a9