Bug 224894 - Nullptr crash in CollectionTraversal::traverseForward for selectedOptions
Summary: Nullptr crash in CollectionTraversal::traverseForward for selectedOptions
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Rob Buis
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-04-21 14:07 PDT by Ryosuke Niwa
Modified: 2021-04-24 00:42 PDT (History)
15 users (show)

See Also:


Attachments
Test (450 bytes, text/html)
2021-04-21 14:10 PDT, Ryosuke Niwa
no flags Details
Patch (4.39 KB, patch)
2021-04-22 08:58 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (6.36 KB, patch)
2021-04-23 00:27 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (6.43 KB, patch)
2021-04-23 23:56 PDT, Rob Buis
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 2021-04-21 14:07:37 PDT
e.g.

Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   com.apple.WebCore                   0x000000010f83a340 WebCore::ContainerNode::firstChild() const + 0 (ContainerNode.h:43) [inlined]
1   com.apple.WebCore                   0x000000010f83a340 WebCore::Node* WebCore::NodeTraversal::traverseNextTemplate<WebCore::ContainerNode const>(WebCore::ContainerNode const&, WebCore::Node const*) + 0 (NodeTraversal.h:90) [inlined]
2   com.apple.WebCore                   0x000000010f83a340 WebCore::NodeTraversal::next(WebCore::ContainerNode const&, WebCore::Node const*) + 0 (NodeTraversal.h:99) [inlined]
3   com.apple.WebCore                   0x000000010f83a340 WebCore::Element* WebCore::Traversal<WebCore::Element>::nextTemplate<WebCore::ContainerNode const>(WebCore::ContainerNode const&, WebCore::Node const*) + 0 (ElementTraversal.h:106) [inlined]
4   com.apple.WebCore                   0x000000010f83a340 WebCore::Traversal<WebCore::Element>::next(WebCore::ContainerNode const&, WebCore::Node const*) + 0 (ElementTraversal.h:252) [inlined]
5   com.apple.WebCore                   0x000000010f83a340 WebCore::ElementIterator<WebCore::Element>::traverseNext() + 8 (ElementIterator.h:87) [inlined]
6   com.apple.WebCore                   0x000000010f83a340 WebCore::ElementDescendantIterator<WebCore::Element>::operator++() + 8 (TypedElementDescendantIterator.h:127) [inlined]
7   com.apple.WebCore                   0x000000010f83a340 void WebCore::CollectionTraversal<(WebCore::CollectionTraversalType)0>::traverseForward<WebCore::GenericCachedHTMLCollection<(WebCore::CollectionTraversalType)0> >(WebCore::GenericCachedHTMLCollection<(WebCore::CollectionTraversalType)0> const&, WebCore::ElementDescendantIterator<WebCore::Element>&, unsigned int, unsigned int&) + 44 (CollectionTraversal.h:81) [inlined]
8   com.apple.WebCore                   0x000000010f83a340 WebCore::CachedHTMLCollection<WebCore::GenericCachedHTMLCollection<(WebCore::CollectionTraversalType)0>, (WebCore::CollectionTraversalType)0>::collectionTraverseForward(WebCore::ElementDescendantIterator<WebCore::Element>&, unsigned int, unsigned int&) const + 48 (CachedHTMLCollection.h:58)
9   com.apple.WebCore                   0x000000010f83a5ea WebCore::CollectionIndexCache<WebCore::GenericCachedHTMLCollection<(WebCore::CollectionTraversalType)0>, WebCore::ElementDescendantIterator<WebCore::Element> >::nodeAt(WebCore::GenericCachedHTMLCollection<(WebCore::CollectionTraversalType)0> const&, unsigned int) + 538 (CollectionIndexCache.h:202)
10  com.apple.WebCore                   0x000000010ead662e WebCore::jsHTMLCollectionPrototypeFunction_itemBody(JSC::JSGlobalObject*, JSC::CallFrame*, WebCore::JSHTMLCollection*) + 63 (JSHTMLCollection.cpp:294) [inlined]

<rdar://76946123>
Comment 1 Ryosuke Niwa 2021-04-21 14:10:57 PDT
Created attachment 426743 [details]
Test
Comment 2 Rob Buis 2021-04-22 08:58:30 PDT
Created attachment 426815 [details]
Patch
Comment 3 Ryosuke Niwa 2021-04-22 19:08:13 PDT
Comment on attachment 426815 [details]
Patch

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

> Source/WebCore/dom/CollectionIndexCache.h:202
> +        collection.collectionTraverseForward(current, index, currentIndex);

What!? tcollectionTraverseForward is invalidating this collection?
That's certainly wrong! We need to fix that.
Comment 4 Rob Buis 2021-04-22 22:47:42 PDT
Comment on attachment 426815 [details]
Patch

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

