Bug 236988

Summary: AX: AccessibilityObject::ariaTreeRows can crash in a deep hierarchy due to recursive descent
Product: WebKit Reporter: Tyler Wilcock <tyler_w>
Component: AccessibilityAssignee: Tyler Wilcock <tyler_w>
Status: NEW    
Severity: Normal CC: aakash_jain, aboxhall, andresg_22, apinheiro, cfleizach, dmazzoni, ews-watchlist, jcraig, jdiggs, samuel_white, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch ews-feeder: commit-queue-

Tyler Wilcock
Reported 2022-02-21 12:01:39 PST
AccessibilityObject::ariaTreeRows calls itself recursively on its child objects all the way down to the leaves. Although WebKit tree depth is limited to 512 levels (https://github.com/WebKit/WebKit/blob/2077b50205f4d8f943b88e233302b52c8b4699af/Source/WebCore/page/SettingsBase.h#L72#L73), this can still sometimes cause a stack overflow. ITM is especially vulnerable to this, as currently we call ariaTreeRows on every single isolated object we create.
Attachments
Patch (10.59 KB, patch)
2022-02-21 12:13 PST, Tyler Wilcock 		no flags
DetailsFormatted DiffDiff
Patch (10.71 KB, patch)
2022-02-21 12:27 PST, Tyler Wilcock 		ews-feeder: commit-queue-
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Radar WebKit Bug Importer
Comment 1 2022-02-21 12:01:55 PST
<rdar://problem/89249539>
Tyler Wilcock
Comment 2 2022-02-21 12:13:16 PST
Created attachment 452753 [details] Patch
Tyler Wilcock
Comment 3 2022-02-21 12:27:31 PST
Created attachment 452756 [details] Patch
Andres Gonzalez
Comment 4 2022-02-21 12:44:02 PST
(In reply to Tyler Wilcock from comment #3) > Created attachment 452756 [details] > Patch --- a/Source/WebCore/accessibility/AccessibilityObject.cpp +++ a/Source/WebCore/accessibility/AccessibilityObject.cpp + // This method descends recursively, so avoid stack overflow by bailing early if we're >450 levels away from where we started. + if (ancestors.size() > 450) Can we add the comment here about the arbitrary choice of 450 and a FIXME for making this non-recursive? Does the test fail/crash without this change?
Aakash Jain
Comment 5 2022-02-25 06:39:53 PST
I cancelled https://ews-build.webkit.org/#/builders/68/builds/9128 to speed up ios-wk2 queue. It finished running layout-tests and indicated accessibility/deep-aria-tree-rows-crash.html failure
chris fleizach
Comment 6 2022-02-25 09:08:20 PST
Comment on attachment 452756 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=452756&action=review > Source/WebCore/accessibility/AccessibilityObject.cpp:1905 > The code for going through aria owns and descendants looks identical. Can we merge those blocks? 450 seems arbitrarily large. Can we cap this to 10 and still be sufficient?
Andres Gonzalez
Comment 7 2022-02-25 09:30:23 PST
(In reply to chris fleizach from comment #6) > Comment on attachment 452756 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=452756&action=review > > > Source/WebCore/accessibility/AccessibilityObject.cpp:1905 > > > > The code for going through aria owns and descendants looks identical. Can we > merge those blocks? > 450 seems arbitrarily large. Can we cap this to 10 and still be sufficient? I think we agreed that this change is not needed, so this can be closed.
Note You need to log in before you can comment on or make changes to this bug.