Closed
Bug 1146101
Opened 9 years ago
Closed 9 years ago
"Assertion failure: false (destroying Text style struct still present in style context tree)"
Categories
(Core :: CSS Parsing and Computation, defect)
Tracking
()
RESOLVED
FIXED
mozilla40
Tracking | Status | |
---|---|---|
firefox36 | --- | wontfix |
firefox37 | --- | wontfix |
firefox38 | --- | fixed |
firefox39 | --- | fixed |
firefox40 | --- | fixed |
firefox-esr31 | --- | unaffected |
firefox-esr38 | --- | fixed |
b2g-v1.4 | --- | unaffected |
b2g-v2.0 | --- | unaffected |
b2g-v2.0M | --- | unaffected |
b2g-v2.1 | --- | unaffected |
b2g-v2.1S | --- | unaffected |
b2g-v2.2 | --- | fixed |
b2g-master | --- | fixed |
People
(Reporter: jruderman, Assigned: heycam)
References
Details
(Keywords: assertion, sec-high, testcase, Whiteboard: [adv-main38+])
Attachments
(4 files, 1 obsolete file)
401 bytes,
text/html
|
Details | |
24.02 KB,
text/plain
|
Details | |
7.09 KB,
patch
|
Sylvestre
:
approval-mozilla-aurora+
Sylvestre
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
1.27 KB,
patch
|
Details | Diff | Splinter Review |
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
Reporter | ||
Comment 1•9 years ago
|
||
Reporter | ||
Comment 2•9 years ago
|
||
Is this dangling pointery? It sounds dangling pointery.
Assignee | ||
Comment 3•9 years ago
|
||
Yes that assertion indicates a dangling pointer.
Assignee: nobody → cam
Status: NEW → ASSIGNED
Assignee | ||
Comment 4•9 years ago
|
||
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
Assignee | ||
Comment 5•9 years ago
|
||
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.
Assignee | ||
Comment 6•9 years ago
|
||
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.
Assignee | ||
Comment 7•9 years ago
|
||
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.
Assignee | ||
Comment 8•9 years ago
|
||
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.
Assignee | ||
Comment 9•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=af628489de78
Assignee | ||
Comment 10•9 years ago
|
||
Attachment #8583587 -
Flags: review?(dbaron)
Updated•9 years ago
|
Attachment #8583587 -
Flags: review?(dbaron) → review+
Assignee | ||
Updated•9 years ago
|
status-firefox36:
--- → affected
status-firefox37:
--- → affected
status-firefox38:
--- → affected
status-firefox39:
--- → affected
Assignee | ||
Comment 11•9 years ago
|
||
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.
Assignee | ||
Comment 12•9 years ago
|
||
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?
Attachment #8583587 -
Flags: sec-approval?
Assignee | ||
Comment 13•9 years ago
|
||
(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•9 years ago
|
||
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.
Whiteboard: [checkin on 4/14]
Updated•9 years ago
|
Attachment #8583587 -
Flags: sec-approval? → sec-approval+
Updated•9 years ago
|
status-b2g-v1.4:
--- → affected
status-b2g-v2.0:
--- → affected
status-b2g-v2.0M:
--- → affected
status-b2g-v2.1:
--- → affected
status-b2g-v2.1S:
--- → affected
status-b2g-v2.2:
--- → affected
status-b2g-master:
--- → affected
status-firefox40:
--- → affected
status-firefox-esr31:
--- → affected
status-firefox-esr38:
--- → affected
Assignee | ||
Comment 15•9 years ago
|
||
Splitting up these patches so I can land the fix before the test.
Assignee | ||
Updated•9 years ago
|
Attachment #8592471 -
Attachment is patch: true
Assignee | ||
Comment 16•9 years ago
|
||
Assignee | ||
Updated•9 years ago
|
Attachment #8583587 -
Attachment is obsolete: true
Assignee | ||
Comment 17•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/a95d4de3ea08
Assignee | ||
Comment 18•9 years ago
|
||
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
Attachment #8592471 -
Flags: approval-mozilla-beta?
Attachment #8592471 -
Flags: approval-mozilla-aurora?
Assignee | ||
Comment 19•9 years ago
|
||
I'll land the test once the Firefox 40 release is out.
Whiteboard: [checkin on 4/14] → [checkin on 4/14][checkin test on 8/12]
Updated•9 years ago
|
Attachment #8592471 -
Flags: approval-mozilla-esr31?
Updated•9 years ago
|
Flags: in-testsuite?
Whiteboard: [checkin on 4/14][checkin test on 8/12] → [checkin test on 8/12]
Assignee | ||
Comment 20•9 years ago
|
||
31 was wrongly marked as affected.
Assignee | ||
Updated•9 years ago
|
Attachment #8592471 -
Flags: approval-mozilla-esr31?
Updated•9 years ago
|
Comment 21•9 years ago
|
||
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
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Updated•9 years ago
|
Attachment #8592471 -
Flags: approval-mozilla-beta?
Attachment #8592471 -
Flags: approval-mozilla-beta+
Attachment #8592471 -
Flags: approval-mozilla-aurora?
Attachment #8592471 -
Flags: approval-mozilla-aurora+
Comment 22•9 years ago
|
||
This is shipping in 38 so you probably don't need to wait until 40 is out to land the test.
Whiteboard: [checkin test on 8/12] → [checkin test on 8/12][adv-main38+]
Assignee | ||
Comment 23•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/8efc942a5dd9
Comment 24•8 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/8efc942a5dd9
Flags: in-testsuite? → in-testsuite+
Updated•8 years ago
|
Whiteboard: [checkin test on 8/12][adv-main38+] → [adv-main38+]
Updated•8 years ago
|
Group: core-security → core-security-release
Updated•7 years ago
|
Group: core-security-release
You need to log in
before you can comment on or make changes to this bug.
Description
•