Bug 14550 (CVE-2011-0114)

Created attachment 15430 [details] Test case

Created attachment 31268 [details] Possible patch Possible patch attached.

Notes from irc conversation: dhyatt: setStyle is top down 5:22pm dhyatt: so you get to the outermost block with first-letter on it 5:22pm dhyatt: and update its style to be green 5:22pm dhyatt: so then the cached first letter style is also green 5:22pm dhyatt: or would be 5:22pm dhyatt: it just inherits 5:22pm dhyatt: but the issue is when you make a pseudo style 5:22pm dhyatt: you pass in a parent 5:23pm dhyatt: and you are passing in the first line style of the <span> 5:23pm dhyatt: and he has not yet been properly updated 5:23pm smfr: ah 5:23pm dhyatt: so his color is black 5:23pm dhyatt: so the first letter style inherits from that 5:23pm dhyatt: later on the <span> is going to update 5:23pm dhyatt: to green 5:23pm dhyatt: but because he;'s a RenderInline 5:23pm dhyatt: first letter isn't updated dhyatt suspects you may need some kind of updateFirstLetter that runs in both cases 5:24pm dhyatt: for both RenderInline and RenderBlock 5:24pm dhyatt: if you had that you could probably simplify a lot 5:24pm smfr: so this bug is more involved than I thought 5:24pm dhyatt: since you wouldn't have to drill if you eventually found the right object 5:24pm dhyatt: it's very involved yes dhyatt: smfr: here are the issues as i see them.... 5:26pm dhyatt: i.e., my laundry liust of first-letter problems 5:26pm dhyatt: list 5:26pm dhyatt: (1) we do a full-blown Detach when first-letter rules change. that sucks. 5:26pm dhyatt: but is not the end of the world 5:26pm dhyatt: in fact this basically means that first-letter being updated only needs to happen because of inheritance 5:26pm dhyatt: and not for any other reason 5:27pm dhyatt: and since first-letter objects have to be the child of either a RenderInline or RenderBlock 5:27pm dhyatt: i think you could just rewrite updateFirstLetter 5:27pm dhyatt: to be a common function to both 5:27pm dhyatt: like before/after stuff 5:27pm smfr: yep 5:27pm dhyatt: then finding it for updating would be really simple 5:28pm dhyatt: but really i do hate that we do a detach when first-letter styles change

See also: bug 15602.

I've uploaded a patch to https://bugs.webkit.org/show_bug.cgi?id=45620 that also fixes this.

Moving the discussion here from 45620. I think you could subclass styleDidChange for the RenderTextFragment and that gives you easy access to the first letter container to update its style. You can just get right to it.

Created attachment 69764 [details] Patch

Dan, Dave, Thank you for your reviews and comments. Can you take another look? Note: An alternative approach, "Update first letter after style has been recalculated for descendants", properly handles: <p id="target"><span>first letter stays black</span></p> ... document.getelementbyid('target').style.color = 'green'; but not: <p><span id="target">first letter stays black</span></p> ... document.getelementbyid('target').style.color = 'green'; (Stale cached pseudo style is used in this case.)

Ping?

Comment on attachment 69764 [details] Patch My one concern here is if updateFirstLetter() causes the RenderTextFragment to be destroyed (e.g., if the display type of first-letter changes from inline to block). I'd like to see a test for that so that we know we won't crash with this patch in place.

Created attachment 70398 [details] Patch

Created attachment 70399 [details] Added test for first letter removal/addition

Thank you for the review. Added a test where RenderTextFragment for the first-letter is removed or added by display property change ore before pseudo-element change. My understanding is that in such cases the changes are considered as "Detach" and the render tree is re-constructed. That is, RenderTextFragment::styleDidChange is not called where it would destroy the fragment instance if it were called. (In reply to comment #10) > (From update of attachment 69764 [details]) > My one concern here is if updateFirstLetter() causes the RenderTextFragment to be destroyed (e.g., if the display type of first-letter changes from inline to block). I'd like to see a test for that so that we know we won't crash with this patch in place.

Ping? Sorry to being naggy, but I'd like to fix this issue as soon as possible.

*** Bug 45620 has been marked as a duplicate of this bug. ***

Yuzo, can you please try reaching Dave on irc.

(In reply to comment #13) > Thank you for the review. > > Added a test where RenderTextFragment for the first-letter is removed or added by > display property change ore before pseudo-element change. > > My understanding is that in such cases the changes are considered as "Detach" and the > render tree is re-constructed. That is, RenderTextFragment::styleDidChange is not called > where it would destroy the fragment instance if it were called. > We only allow the block display type on floating first letters, so instead of switching between block and inline you need to switch between floating and not floating. You left in the updateFirstLetter() in RenderBlock::styleDidChange. That call is now wasteful/redundant in most cases, although it does admittedly catch the display change as a result of the float status change. I think you could still trip a display change from RenderTextFragment::styleDidChange though. If the first-letter style has float:inherit, and you put it inside a span inside a div, and then make the span float (and have the span have its own first-letter style rule also).

It might not actually crash if the display changes. That's what I want to see.

Created attachment 71022 [details] Patch

Thank you for the review. (In reply to comment #17) > (In reply to comment #13) > > Thank you for the review. > > > > Added a test where RenderTextFragment for the first-letter is removed or added by > > display property change ore before pseudo-element change. > > > > My understanding is that in such cases the changes are considered as "Detach" and the > > render tree is re-constructed. That is, RenderTextFragment::styleDidChange is not called > > where it would destroy the fragment instance if it were called. > > > > We only allow the block display type on floating first letters, so instead of switching between block and inline you need to switch between floating and not floating. > > You left in the updateFirstLetter() in RenderBlock::styleDidChange. That call is now wasteful/redundant in most cases, although it does admittedly catch the display change as a result of the float status change. > > I think you could still trip a display change from RenderTextFragment::styleDidChange though. If the first-letter style has float:inherit, and you put it inside a span inside a div, and then make the span float (and have the span have its own first-letter style rule also). I've added tests as suggested. (Please let me know if I misunderstood anything). As far as I understand, updateFirstLetter can destroy: - Anonymous RenderInline/RenderBlock generated for first letter - Original RenderText that is "split" into text fragments to achieve first letter rendering but not the text fragments created by the "split"

Created attachment 71024 [details] Added tests for float

Is this one almost ready to land?

Comment on attachment 71024 [details] Added tests for float r=me

Committed r70797: <http://trac.webkit.org/changeset/70797>

This was assigned CVE-2011-0114.