>> Source/WebCore/dom/CollectionIndexCache.h:202
>> +        collection.collectionTraverseForward(current, index, currentIndex);
> 
> What!? tcollectionTraverseForward is invalidating this collection?
> That's certainly wrong! We need to fix that.

Indirectly it does. elementMatches will call HTMLOptionElement::selected.  This can call updateListItemSelectedStates on HTMLSelectElement, this can call HTMLOptionElement::setSelectedState which will call HTMLSelectElement::invalidateSelectedItems, which will call invalidateCache on the SelectedOptions cache. Obviously the test case triggers exactly this chain of events. But it seems reasonable to me for the option to ask the select to update itself so selected really reflects the state of selection (I guess this is just in time design)...
Comment 5 Rob Buis 2021-04-22 23:16:42 PDT
A thing about the test, it builds a DOM tree under <select> that would not be accepted as-is when parsed. However Firefox and chromium also build the desired DOM tree when created dynamically.
Comment 6 Ryosuke Niwa 2021-04-22 23:49:46 PDT
(In reply to Rob Buis from comment #4)
> Comment on attachment 426815 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=426815&action=review
> 
> >> Source/WebCore/dom/CollectionIndexCache.h:202
> >> +        collection.collectionTraverseForward(current, index, currentIndex);
> > 
> > What!? tcollectionTraverseForward is invalidating this collection?
> > That's certainly wrong! We need to fix that.
> 
> Indirectly it does. elementMatches will call HTMLOptionElement::selected. 
> This can call updateListItemSelectedStates on HTMLSelectElement, this can
> call HTMLOptionElement::setSelectedState which will call
> HTMLSelectElement::invalidateSelectedItems, which will call invalidateCache
> on the SelectedOptions cache.

This is not okay. The HTML collection traversal cannot have a side effect like this. We need to change this so that we don't invalidate the HTMLCollection while we're traversing options to find the selected options.

It is a bit suspicious that HTMLOptionElement::setSelectedState calls HTMLSelectElement::invalidateSelectedItems.

Maybe the right fix is to get rid of that call and instead invalidate the HTML Collection in HTMLSelectElement::updateListBoxSelection, HTMLSelectElement::selectOption, HTMLSelectElement::deselectItemsWithoutValidation, HTMLSelectElement::restoreFormControlState, HTMLSelectElement::reset(), and HTMLSelectElement::updateSelectedState
Comment 7 Rob Buis 2021-04-23 00:27:14 PDT
Created attachment 426894 [details]
Patch
Comment 8 Rob Buis 2021-04-23 01:48:05 PDT
Comment on attachment 426815 [details]
Patch

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

>>>> Source/WebCore/dom/CollectionIndexCache.h:202
>>>> +        collection.collectionTraverseForward(current, index, currentIndex);
>>> 
>>> What!? tcollectionTraverseForward is invalidating this collection?
>>> That's certainly wrong! We need to fix that.
>> 
>> Indirectly it does. elementMatches will call HTMLOptionElement::selected.  This can call updateListItemSelectedStates on HTMLSelectElement, this can call HTMLOptionElement::setSelectedState which will call HTMLSelectElement::invalidateSelectedItems, which will call invalidateCache on the SelectedOptions cache. Obviously the test case triggers exactly this chain of events. But it seems reasonable to me for the option to ask the select to update itself so selected really reflects the state of selection (I guess this is just in time design)...
> 
> This is not okay. The HTML collection traversal cannot have a side effect like this. We need to change this so that we don't invalidate the HTMLCollection while we're traversing options to find the selected options.
> 
> It is a bit suspicious that HTMLOptionElement::setSelectedState calls HTMLSelectElement::invalidateSelectedItems.
> 
> Maybe the right fix is to get rid of that call and instead invalidate the HTML Collection in HTMLSelectElement::updateListBoxSelection, HTMLSelectElement::selectOption, HTMLSelectElement::deselectItemsWithoutValidation, HTMLSelectElement::restoreFormControlState, HTMLSelectElement::reset(), and HTMLSelectElement::updateSelectedState

Thanks, that makes a lot of sense. I implemented that and added a somewhat revealing ChangeLog and test, since I think this is not a security issue.
Comment 9 EWS 2021-04-23 23:47:50 PDT
ChangeLog entry in LayoutTests/ChangeLog contains OOPS!.
Comment 10 Rob Buis 2021-04-23 23:56:42 PDT
Created attachment 426975 [details]
Patch
Comment 11 EWS 2021-04-24 00:42:02 PDT
Committed r276547 (236987@main): <https://commits.webkit.org/236987@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 426975 [details].