1146101 - "Assertion failure: false (destroying Text style struct still present in style context tree)"

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Jesse Ruderman]

Attachment: testcase

1. Set:
     user_pref("layout.css.expensive-style-struct-assertions.enabled", true);
2. Load the testcase

style struct 0x12f319ed8 found on style context 0x12f35ed98
  in a.html
Assertion failure: false (destroying Text style struct still present in style context tree), at ./nsStyleStructList.h:71

Comment 1

[Jesse Ruderman]

Attachment: stack

Comment 2

[Jesse Ruderman]

Is this dangling pointery? It sounds dangling pointery.

Comment 3

[Cameron McCormack (:heycam)]

Yes that assertion indicates a dangling pointer.

Comment 4

[Cameron McCormack (:heycam)]

Here's the restyle log.  We're leaving an old style context on a frame due to nsChangeHint_ReconstructFrame, so presumably that style context will go away after the reframe.  The fact that we have a provider frame might be preventing us from processing the old style context in mContextsToClear somehow.

Processing 2 pending restyles with 2 restyle roots for file:///tmp/assert.html
  processing style root TableOuter(table)(1)@7fda646701d8 at index 1
    aRestyleHint = eRestyle_StyleAttribute, aChangeHint = NS_STYLE_HINT_NONE
      RestyleSelf TableOuter(table)(1)@7fda646701d8, aRestyleHint = eRestyle_StyleAttribute
        resolving child provider frame
          RestyleSelf Table(table)(1)@7fda64670408, aRestyleHint = eRestyle_StyleAttribute
            parentContext = 7fda6466e160
            resolving style with replacement
            oldContext = 7fda6469c230, newContext = 7fda64669310
            CaptureChange, ourChange = NS_STYLE_HINT_NONE, aChangeToAssume = NS_STYLE_HINT_NONE
              mHintsNotHandledForDescendants = NS_STYLE_HINT_NONE
            swapping style structs between 7fda6469c230 and 7fda64669310
            setting new style context
            returning eRestyleResult_Continue
          RestyleContentChildren
            RestyleSelf TableRowGroup(tbody)(0)@7fda64670828, aRestyleHint = eRestyle_StyleAttribute
              parentContext = 7fda64669310
              resolving style with replacement
              oldContext = 7fda6469c710, newContext = 7fda646699d0
              CaptureChange, ourChange = nsChangeHint_ReconstructFrame, aChangeToAssume = NS_STYLE_HINT_NONE
                appending change nsChangeHint_ReconstructFrame
                mHintsNotHandledForDescendants = NS_STYLE_HINT_NONE
              continuing restyle since there is different style data: Display
              not setting new style context, since we'll reframe
              returning eRestyleResult_Continue
        continuing restyle since we had a provider frame
        parentContext = 7fda64669310
        resolving style with replacement
        oldContext = 7fda6466e478, newContext = 7fda646699d0
        CaptureChange, ourChange = NS_STYLE_HINT_NONE, aChangeToAssume = NS_STYLE_HINT_NONE
          mHintsNotHandledForDescendants = NS_STYLE_HINT_NONE
        swapping style structs between 7fda6466e478 and 7fda646699d0
        setting new style context
        returning eRestyleResult_Continue
      RestyleContentChildren
style struct 0x7fda6466ed08 found on style context 0x7fda6469c710
  in file:///tmp/assert.html

Comment 5

[Cameron McCormack (:heycam)]

Another copy of the restyle log, this time from an rr recording so I've got stable addresses.

Processing 2 pending restyles with 2 restyle roots for file:///tmp/assert.html
  processing style root TableOuter(table)(1)@2aaad9e7a1d8 at index 1
    aRestyleHint = eRestyle_StyleAttribute, aChangeHint = NS_STYLE_HINT_NONE
    style context tree before restyle:
      2aaad9e74478(1) Text=2aaad9e74d08(dependent) :-moz-table-outer USES_GRANDANCESTOR_STYLE parent=2aaad8597230 
      RestyleSelf TableOuter(table)(1)@2aaad9e7a1d8, aRestyleHint = eRestyle_StyleAttribute
        resolving child provider frame
          RestyleSelf Table(table)(1)@2aaad9e7a408, aRestyleHint = eRestyle_StyleAttribute
            parentContext = 2aaad9e74160
            resolving style with replacement
            oldContext = 2aaad8597230, newContext = 2aaad9e71310
            CaptureChange, ourChange = NS_STYLE_HINT_NONE, aChangeToAssume = NS_STYLE_HINT_NONE
              mHintsNotHandledForDescendants = NS_STYLE_HINT_NONE
            swapping style structs between 2aaad8597230 and 2aaad9e71310
              old style context now has: Text=2aaad8597578(owned)
              new style context now has: Text=2aaad9e74d08(owned)
            setting new style context
            returning eRestyleResult_Continue
          RestyleContentChildren
            RestyleSelf TableRowGroup(tbody)(0)@2aaad9e7a828, aRestyleHint = eRestyle_StyleAttribute
              parentContext = 2aaad9e71310
              resolving style with replacement
              oldContext = 2aaad8597710, newContext = 2aaad9e719d0
              CaptureChange, ourChange = nsChangeHint_ReconstructFrame, aChangeToAssume = NS_STYLE_HINT_NONE
                appending change nsChangeHint_ReconstructFrame
                mHintsNotHandledForDescendants = NS_STYLE_HINT_NONE
              continuing restyle since there is different style data: Display
              not setting new style context, since we'll reframe
              returning eRestyleResult_Continue
        continuing restyle since we had a provider frame
        parentContext = 2aaad9e71310
        resolving style with replacement
        oldContext = 2aaad9e74478, newContext = 2aaad9e719d0
        CaptureChange, ourChange = NS_STYLE_HINT_NONE, aChangeToAssume = NS_STYLE_HINT_NONE
          mHintsNotHandledForDescendants = NS_STYLE_HINT_NONE
        swapping style structs between 2aaad9e74478 and 2aaad9e719d0
          old style context now has: Text=0(dependent)
          new style context now has: Text=2aaad9e74d08(dependent)
        setting new style context
        returning eRestyleResult_Continue
      RestyleContentChildren
