Bug 234018 - nullptr deref in ComputeFloatOffsetForLineLayoutAdapter<FloatingObject::FloatLeft>::updateOffsetIfNeeded
Summary: nullptr deref in ComputeFloatOffsetForLineLayoutAdapter<FloatingObject::Float...
Status: REOPENED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Gabriel Nava Marino
URL:
Keywords: InRadar
: 233554 234326 (view as bug list)
Depends on:
Blocks:
 
Reported: 2021-12-08 10:25 PST by Gabriel Nava Marino
Modified: 2022-01-27 14:56 PST (History)
12 users (show)

See Also:


Attachments
Patch (7.05 KB, patch)
2021-12-08 11:25 PST, Gabriel Nava Marino
no flags Details | Formatted Diff | Diff
Patch (2.62 KB, patch)
2021-12-10 16:22 PST, Gabriel Nava Marino
no flags Details | Formatted Diff | Diff
Patch (2.67 KB, patch)
2021-12-10 20:07 PST, Gabriel Nava Marino
no flags Details | Formatted Diff | Diff
Patch (2.87 KB, patch)
2021-12-15 10:28 PST, Gabriel Nava Marino
no flags Details | Formatted Diff | Diff
Patch (6.72 KB, patch)
2022-01-13 15:09 PST, Gabriel Nava Marino
no flags Details | Formatted Diff | Diff
Patch (6.03 KB, patch)
2022-01-13 15:11 PST, Gabriel Nava Marino
no flags Details | Formatted Diff | Diff
Patch (6.10 KB, patch)
2022-01-27 10:27 PST, Gabriel Nava Marino
no flags Details | Formatted Diff | Diff
Patch (6.04 KB, patch)
2022-01-27 10:42 PST, Gabriel Nava Marino
ews-feeder: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Gabriel Nava Marino 2021-12-08 10:25:36 PST
We are accessing a renderer for a floatingObject that has already been destroyed because the floatingObject was never cleaned up from the current block.
Comment 1 Gabriel Nava Marino 2021-12-08 10:47:11 PST
RenderTree structure from attached test case with proposed patch. Notice the two floating objects:
Content-Type: text/plain
layer at (0,0) size 816x585
  RenderView at (0,0) size 800x585
