221650 2021-02-09 21:24:08 -0800 Nullptr crash in ModifySelectionListLevelCommand::appendSiblingNodeRange 2021-02-23 11:40:08 -0800 1 1 1 Unclassified WebKit HTML Editing WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED InRadar P2 Normal --- 1 rniwa fred.wang bfulgham cgarcia ews-feeder fred.wang gpoo product-security rbuis svillar webkit-bug-importer wenson_hsieh oldest_to_newest 1727422 0 419813 rniwa 2021-02-09 21:24:08 -0800 Created attachment 419813 Test e.g. Thread 0 Crashed:: Dispatch queue: com.apple.main-thread 0 com.apple.WebCore 0x00007fff3b1a22d9 WebCore::ModifySelectionListLevelCommand::appendSiblingNodeRange(WebCore::Node*, WebCore::Node*, WebCore::Element*) + 57 1 com.apple.WebCore 0x00007fff3b1a24e1 WebCore::IncreaseSelectionListLevelCommand::doApply() + 337 2 com.apple.WebCore 0x00007fff3b12e10f WebCore::CompositeEditCommand::applyCommandToComposite(WTF::Ref<WebCore::EditCommand, WTF::DumbPtrTraits<WebCore::EditCommand> >&&) + 79 3 com.apple.WebCore 0x00007fff3b196fbf WebCore::InsertNestedListCommand::doApply() + 1647 4 com.apple.WebCore 0x00007fff39dcb684 WebCore::CompositeEditCommand::apply() + 500 5 com.apple.WebCore 0x00007fff3b188462 WebCore::executeInsertNestedOrderedList(WebCore::Frame&, WebCore::Event*, WebCore::EditorCommandSource, WTF::String const&) + 98 6 com.apple.WebCore 0x00007fff39e30f7d WebCore::Document::execCommand(WTF::String const&, bool, WTF::String const&) + 77 7 com.apple.WebCore 0x00007fff3a2324cc WebCore::jsDocumentPrototypeFunctionExecCommand(JSC::JSGlobalObject*, JSC::CallFrame*) + 428 <rdar://problem/74148852> 1730782 1 420807 fred.wang 2021-02-18 01:24:29 -0800 Created attachment 420807 Patch So the issue here is that ModifySelectionListLevelCommand::appendSiblingNodeRange browses the list of children from the startNode to the endNode, assuming they are siblings in that order. But will do a null pointer dereference if that's assumption is false: https://webkit-search.igalia.com/webkit/rev/96373eab5c9a5d968cdfac1e0c0069773d717c23/Source/WebCore/editing/ModifySelectionListLevel.cpp#123 Debugging backwards (see output below), the mistake is happening in getStartEndListChildren where the end node is moved to the next sibling in the render tree, which does not correspond to a sibling in the DOM tree (here, because the SLOT element does not generate any renderer). It seems the crash in ModifySelectionListLevelCommand::appendSiblingNodeRange is not a security bug (just a nullptr dereference) so I'm adding the tests to the patch for now and can change if you disagree. I haven't checked whether the mistake in getStartEndListChildren can cause similar issues, but that function is used for both list level increment and decrement. (rr) p showTree(startListChild) BODY 0x7f76f41f7d30 (renderer 0x7f76f41f57c0) SLOT 0x7f76f41f7dc0 (renderer (nil)) #text 0x7f76f41f7e50 "\n " LI 0x7f76f41f7ec0 (renderer 0x7f76f41e8190) * LI 0x7f76f41e8800 (renderer 0x7f76f41e8890) #text 0x7f76f41f7f50 "\n" #text 0x7f76f41e8010 "\n" OL 0x7f76f41e8080 (renderer 0x7f76f41e83f0) #text 0x7f76f41e8120 "\n" $1 = void (rr) p showTree(endListChild) BODY 0x7f76f41f7d30 (renderer 0x7f76f41f57c0) SLOT 0x7f76f41f7dc0 (renderer (nil)) #text 0x7f76f41f7e50 "\n " LI 0x7f76f41f7ec0 (renderer 0x7f76f41e8190) * LI 0x7f76f41e8800 (renderer 0x7f76f41e8890) #text 0x7f76f41f7f50 "\n" #text 0x7f76f41e8010 "\n" OL 0x7f76f41e8080 (renderer 0x7f76f41e83f0) #text 0x7f76f41e8120 "\n" $2 = void (rr) p showRenderTree(r) (B)lock/(I)nline/I(N)line-block, (A)bsolute/Fi(X)ed/(R)elative/Stic(K)y, (F)loating, (O)verflow clip, Anon(Y)mous, (G)enerated, has(L)ayer, hasLayer(S)crollableArea, (C)omposited, (+)Dirty style, (+)Dirty layout B---YGLS- -- RenderView at (0,0) size 800x600 renderer->(0x7f76f41f5240) B-----LS- -- HTML RenderBlock at (0,0) size 800x600 renderer->(0x7f76f41f56a0) node->(0x7f76f41f7a60) B-------- -- BODY RenderBody at (8,8) size 784x576 renderer->(0x7f76f41f57c0) node->(0x7f76f41f7d30) (layout overflow -1,0 785x576) (visual overflow -1,0 785x576) B-------- -- LI RenderListItem at (0,0) size 784x18 renderer->(0x7f76f41e8190) node->(0x7f76f41f7ec0) (layout overflow -1,0 785x18) (visual overflow -1,0 785x18) -------- -- Line: (top: 0 bottom: 17) with leading (top: 0 bottom: 18) -------- -- RootInlineBox at (0,0) size 14x17 (0x7f76f41e8560) renderer->(0x7f76f41e8190) -------- -- InlineBox at (-1,0) size 7x17 (0x7f76f41e8510) renderer->(0x7f76f41e82d0) I---YG--- -- RenderListMarker at (-1,0) size 7x17 renderer->(0x7f76f41e82d0) B-------- -- LI RenderListItem at (0,18) size 784x18 renderer->(0x7f76f41e8890) node->(0x7f76f41e8800) (layout overflow -1,0 785x18) (visual overflow -1,0 785x18) -------- -- Line: (top: 0 bottom: 17) with leading (top: 0 bottom: 18) -------- -- RootInlineBox at (0,0) size 14x17 (0x7f76f41e8b40) renderer->(0x7f76f41e8890) -------- -- InlineBox at (-1,0) size 7x17 (0x7f76f41e8af0) renderer->(0x7f76f41e89d0) I---YG--- -- RenderListMarker at (-1,0) size 7x17 renderer->(0x7f76f41e89d0) B-------- --* OL RenderBlock at (0,52) size 784x0 renderer->(0x7f76f41e83f0) node->(0x7f76f41e8080) $3 = void (rr) n 84 endListChild = r->node(); (rr) n 87 start = startListChild; (rr) p showTree(endListChild) BODY 0x7f76f41f7d30 (renderer 0x7f76f41f57c0) SLOT 0x7f76f41f7dc0 (renderer (nil)) #text 0x7f76f41f7e50 "\n " LI 0x7f76f41f7ec0 (renderer 0x7f76f41e8190) LI 0x7f76f41e8800 (renderer 0x7f76f41e8890) #text 0x7f76f41f7f50 "\n" #text 0x7f76f41e8010 "\n" * OL 0x7f76f41e8080 (renderer 0x7f76f41e83f0) #text 0x7f76f41e8120 "\n" $4 = void 1731254 2 fred.wang 2021-02-19 00:41:46 -0800 (In reply to Frédéric Wang (:fredw) from comment #1) > Debugging backwards (see output below), the mistake is happening in > getStartEndListChildren where the end node is moved to the next sibling in > the render tree, which does not correspond to a sibling in the DOM tree > (here, because the SLOT element does not generate any renderer). Additional remark here. The slot element has "display: contents" https://webkit-search.igalia.com/webkit/rev/6c8fdfee755dcaf35bd338726b21a7527d3917de/Source/WebCore/editing/markup.cpp#575 so this is really another issue with editing & display: contents. 1732436 3 420807 rniwa 2021-02-22 20:00:54 -0800 Comment on attachment 420807 Patch Ok, seems reasonable. 1732487 4 ews-feeder 2021-02-23 04:28:26 -0800 Committed r273302: <https://commits.webkit.org/r273302> All reviewed patches have been landed. Closing bug and clearing flags on attachment 420807. 419813 2021-02-09 21:24:08 -0800 2021-02-09 21:24:08 -0800 Test repro_510.html text/html 204 rniwa PHNjcmlwdD4KICBvbmxvYWQgPSAoKSA9PiB7CiAgICBkb2N1bWVudC5leGVjQ29tbWFuZCgnU2Vs ZWN0QWxsJyk7CiAgICBkb2N1bWVudC5kZXNpZ25Nb2RlID0gJ29uJzsKICAgIGRvY3VtZW50LmV4 ZWNDb21tYW5kKCdJbnNlcnROZXN0ZWRPcmRlcmVkTGlzdCcpOwogIH07Cjwvc2NyaXB0Pgo8c2xv dD4KICA8bGk+PC9saT4KPC9zbG90Pgo8b2w+PC9vbD4K 420807 2021-02-18 01:24:29 -0800 2021-02-23 04:28:27 -0800 Patch 0001-Bug-221650-Nullptr-crash-in-ModifySelectionListLevel.patch text/plain 4616 fred.wang RnJvbSA2YWI0NmViNzk0OWY4OTVkYTM2OWJiYjIxN2QyYjU0NjVkZWI5MmFkIE1vbiBTZXAgMTcg MDA6MDA6MDAgMjAwMQpGcm9tOiA9P1VURi04P3E/RnI9QzM9QTlkPUMzPUE5cmljPTIwV2FuZz89 IDxmd2FuZ0BpZ2FsaWEuY29tPgpEYXRlOiBUaHUsIDE4IEZlYiAyMDIxIDEwOjEzOjMzICswMTAw ClN1YmplY3Q6IFtQQVRDSF0gQnVnIDIyMTY1MCAtIE51bGxwdHIgY3Jhc2ggaW4KIE1vZGlmeVNl bGVjdGlvbkxpc3RMZXZlbENvbW1hbmQ6OmFwcGVuZFNpYmxpbmdOb2RlUmFuZ2UKCi0tLQogTGF5 b3V0VGVzdHMvQ2hhbmdlTG9nICAgICAgICAgICAgICAgICAgICAgICAgIHwgMTAgKysrKysrKysr KwogLi4uZnktc2VsZWN0aW9uLWxpc3QtbGV2ZWwtY3Jhc2gtZXhwZWN0ZWQudHh0IHwgIDIgKysK IC4uLi9tb2RpZnktc2VsZWN0aW9uLWxpc3QtbGV2ZWwtY3Jhc2guaHRtbCAgICB8IDEzICsrKysr KysrKysrKwogU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nICAgICAgICAgICAgICAgICAgICAgIHwg MjAgKysrKysrKysrKysrKysrKysrKwogLi4uL2VkaXRpbmcvTW9kaWZ5U2VsZWN0aW9uTGlzdExl dmVsLmNwcCAgICAgIHwgIDIgKy0KIDUgZmlsZXMgY2hhbmdlZCwgNDYgaW5zZXJ0aW9ucygrKSwg MSBkZWxldGlvbigtKQogY3JlYXRlIG1vZGUgMTAwNjQ0IExheW91dFRlc3RzL2Zhc3QvZWRpdGlu Zy9tb2RpZnktc2VsZWN0aW9uLWxpc3QtbGV2ZWwtY3Jhc2gtZXhwZWN0ZWQudHh0CiBjcmVhdGUg bW9kZSAxMDA2NDQgTGF5b3V0VGVzdHMvZmFzdC9lZGl0aW5nL21vZGlmeS1zZWxlY3Rpb24tbGlz dC1sZXZlbC1jcmFzaC5odG1sCgpkaWZmIC0tZ2l0IGEvTGF5b3V0VGVzdHMvQ2hhbmdlTG9nIGIv TGF5b3V0VGVzdHMvQ2hhbmdlTG9nCmluZGV4IGE5ODQ4YjUzYmMuLjAyNDc3MzdjNzQgMTAwNjQ0 Ci0tLSBhL0xheW91dFRlc3RzL0NoYW5nZUxvZworKysgYi9MYXlvdXRUZXN0cy9DaGFuZ2VMb2cK QEAgLTEsMyArMSwxMyBAQAorMjAyMS0wMi0xOCAgRnJlZGVyaWMgV2FuZyAgPGZ3YW5nQGlnYWxp YS5jb20+CisKKyAgICAgICAgTnVsbHB0ciBjcmFzaCBpbiBNb2RpZnlTZWxlY3Rpb25MaXN0TGV2 ZWxDb21tYW5kOjphcHBlbmRTaWJsaW5nTm9kZVJhbmdlCisgICAgICAgIGh0dHBzOi8vYnVncy53 ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0yMjE2NTAKKworICAgICAgICBSZXZpZXdlZCBieSBO T0JPRFkgKE9PUFMhKS4KKworICAgICAgICAqIGZhc3QvZWRpdGluZy9tb2RpZnktc2VsZWN0aW9u LWxpc3QtbGV2ZWwtY3Jhc2gtZXhwZWN0ZWQudHh0OiBBZGRlZC4KKyAgICAgICAgKiBmYXN0L2Vk aXRpbmcvbW9kaWZ5LXNlbGVjdGlvbi1saXN0LWxldmVsLWNyYXNoLmh0bWw6IEFkZGVkLgorCiAy MDIxLTAyLTE1ICBGcmVkZXJpYyBXYW5nICA8ZndhbmdAaWdhbGlhLmNvbT4KIAogICAgICAgICBD cmFzaCBpbiBSZXRhaW5QdHI8Q0dJbWFnZSo+OjpSZXRhaW5QdHIgdmlhIEltYWdlQnVmZmVyQ0dC YWNrZW5kOjp0b0NGRGF0YQpkaWZmIC0tZ2l0IGEvTGF5b3V0VGVzdHMvZmFzdC9lZGl0aW5nL21v ZGlmeS1zZWxlY3Rpb24tbGlzdC1sZXZlbC1jcmFzaC1leHBlY3RlZC50eHQgYi9MYXlvdXRUZXN0 cy9mYXN0L2VkaXRpbmcvbW9kaWZ5LXNlbGVjdGlvbi1saXN0LWxldmVsLWNyYXNoLWV4cGVjdGVk LnR4dApuZXcgZmlsZSBtb2RlIDEwMDY0NAppbmRleCAwMDAwMDAwMDAwLi4wMTYzYWJjOWU0Ci0t LSAvZGV2L251bGwKKysrIGIvTGF5b3V0VGVzdHMvZmFzdC9lZGl0aW5nL21vZGlmeS1zZWxlY3Rp b24tbGlzdC1sZXZlbC1jcmFzaC1leHBlY3RlZC50eHQKQEAgLTAsMCArMSwyIEBACitUaGlzIHRl c3QgcGFzc2VzIGlmIGl0IGRvZXMgbm90IGNyYXNoLgorCmRpZmYgLS1naXQgYS9MYXlvdXRUZXN0 cy9mYXN0L2VkaXRpbmcvbW9kaWZ5LXNlbGVjdGlvbi1saXN0LWxldmVsLWNyYXNoLmh0bWwgYi9M YXlvdXRUZXN0cy9mYXN0L2VkaXRpbmcvbW9kaWZ5LXNlbGVjdGlvbi1saXN0LWxldmVsLWNyYXNo Lmh0bWwKbmV3IGZpbGUgbW9kZSAxMDA2NDQKaW5kZXggMDAwMDAwMDAwMC4uOWQ5YTdiYTlmZQot LS0gL2Rldi9udWxsCisrKyBiL0xheW91dFRlc3RzL2Zhc3QvZWRpdGluZy9tb2RpZnktc2VsZWN0 aW9uLWxpc3QtbGV2ZWwtY3Jhc2guaHRtbApAQCAtMCwwICsxLDEzIEBACis8c2NyaXB0PgorICBp ZiAod2luZG93LnRlc3RSdW5uZXIpCisgICAgICB0ZXN0UnVubmVyLmR1bXBBc1RleHQoKTsKKyAg b25sb2FkID0gKCkgPT4geworICAgIGRvY3VtZW50LmV4ZWNDb21tYW5kKCdTZWxlY3RBbGwnKTsK KyAgICBkb2N1bWVudC5kZXNpZ25Nb2RlID0gJ29uJzsKKyAgICBkb2N1bWVudC5leGVjQ29tbWFu ZCgnSW5zZXJ0TmVzdGVkT3JkZXJlZExpc3QnKTsKKyAgfTsKKzwvc2NyaXB0PgorPHNsb3Q+Cisg IDxsaT5UaGlzIHRlc3QgcGFzc2VzIGlmIGl0IGRvZXMgbm90IGNyYXNoLjwvbGk+Cis8L3Nsb3Q+ Cis8b2w+PC9vbD4KZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZyBiL1NvdXJj ZS9XZWJDb3JlL0NoYW5nZUxvZwppbmRleCBkNGZiOGU3ZmI4Li40NTU2OGM3ZWZjIDEwMDY0NAot LS0gYS9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKKysrIGIvU291cmNlL1dlYkNvcmUvQ2hhbmdl TG9nCkBAIC0xLDMgKzEsMjMgQEAKKzIwMjEtMDItMTggIEZyZWRlcmljIFdhbmcgIDxmd2FuZ0Bp Z2FsaWEuY29tPgorCisgICAgICAgIE51bGxwdHIgY3Jhc2ggaW4gTW9kaWZ5U2VsZWN0aW9uTGlz dExldmVsQ29tbWFuZDo6YXBwZW5kU2libGluZ05vZGVSYW5nZQorICAgICAgICBodHRwczovL2J1 Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MjIxNjUwCisKKyAgICAgICAgUmV2aWV3ZWQg YnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgZ2V0U3RhcnRFbmRMaXN0Q2hpbGRyZW4gcmVs aWVzIG9uIHRoZSByZW5kZXIgdHJlZSB0byBtb3ZlIHRoZSAiZW5kIiBub2RlIHRvIHRoZSBuZXh0 IHNpYmxpbmcsCisgICAgICAgIGJ1dCB0aGlzIGRvZXMgbm90IG5lY2Vzc2FyaWx5IGNvcnJlc3Bv bmQgdG8gYSBzaWJsaW5nIG9mIHRoZSAic3RhcnQiIG5vZGUgaW4gdGhlIERPTSB0cmVlLgorICAg ICAgICBUaGlzIGJyZWFrcyB0aGUgYXNzdW1wdGlvbiBvZiBNb2RpZnlTZWxlY3Rpb25MaXN0TGV2 ZWxDb21tYW5kOjphcHBlbmRTaWJsaW5nTm9kZVJhbmdlIHRoYXQKKyAgICAgICAgdGhlICJzdGFy dCIgYW5kICJlbmQiIG5vZGVzIGFyZSBzaWJsaW5ncyAoaW4gdGhhdCBvcmRlciksIGNhdXNpbmcg YSBudWxsLXBvaW50ZXIgZGVyZWZlcmVuY2UuCisgICAgICAgIFRoaXMgcGF0Y2ggZml4ZXMgdGhl IGlzc3VlIGJ5IGVuc3VyaW5nIHRoYXQgZ2V0U3RhcnRFbmRMaXN0Q2hpbGRyZW4gZG9lcyBub3Qg dHJ5IHRvIGNoYW5nZQorICAgICAgICB0aGUgImVuZCIgbm9kZSBpZiBpdCBpcyBub3QgYSBzaWJs aW5nIG9mIHRoZSAic3RhcnQiIG9uZS4KKworICAgICAgICBUZXN0OiBmYXN0L2VkaXRpbmcvbW9k aWZ5LXNlbGVjdGlvbi1saXN0LWxldmVsLWNyYXNoLmh0bWwKKworICAgICAgICAqIGVkaXRpbmcv TW9kaWZ5U2VsZWN0aW9uTGlzdExldmVsLmNwcDoKKyAgICAgICAgKFdlYkNvcmU6OmdldFN0YXJ0 RW5kTGlzdENoaWxkcmVuKTogRG9uJ3QgY2hhbmdlIHRoZSBlbmQgbm9kZSBpZiByLT5ub2RlKCkg aXMgYSBzaWJsaW5nIG9mCisgICAgICAgIHN0YXJ0Q2hpbGRMaXN0LgorCiAyMDIxLTAyLTE1ICBG cmVkZXJpYyBXYW5nICA8ZndhbmdAaWdhbGlhLmNvbT4KIAogICAgICAgICBDcmFzaCBpbiBSZXRh aW5QdHI8Q0dJbWFnZSo+OjpSZXRhaW5QdHIgdmlhIEltYWdlQnVmZmVyQ0dCYWNrZW5kOjp0b0NG RGF0YQpkaWZmIC0tZ2l0IGEvU291cmNlL1dlYkNvcmUvZWRpdGluZy9Nb2RpZnlTZWxlY3Rpb25M aXN0TGV2ZWwuY3BwIGIvU291cmNlL1dlYkNvcmUvZWRpdGluZy9Nb2RpZnlTZWxlY3Rpb25MaXN0 TGV2ZWwuY3BwCmluZGV4IDkyMmU0NDg4MTIuLmNjYjk4YmNlZWYgMTAwNjQ0Ci0tLSBhL1NvdXJj ZS9XZWJDb3JlL2VkaXRpbmcvTW9kaWZ5U2VsZWN0aW9uTGlzdExldmVsLmNwcAorKysgYi9Tb3Vy Y2UvV2ViQ29yZS9lZGl0aW5nL01vZGlmeVNlbGVjdGlvbkxpc3RMZXZlbC5jcHAKQEAgLTgwLDcg KzgwLDcgQEAgc3RhdGljIGJvb2wgZ2V0U3RhcnRFbmRMaXN0Q2hpbGRyZW4oY29uc3QgVmlzaWJs ZVNlbGVjdGlvbiYgc2VsZWN0aW9uLCBOb2RlKiYgc3QKICAgICAvLyBpZiB0aGUgc2VsZWN0aW9u IGVuZHMgb24gYSBsaXN0IGl0ZW0gd2l0aCBhIHN1Ymxpc3QsIGluY2x1ZGUgdGhlIGVudGlyZSBz dWJsaXN0CiAgICAgaWYgKGVuZExpc3RDaGlsZC0+cmVuZGVyZXIoKS0+aXNMaXN0SXRlbSgpKSB7 CiAgICAgICAgIFJlbmRlck9iamVjdCogciA9IGVuZExpc3RDaGlsZC0+cmVuZGVyZXIoKS0+bmV4 dFNpYmxpbmcoKTsKLSAgICAgICAgaWYgKHIgJiYgaXNMaXN0SFRNTEVsZW1lbnQoci0+bm9kZSgp KSkKKyAgICAgICAgaWYgKHIgJiYgaXNMaXN0SFRNTEVsZW1lbnQoci0+bm9kZSgpKSAmJiByLT5u b2RlKCktPnBhcmVudE5vZGUoKSA9PSBzdGFydExpc3RDaGlsZC0+cGFyZW50Tm9kZSgpKQogICAg ICAgICAgICAgZW5kTGlzdENoaWxkID0gci0+bm9kZSgpOwogICAgIH0KIAotLSAKMi4yNS4xCgo=