style struct 0x2aaad9e74d08 found on style context 0x2aaad8597710
  in file:///tmp/assert.html
Assertion failure: false (destroying Text style struct still present in style context tree), at ./nsStyleStructList.h:71


When we leave the old style context on the tbody frame due to the nsChangeHint_ReconstructFrame, we return eRestyleResult_Continue.  ElementRestyler::Restyle doesn't restyle the children.  Because we did no struct swaps on this style context, the |if (!swappedStructs)| check in ElementRestyler::Restyle succeeds, which means that we don't get down to the mStructsToClear.AppendElement() call.

This is the frame tree before doing the frame reconstruction:

Block(body)(2)@2aaad9e74640 {480,480,0,0} [state=000b120008100200] [content=2aaad85502e0] [sc=2aaad9e74160]<
  line 2aaad9e7a518: count=1 state=block,clean,prevmarginclean,not impacted,not wrapped,before:nobr,after:nobr[0x148] {0,0,0,0} {0,0,0,0;cw=0} <
    TableOuter(table)(1)@2aaad9e7a1d8 next=2aaad9e7a958 {0,0,0,0} [state=0002020000000200] [content=2aaad9e1e5b0] [sc=2aaad9e719d0:-moz-table-outer^2aaad9e71310^2aaad9e74160^2aaad9e72a88]<
      Table(table)(1)@2aaad9e7a408 {0,0,0,0} [state=0001100000000000] [content=2aaad9e1e5b0] [sc=2aaad9e71310^2aaad9e74160^2aaad9e72a88^0]<
        TableRowGroup(tbody)(0)@2aaad9e7a828 {120,120,0,0} [state=0000120000000000] [content=2aaad7d365c0] [sc=2aaad8597710^2aaad8597230^2aaad9e74160^2aaad9e72a88]<>
      >
    >
  >
  line 2aaad9e7a9c8: count=1 state=inline,clean,prevmarginclean,not impacted,not wrapped,before:nobr,after:nobr[0x300] {0,0,0,0} {0,0,0,0;cw=0} <
    Text(2)"\n\n\n\n"@2aaad9e7a958 {0,0,0,0} [state=4001000028600000] [content=2aaad9e66ce0] [sc=2aaad9e74de8:-moz-non-element] [run=2aaad9e8a700][0,4,T] 
  >
(null)>

and this is the style context tree:

2aaad9e74160(4) Text=2aaad9e745f0(dependent) parent=2aaad9e72a88 
  2aaad9e71310(2) Text=2aaad9e74d08(owned) 
    2aaad9e719d0(1) Text=2aaad9e74d08(dependent) :-moz-table-outer USES_GRANDANCESTOR_STYLE 
  2aaad8597230(2) Text=2aaad8597578(owned) 
    2aaad8597710(1) Text=2aaad9e74d08(dependent) 
  2aaad9e74de8(1) Text=2aaad9e745f0(dependent) :-moz-non-element 

2aaad8597230 is the table frame's old style context, and 2aaad8597710 is the tbody's about-to-be-reconstructed frame's style context.

So you can see at this point that the Text struct 2aaad9e74d08 is incorrectly cached since its owner is 2aaad8597710's uncle/aunt, rather than its parent.  This is to be expected as we swapped structs between 2aaad8597710's old and new parent, and didn't call ClearCachedStructsOnDescendants on 2aaad8597710.

For whatever reason, we end up destroying 2aaad9e71310 before we reframe the tbody.  I guess this ordering of frame reconstruction is why we trigger the assertion in a case with table frames but not with frames with "normal" style context inheritance.

Now, even had we appended 2aaad8597710 to mContextsToClear, this wouldn't have helped avoid the assertion, since ClearCachedInheritedStyleDataOnDescendants is called after ProcessRestyledFrames.

I think what we need to do is hold on to 2aaad9e71310 until after the frame reconstructions have been done.  This is exactly what mSwappedStructOwners is for -- the comment above the mSwappedStructOwners.AppendElement() call pretty much describes out situation -- but we only use that for cases where we leave an old style context on a frame due to eRestyleResult_Stop, and not for cases where we got a nsChangeHint_ReconstructFrame.

