[webkit-changes] [WebKit/WebKit] cd7e28: heap-use-after-free | WebCore::RenderMenuList::set...

[Aditya Keerthi]

  Branch: refs/heads/main
  Home:   https://github.com/WebKit/WebKit
  Commit: cd7e28cf47f603ff24c10eae96a5bf8ccbae8713
      https://github.com/WebKit/WebKit/commit/cd7e28cf47f603ff24c10eae96a5bf8ccbae8713
  Author: Aditya Keerthi <akeerthi at apple.com>
  Date:   2024-08-05 (Mon, 05 Aug 2024)

  Changed paths:
    A LayoutTests/fast/css/container-query-listbox-expected.html
    A LayoutTests/fast/css/container-query-listbox.html
    A LayoutTests/fast/forms/select-multiple-changed-with-containment-crash-expected.txt
    A LayoutTests/fast/forms/select-multiple-changed-with-containment-crash.html
    M LayoutTests/platform/ios/TestExpectations
    M Source/WebCore/rendering/RenderListBox.cpp
    M Source/WebCore/rendering/RenderMenuList.cpp

  Log Message:
  -----------
  heap-use-after-free | WebCore::RenderMenuList::setTextFromOption; WebCore::HTMLSelectElement::selectOption; WebCore::Element::didAddAttribute
https://bugs.webkit.org/show_bug.cgi?id=272882
rdar://126279123

Reviewed by Antti Koivisto.

On macOS, `<select>` and `<select multiple>` use `RenderMenuList` and
`RenderMenuList` as their respective renderers. Consequently, whenever the
`multiple` attribute is added, `invalidateStyleAndRenderersForSubtree` is
called and the `RenderMenuList` is marked for destruction.

Additionally, for interoperability, the selected index must be updated when the
`multiple` attribute is added or removed. This update will also trigger an
update on the renderer, in this case, via `RenderMenuList::updateFromElement`.

At this point, the element is `<select multiple>`, but still has a `RenderMenuList`.
Eventually, the update gets into `RenderMenuList::setTextFromOption`, which
calls `computedStyle()` on an `<option>` element. Following 267786 at main, when
using containment, this triggers a render tree update, as `Document::resolveStyle`
is called, and `resolver.hasUnresolvedQueryContainers()` is true. The
`RenderMenuList` is then destroyed, as it was previously invalidated, while
inside one of its own methods. Use-after-free is then encountered due to attempted
member variable access.

To fix, take a similar approach as the crash fix in 272334 at main and elide a full
style update when a query container with invalid style is encountered.
`fast/css/container-query-listbox.html` has been added to ensure <option>
styling continues to work with container queries. Finally, adopt `CheckedPtr` as
a hardening measure.

Alternatives considered:

1. Call `updateStyleIfNeeded()` in `HTMLSelectElement` prior to entering the
   renderer. This approach was rejected as there are too many entry points, and
   it would be fragile to new entry points.

2. Pass `<option>` style down from `HTMLSelectElement` into the renderer. Again,
   there are too many entry points (including outside of the element). Additionally,
   it is not sufficient to store a single style (for the selected option), as every
   `<option>` participates in width determination.

3. Use `existingComputedStyle()` instead of `computedStyle()`. This resulted in
   paint time regressions where the existing computed style was empty.

* LayoutTests/fast/css/container-query-listbox-expected.html: Added.
* LayoutTests/fast/css/container-query-listbox.html: Added.
* LayoutTests/fast/forms/select-multiple-changed-with-containment-crash-expected.txt: Added.
* LayoutTests/fast/forms/select-multiple-changed-with-containment-crash.html: Added.
* LayoutTests/platform/ios/TestExpectations:
* Source/WebCore/html/HTMLSelectElement.cpp:
(WebCore::HTMLSelectElement::optionSelectedByUser):
(WebCore::HTMLSelectElement::selectOption):
* Source/WebCore/rendering/RenderListBox.cpp:
(WebCore::RenderListBox::paintItemForeground):
(WebCore::RenderListBox::paintItemBackground):
* Source/WebCore/rendering/RenderMenuList.cpp:
(RenderMenuList::updateOptionsWidth):
(RenderMenuList::setTextFromOption):
(RenderMenuList::itemStyle const):
(RenderMenuList::getItemBackgroundColor const):

Originally-landed-as: 272448.982 at safari-7618-branch (c4b6c7757697). rdar://132958569
Canonical link: https://commits.webkit.org/281864@main



To unsubscribe from these emails, change your notification settings at https://github.com/WebKit/WebKit/settings/notifications