layer at (0,0) size 816x16
  RenderBlock (floating) {HTML} at (0,0) size 816x16
    RenderBlock (anonymous) at (0,0) size 816x0
      RenderInline {SPAN} at (0,0) size 0x0
      RenderBody {BODY} at (8,8) size 800x0
        RenderBlock {DIV} at (0,0) size 800x0
        RenderBlock (anonymous) at (0,0) size 800x0
          RenderInline {SPAN} at (0,0) size 0x0
          RenderBlock (floating) {HTML} at (0,0) size 800x0
            RenderInline {Q} at (0,0) size 14x18
              RenderInline (generated) at (0,0) size 8x18
                RenderQuote at (0,0) size 8x18
                  RenderText at (150,0) size 8x18
                    text run at (150,0) width 8: "\""
              RenderInline (generated) at (0,0) size 7x18
                RenderQuote at (0,0) size 7x18
                  RenderText at (157,0) size 7x18
                    text run at (157,0) width 7: "\""
            RenderTextControl {INPUT} at (2,2) size 147x19 [bgcolor=#FFFFFF] [border: (2px inset #000000)]
    RenderBlock {DIV} at (0,0) size 816x0
layer at (13,13) size 141x13
  RenderBlock {DIV} at (3,3) size 141x13
layer at (8,31) size 800x400
  RenderVideo {VIDEO} at (0,23) size 800x400
#EOF
#EOF
#EOF
Comment 2 Gabriel Nava Marino 2021-12-08 11:25:36 PST
Created attachment 446394 [details]
Patch
Comment 3 Gabriel Nava Marino 2021-12-08 11:34:27 PST
<rdar://problem/85503666>
Comment 4 zalan 2021-12-10 09:25:45 PST
Comment on attachment 446394 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=446394&action=review

> Source/WebCore/ChangeLog:17
> +        or be able to shrink to avoid floats. However, we can have a scenario where the current child block
> +        doesnât have a float, but one of its descendants does. In this case, although we should continue to

and we don't propagate this "contains float" state to the ancestors when we insert the float to a descendant block? I wonder how this works during layout when we need to discover these floats.
Comment 5 Gabriel Nava Marino 2021-12-10 09:55:20 PST
(In reply to zalan from comment #4)
> Comment on attachment 446394 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=446394&action=review
> 
> > Source/WebCore/ChangeLog:17
> > +        or be able to shrink to avoid floats. However, we can have a scenario where the current child block
> > +        doesnât have a float, but one of its descendants does. In this case, although we should continue to
> 
> and we don't propagate this "contains float" state to the ancestors when we
> insert the float to a descendant block? I wonder how this works during
> layout when we need to discover these floats.

Currently this "contains float" state is only a function of the block's `m_floatingObjects` set:
```
bool containsFloats() const override { return m_floatingObjects && !m_floatingObjects->set().isEmpty(); }
```

and

```
bool RenderBlockFlow::containsFloat(RenderBox& renderer) const
{
    return m_floatingObjects && m_floatingObjects->set().contains<FloatingObjectHashTranslator>(renderer);
}
```

so only the object that contains this float will have this state. There is no analogous flag.

It is not a flag.
Comment 6 EWS 2021-12-10 11:38:56 PST
Committed r286866 (245097@main): <https://commits.webkit.org/245097@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 446394 [details].
Comment 7 Darin Adler 2021-12-10 11:58:42 PST
Comment on attachment 446394 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=446394&action=review

> Source/WebCore/rendering/RenderBlockFlow.cpp:2121
> +    bool contains = m_floatingObjects && m_floatingObjects->set().contains<FloatingObjectHashTranslator>(renderer);
> +    for (auto& block : childrenOfType<RenderBlock>(*this)) {
> +        if (!is<RenderBlockFlow>(block))
> +            continue;
> +        auto& blockFlow = downcast<RenderBlockFlow>(block);
> +        contains |= blockFlow.subtreeContainsFloat(renderer);
> +    }
> +    return contains;

Since this boolean only gets more true over time, we should not use a local variable. We should just return true when we find something, and then return false at the end of the function.

> Source/WebCore/rendering/RenderBlockFlow.cpp:2133
> +    bool contains = m_floatingObjects && !m_floatingObjects->set().isEmpty();
> +    for (auto& block : childrenOfType<RenderBlock>(*this)) {
> +        if (!is<RenderBlockFlow>(block))
> +            continue;
> +        auto& blockFlow = downcast<RenderBlockFlow>(block);
> +        contains |= blockFlow.subtreeContainsFloats();
> +    }
> +    return contains;

Ditto.
Comment 8 Gabriel Nava Marino 2021-12-10 16:22:24 PST
Reopening to attach new patch.
Comment 9 Gabriel Nava Marino 2021-12-10 16:22:27 PST
Created attachment 446840 [details]
Patch
Comment 10 Gabriel Nava Marino 2021-12-10 16:23:15 PST
(In reply to Darin Adler from comment #7)
> Comment on attachment 446394 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=446394&action=review
> 
> > Source/WebCore/rendering/RenderBlockFlow.cpp:2121
> > +    bool contains = m_floatingObjects && m_floatingObjects->set().contains<FloatingObjectHashTranslator>(renderer);
> > +    for (auto& block : childrenOfType<RenderBlock>(*this)) {
> > +        if (!is<RenderBlockFlow>(block))
> > +            continue;
> > +        auto& blockFlow = downcast<RenderBlockFlow>(block);
> > +        contains |= blockFlow.subtreeContainsFloat(renderer);
> > +    }
> > +    return contains;
> 
> Since this boolean only gets more true over time, we should not use a local
> variable. We should just return true when we find something, and then return
> false at the end of the function.
> 
> > Source/WebCore/rendering/RenderBlockFlow.cpp:2133
> > +    bool contains = m_floatingObjects && !m_floatingObjects->set().isEmpty();
> > +    for (auto& block : childrenOfType<RenderBlock>(*this)) {
> > +        if (!is<RenderBlockFlow>(block))
> > +            continue;
> > +        auto& blockFlow = downcast<RenderBlockFlow>(block);
> > +        contains |= blockFlow.subtreeContainsFloats();
> > +    }
> > +    return contains;
> 
> Ditto.

Thank you for the suggestion Darin! I have updated the patch so that we have this early return.
Comment 11 Darin Adler 2021-12-10 16:36:34 PST
Comment on attachment 446840 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=446840&action=review

> Source/WebCore/rendering/RenderBlockFlow.cpp:2121
> +        return blockFlow.subtreeContainsFloat(renderer);

This is wrong. Should be:

    if (blockFlow.subtreeContainsFloat(renderer))
        return true;

One other thought: we could probably do a version of this algorithm that walks the tree without using recursion.

> Source/WebCore/rendering/RenderBlockFlow.cpp:2136
> +        return blockFlow.subtreeContainsFloats();

Ditto.
Comment 12 Gabriel Nava Marino 2021-12-10 16:49:05 PST
(In reply to Darin Adler from comment #11)
> Comment on attachment 446840 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=446840&action=review
> 
> > Source/WebCore/rendering/RenderBlockFlow.cpp:2121
> > +        return blockFlow.subtreeContainsFloat(renderer);
> 
> This is wrong. Should be:

Thank you for pointing this out. I now see the issue, we would be returning early on the first child, without checking the other siblings.

> 
>     if (blockFlow.subtreeContainsFloat(renderer))
>         return true;
> 
> One other thought: we could probably do a version of this algorithm that
> walks the tree without using recursion.

In WebKit, is there a preference for using one vs. the other?

> 
> > Source/WebCore/rendering/RenderBlockFlow.cpp:2136
> > +        return blockFlow.subtreeContainsFloats();
> 
> Ditto.
Comment 13 Gabriel Nava Marino 2021-12-10 20:07:33 PST
Created attachment 446856 [details]
Patch
Comment 14 Gabriel Nava Marino 2021-12-13 14:07:55 PST
There are ios-wk2 failures, but they appear unrelated to the changes in this bug and tracked in Bug 234181.
Comment 15 Darin Adler 2021-12-13 14:32:25 PST
(In reply to Gabriel Nava Marino from comment #12)
> (In reply to Darin Adler from comment #11)
> > One other thought: we could probably do a version of this algorithm that
> > walks the tree without using recursion.
> 
> In WebKit, is there a preference for using one vs. the other?

I think we prefer non-recursive.

Our tree structure is designed so we can walk it without using a stack, and write non-recursive versions of algorithms that involve walking the entire tree, and I think we want to take advantage of that when possible for better efficiency.
Comment 16 Gavin 2021-12-14 09:25:04 PST
*** Bug 233554 has been marked as a duplicate of this bug. ***
Comment 17 Gabriel Nava Marino 2021-12-14 16:16:46 PST
(In reply to Darin Adler from comment #15)
> (In reply to Gabriel Nava Marino from comment #12)
> > (In reply to Darin Adler from comment #11)
> > > One other thought: we could probably do a version of this algorithm that
> > > walks the tree without using recursion.
> > 
> > In WebKit, is there a preference for using one vs. the other?
> 
> I think we prefer non-recursive.
> 
> Our tree structure is designed so we can walk it without using a stack, and
> write non-recursive versions of algorithms that involve walking the entire
> tree, and I think we want to take advantage of that when possible for better
> efficiency.

I have attempted the non-recursive route using an iterator, but unfortunately the render descendant iterator does not currently work with const objects. That issue is tracked in bug 234325. I am moving these changes to commit-queue? for now, and filing a separate bug to bring non-recursion via the iterator blocked on 234325.
Comment 18 Darin Adler 2021-12-14 17:38:33 PST
(In reply to Gabriel Nava Marino from comment #17)
> the render descendant iterator does not currently work with
> const objects

I’d suggest using const_cast to work around that.
Comment 19 Gabriel Nava Marino 2021-12-15 10:28:39 PST
Created attachment 447254 [details]
Patch
Comment 20 Gabriel Nava Marino 2021-12-15 10:30:25 PST
(In reply to Darin Adler from comment #18)
> (In reply to Gabriel Nava Marino from comment #17)
> > the render descendant iterator does not currently work with
> > const objects
> 
> I’d suggest using const_cast to work around that.

Thank you for the recommendation! I have updated the patch.
Comment 21 Gabriel Nava Marino 2021-12-15 13:48:53 PST
*** Bug 234326 has been marked as a duplicate of this bug. ***
Comment 22 Gabriel Nava Marino 2022-01-07 10:52:33 PST
(In reply to Darin Adler from comment #18)
> (In reply to Gabriel Nava Marino from comment #17)
> > the render descendant iterator does not currently work with
> > const objects
> 
> I’d suggest using const_cast to work around that.

This approach worked and seems to have made EWS happy, so I have updated the cq flag to cq?.
Comment 23 Darin Adler 2022-01-07 10:58:59 PST
Comment on attachment 447254 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=447254&action=review

This is great. Fine to land as is.

To refine further, we can take one more step by adding inclusiveDescendantsOfType to RenderDescendantIterator.h, which we already have in TypedElementDecendantIterator.h. If we do that, we can remove the check at the top of the function, and have only the loop.

> Source/WebCore/rendering/RenderBlockFlow.cpp:2122
>          if (!is<RenderBlockFlow>(block))
>              continue;
>          auto& blockFlow = downcast<RenderBlockFlow>(block);
> -        contains |= blockFlow.subtreeContainsFloat(renderer);
> +        if (blockFlow.containsFloat(renderer))
> +            return true;

To refine further we *could* collapse these 5 lines into two:

    if (is<RenderBlockFlow>(block) && downcast<RenderBlockFlow>(block).containsFloat(renderer))
        return true;
Comment 24 EWS 2022-01-07 11:36:10 PST
Committed r287771 (245831@main): <https://commits.webkit.org/245831@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 447254 [details].
Comment 25 Gabriel Nava Marino 2022-01-10 09:42:47 PST
(In reply to Darin Adler from comment #23)
> Comment on attachment 447254 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=447254&action=review
> 
> This is great. Fine to land as is.
> 
> To refine further, we can take one more step by adding
> inclusiveDescendantsOfType to RenderDescendantIterator.h, which we already
> have in TypedElementDecendantIterator.h. If we do that, we can remove the
> check at the top of the function, and have only the loop.
> 
> > Source/WebCore/rendering/RenderBlockFlow.cpp:2122
> >          if (!is<RenderBlockFlow>(block))
> >              continue;
> >          auto& blockFlow = downcast<RenderBlockFlow>(block);
> > -        contains |= blockFlow.subtreeContainsFloat(renderer);
> > +        if (blockFlow.containsFloat(renderer))
> > +            return true;
> 
> To refine further we *could* collapse these 5 lines into two:
> 
>     if (is<RenderBlockFlow>(block) &&
> downcast<RenderBlockFlow>(block).containsFloat(renderer))
>         return true;

Thank you for the feedback. I will update these functions per the refinement recommendations.
Comment 26 Gabriel Nava Marino 2022-01-13 15:09:32 PST
Created attachment 449116 [details]
Patch
Comment 27 Gabriel Nava Marino 2022-01-13 15:11:44 PST
Created attachment 449117 [details]
Patch
Comment 28 Darin Adler 2022-01-13 15:53:46 PST
Comment on attachment 449117 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=449117&action=review

> Source/WebCore/ChangeLog:9
> +        Implement InclusiveRenderDescendantIteratorAdapter and use inclusiveDescendantsOfType in
> +        RenderBlockFlow::subtreeContainsFloat and RenderBlockFlow::subtreeContainsFloats.

I suspect there is a way to implement this sharing more code with the non-inclusive case, since non-inclusive is just like inclusive, but just skipping the thing itself, but it’s more important how we *use* inclusiveDescendantsOfType than how smooth the implementation is. That can always be refined later.

I would suggest, though, that we address the const-ness issue eventually. Maybe even at the same time.

Ideally the descendantsOfType on a const reference would iterate const references, and non-const would iterate non-const. Ideally without duplicating all the code twice, so maybe using some const_cast in the implementation.

> Source/WebCore/rendering/RenderBlockFlow.cpp:1957
> +    for (auto& block : inclusiveDescendantsOfType<RenderBlock>(const_cast<RenderBlockFlow&>(*this))) {
> +        if (is<RenderBlockFlow>(block) && downcast<RenderBlockFlow>(block).containsFloat(renderer))
>              return true;
>      }

Hmm, can we write this?

    for (auto& block : inclusiveDescendantsOfType<RenderBlockFlow>(*this)) {
        if (block.containsFloat(renderer))
            return true;
    }

That’s the whole point of having the xxxOfType feature (and assumes the const).
Comment 29 Gabriel Nava Marino 2022-01-27 10:24:57 PST
Comment on attachment 449117 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=449117&action=review

>> Source/WebCore/ChangeLog:9
>> +        RenderBlockFlow::subtreeContainsFloat and RenderBlockFlow::subtreeContainsFloats.
> 
> I suspect there is a way to implement this sharing more code with the non-inclusive case, since non-inclusive is just like inclusive, but just skipping the thing itself, but it’s more important how we *use* inclusiveDescendantsOfType than how smooth the implementation is. That can always be refined later.
> 
> I would suggest, though, that we address the const-ness issue eventually. Maybe even at the same time.
> 
> Ideally the descendantsOfType on a const reference would iterate const references, and non-const would iterate non-const. Ideally without duplicating all the code twice, so maybe using some const_cast in the implementation.

Thank you very much for the suggestion Darin. I have some ideas on how I might tackle the implementation of the inclusive and non-inclusive cases as well as how one might address the const-ness issue using the recommended tips.

Given the scope of the changes required, I would prefer to tackle this separately in https://bugs.webkit.org/show_bug.cgi?id=235680, as I expect it will require updating areas of code currently using const_cast with descendantsOfType.

>> Source/WebCore/rendering/RenderBlockFlow.cpp:1957
>>      }
> 
> Hmm, can we write this?
> 
>     for (auto& block : inclusiveDescendantsOfType<RenderBlockFlow>(*this)) {
>         if (block.containsFloat(renderer))
>             return true;
>     }
> 
> That’s the whole point of having the xxxOfType feature (and assumes the const).

Thank you for pointing this out! I will update the patch.

However, please see my other comment. I would prefer to deal with the const issue separately in another patch for a cleaner change-set.
Comment 30 Darin Adler 2022-01-27 10:26:22 PST
Sounds great.
Comment 31 Gabriel Nava Marino 2022-01-27 10:27:28 PST
Created attachment 450152 [details]
Patch
Comment 32 Darin Adler 2022-01-27 10:28:00 PST
Comment on attachment 449117 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=449117&action=review

>>> Source/WebCore/rendering/RenderBlockFlow.cpp:1957
>>>      }
>> 
>> Hmm, can we write this?
>> 
>>     for (auto& block : inclusiveDescendantsOfType<RenderBlockFlow>(*this)) {
>>         if (block.containsFloat(renderer))
>>             return true;
>>     }
>> 
>> That’s the whole point of having the xxxOfType feature (and assumes the const).
> 
> Thank you for pointing this out! I will update the patch.
> 
> However, please see my other comment. I would prefer to deal with the const issue separately in another patch for a cleaner change-set.

Yes, eventually we’ll deal with const. But built-in type checking is available now.
Comment 33 Darin Adler 2022-01-27 10:30:49 PST
Comment on attachment 450152 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=450152&action=review

> Source/WebCore/ChangeLog:3
> +        nullptr deref in ComputeFloatOffsetForLineLayoutAdapter<FloatingObject::FloatLeft>::updateOffsetIfNeeded

This doesn’t seem the right title any more. At this point, this patch is a refactor so the title would be:

    Simplify code by using inclusiveDescendantsOfType

The reason I suggested fixing the const, is that it’s a similar refactoring, but fine to do this one step at a time.
Comment 34 Darin Adler 2022-01-27 10:31:57 PST
Doing this clean-up and refactoring all on this original bug isn’t the ideal way to use bugs.webkit.org; I would have used a new bug. But let’s just get the change in.
Comment 35 Gabriel Nava Marino 2022-01-27 10:34:34 PST
(In reply to Darin Adler from comment #34)
> Doing this clean-up and refactoring all on this original bug isn’t the ideal
> way to use bugs.webkit.org; I would have used a new bug. But let’s just get
> the change in.

I will be sure to create a new bug for these type of clean-up and refactoring-related changes in subsequent patches.
Comment 36 Gabriel Nava Marino 2022-01-27 10:41:37 PST
(In reply to Darin Adler from comment #33)
> Comment on attachment 450152 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=450152&action=review
> 
> > Source/WebCore/ChangeLog:3
> > +        nullptr deref in ComputeFloatOffsetForLineLayoutAdapter<FloatingObject::FloatLeft>::updateOffsetIfNeeded
> 
> This doesn’t seem the right title any more. At this point, this patch is a
> refactor so the title would be:
> 
>     Simplify code by using inclusiveDescendantsOfType
> 

Thank you for the suggestion. I will update the title. 

> The reason I suggested fixing the const, is that it’s a similar refactoring,
> but fine to do this one step at a time.

Thank you, I see. I was not sure about the scope of the required changes to fix the const issue outside of RenderBlockFlow, and so that is why I originally created the other bug.
Comment 37 Gabriel Nava Marino 2022-01-27 10:42:57 PST
Created attachment 450157 [details]
Patch