We only have to do this if the style context being left on the frame due to nsChangeHint_ReconstructFrame had structs swapped between its old and new parent, but we don't have that information in ElementRestyler::RestyleSelf.  I'm not sure whether it's worth recording the struct swaps on the parent ElementRestyler object so that it can get passed down, or whether we should just unconditionally append the about-to-be-reconstructed frame's putative new style context's parent to mSwappedStructOwners.

[Several minutes later...]

Having tried that, I only delayed the assertion triggering.  Now it triggers when the style is destroyed due to being released from the mSwappedStructOwners going out of scope.  The remaining old style context with the invalidly-inheriting cached struct is sticking around due to being in the ReframingStyleContexts table.

Comment 6

[Cameron McCormack (:heycam)]

Calling ClearCachedInheritedStyleDataOnDescendants before swappedStructOwners goes out of scope works.

The current ordering of those two things was a small optimisation; by dropping the style context references in swappedStructOwners, more of the contextsToClear entries will be left with a single reference and thus can have their ClearCachedInheritedStyleDataOnDescendants call skipped.  But I guess we can live without it.

Comment 7

[Cameron McCormack (:heycam)]

Alternatively we could remove entries from ReframingStyleContexts earlier.

Would it be too much overhead, or be incorrect, to have a ReframingStyleContexts object per restyle that is processed?  We could then explicitly clear it right after the ProcessRestyledFrames call.

But comment 6 is simpler, so I think we should just do that.

Comment 8

[Cameron McCormack (:heycam)]

As for whether this is exploitable, I'm not sure it is.  At least with this test case, the point at which the style context is destroyed is right at the end of nsCSSFrameConstructor::ContentRangeInserted, where the last strong reference was being held in the FrameConstructionItemList.  We don't end up accessing anything on the destroyed style context.

Comment 9

[Cameron McCormack (:heycam)]

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

Comment 10

[Cameron McCormack (:heycam)]

Attachment: patch

Comment 11

[Cameron McCormack (:heycam)]

I think we could end up reading from the deleted style struct if we had a sibling of the tbody, which starts off with different styles from the the original tbody, we then make style changes as in the test case but also make style changes to the second tbody so that it would want to share the tbody's old style.  Once we have finished restyling, we process the change hints.  We reframe the first tbody, and end up destroying the struct.  We then go to reframe the second tbody using the first tbody's old style context, which now has the bad pointer in it.  There are various places in the frame construction code that could read from this style.

Comment 12

[Cameron McCormack (:heycam)]

Comment on attachment 8583587 [details] [diff] [review]
patch

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

No; the style struct optimisation code is complex enough that it is not obvious.  The assertion doesn't fire on an exploitable access, but it warns that there is a pointer outliving the data being pointed to.

Which older supported branches are affected by this flaw?

all

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

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

From inspection I think the patch will apply.

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

Comment 13

[Cameron McCormack (:heycam)]

(In reply to Cameron McCormack (:heycam) from comment #12)
> How likely is this patch to cause regressions; how much testing does it need?

I think a try push for each branch with the patch applied and the expensive style struct assertions forced on (in the nsStyleContext destructor, for older branches where we don't have the layout.css.expensive-style-struct-assertions.enabled pref) will be sufficient.

Comment 14

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

We're about to ship 37, with RC candidates built, next week. This can go into the next cycle two weeks in, on April 14.

Comment 15

[Cameron McCormack (:heycam)]

Attachment: Call ClearCachedInheritedStyleDataOnDescendants on more style contexts that had structs swapped out from them. r=dbaron a=abillings

Splitting up these patches so I can land the fix before the test.

Comment 16

[Cameron McCormack (:heycam)]

Attachment: Test. r=dbaron a=abillings

Comment 17

[Cameron McCormack (:heycam)]

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

Comment 18

[Cameron McCormack (:heycam)]

Comment on attachment 8592471 [details] [diff] [review]
Call ClearCachedInheritedStyleDataOnDescendants on more style contexts that had structs swapped out from them. r=dbaron a=abillings

Approval Request Comment
[Feature/regressing bug #]: bug 931668
[User impact if declined]: out of bounds memory reads
[Describe test coverage new/current, TreeHerder]: landed on inbound, tested locally
[Risks and why]: low, we are just keeping some objects alive a little longer; confident with the local testing I've done plus the tests run once landed
[String/UUID change made/needed]: N/A

Comment 19

[Cameron McCormack (:heycam)]

I'll land the test once the Firefox 40 release is out.

Comment 20

[Cameron McCormack (:heycam)]

31 was wrongly marked as affected.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a95d4de3ea08
https://hg.mozilla.org/releases/mozilla-aurora/rev/a7ba1ecfd134
https://hg.mozilla.org/releases/mozilla-beta/rev/baa8222aaafd
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/ba2e4d7c8e10

Comment 22

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

This is shipping in 38 so you probably don't need to wait until 40 is out to land the test.

Comment 23

[Cameron McCormack (:heycam)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8efc942a5dd9

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8efc942a5dd9