200023 2019-07-22 18:40:26 -0700 Fix crashes in ScrollingStateNode::insertChild() 2019-07-23 19:42:54 -0700 1 1 1 Unclassified WebKit New Bugs WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED InRadar P2 Normal --- 1 simon.fraser simon.fraser cmarcelo darin ews-watchlist fred.wang jamesr koivisto luiz simon.fraser thorton tonikitoo webkit-bug-importer oldest_to_newest 1554959 0 simon.fraser 2019-07-22 18:40:26 -0700 Fix crashes in ScrollingStateNode::insertChild() 1554960 1 374662 simon.fraser 2019-07-22 18:41:50 -0700 Created attachment 374662 Patch 1554961 2 simon.fraser 2019-07-22 18:41:52 -0700 <rdar://problem/53265378> 1554965 3 374662 darin 2019-07-22 18:52:37 -0700 Comment on attachment 374662 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=374662&action=review > Source/WebCore/ChangeLog:13 > + Crash data suggest that ScrollingStateNode::insertChild() can be passed an index that > + is larger than the size of the vector, causing crashes. > + > + Fix defensively by falling back to append() if the passed index is equal to or larger > + than the size of the children vector. Is there a reason we don’t do this inside the insert function instead? Are there other call sites where we want the stricter behavior? What about asserting the index is valid, even if we prevent the crash in such cases? That could help us some day understand how this happens if we reproduce in a build with assertions on. 1555113 4 simon.fraser 2019-07-23 12:50:46 -0700 https://trac.webkit.org/r247734 1555284 5 374662 darin 2019-07-23 19:12:46 -0700 Comment on attachment 374662 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=374662&action=review > Source/WebCore/page/scrolling/ScrollingStateNode.cpp:119 > + if (index >= m_children->size()) The patch you landed uses ">" rather than ">=". Why is that? 1555292 6 simon.fraser 2019-07-23 19:42:54 -0700 (In reply to Darin Adler from comment #5) > Comment on attachment 374662 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=374662&action=review > > > Source/WebCore/page/scrolling/ScrollingStateNode.cpp:119 > > + if (index >= m_children->size()) > > The patch you landed uses ">" rather than ">=". Why is that? Because Vector::insert() allows inserting at index = size() - it has: ASSERT_WITH_SECURITY_IMPLICATION(position <= size()); 374662 2019-07-22 18:41:50 -0700 2019-07-22 18:52:37 -0700 Patch bug-200023-20190722184150.patch text/plain 1793 simon.fraser U3VidmVyc2lvbiBSZXZpc2lvbjogMjQ3NjkyCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggZGE0NjZiY2IxODA3ZWQy YmQ3MjA0ODIzNDVmNzQ2Njc5ZWZlOWFiMy4uY2FhZjUyZDYzOWM4NTYzMTVmNWExODNmYTQ0ZTFm NjcwMDQ0YjFhNSAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDIwIEBACisyMDE5LTA3LTIyICBTaW1v biBGcmFzZXIgIDxzaW1vbi5mcmFzZXJAYXBwbGUuY29tPgorCisgICAgICAgIEZpeCBjcmFzaGVz IGluIFNjcm9sbGluZ1N0YXRlTm9kZTo6aW5zZXJ0Q2hpbGQoKQorICAgICAgICBodHRwczovL2J1 Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MjAwMDIzCisgICAgICAgIHJkYXI6Ly9wcm9i bGVtLzUzMjY1Mzc4CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAg ICAgICAgQ3Jhc2ggZGF0YSBzdWdnZXN0IHRoYXQgU2Nyb2xsaW5nU3RhdGVOb2RlOjppbnNlcnRD aGlsZCgpIGNhbiBiZSBwYXNzZWQgYW4gaW5kZXggdGhhdAorICAgICAgICBpcyBsYXJnZXIgdGhh biB0aGUgc2l6ZSBvZiB0aGUgdmVjdG9yLCBjYXVzaW5nIGNyYXNoZXMuCisKKyAgICAgICAgRml4 IGRlZmVuc2l2ZWx5IGJ5IGZhbGxpbmcgYmFjayB0byBhcHBlbmQoKSBpZiB0aGUgcGFzc2VkIGlu ZGV4IGlzIGVxdWFsIHRvIG9yIGxhcmdlcgorICAgICAgICB0aGFuIHRoZSBzaXplIG9mIHRoZSBj aGlsZHJlbiB2ZWN0b3IuCisKKyAgICAgICAgKiBwYWdlL3Njcm9sbGluZy9TY3JvbGxpbmdTdGF0 ZU5vZGUuY3BwOgorICAgICAgICAoV2ViQ29yZTo6U2Nyb2xsaW5nU3RhdGVOb2RlOjppbnNlcnRD aGlsZCk6CisKIDIwMTktMDctMjIgIFNpbW9uIEZyYXNlciAgPHNpbW9uLmZyYXNlckBhcHBsZS5j b20+CiAKICAgICAgICAgTWFrZSBzb21lIGNvbnN0cnVjdG9ycyBleHBsaWNpdApkaWZmIC0tZ2l0 IGEvU291cmNlL1dlYkNvcmUvcGFnZS9zY3JvbGxpbmcvU2Nyb2xsaW5nU3RhdGVOb2RlLmNwcCBi L1NvdXJjZS9XZWJDb3JlL3BhZ2Uvc2Nyb2xsaW5nL1Njcm9sbGluZ1N0YXRlTm9kZS5jcHAKaW5k ZXggMmU2ZDRkM2Y4MmI5ZjI0N2UzNDUzMzg0NTAxYThhYmU2NWI5MWMwNi4uNGY0Mjk1MWIwMzA0 MzA0OGE1ZTliZWIyZGFhYjNmZDc4YWFkMzMyYSAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUv cGFnZS9zY3JvbGxpbmcvU2Nyb2xsaW5nU3RhdGVOb2RlLmNwcAorKysgYi9Tb3VyY2UvV2ViQ29y ZS9wYWdlL3Njcm9sbGluZy9TY3JvbGxpbmdTdGF0ZU5vZGUuY3BwCkBAIC0xMTYsNyArMTE2LDEx IEBAIHZvaWQgU2Nyb2xsaW5nU3RhdGVOb2RlOjppbnNlcnRDaGlsZChSZWY8U2Nyb2xsaW5nU3Rh dGVOb2RlPiYmIGNoaWxkTm9kZSwgc2l6ZV90CiAgICAgICAgIG1fY2hpbGRyZW4gPSBzdGQ6Om1h a2VfdW5pcXVlPFZlY3RvcjxSZWZQdHI8U2Nyb2xsaW5nU3RhdGVOb2RlPj4+KCk7CiAgICAgfQog Ci0gICAgbV9jaGlsZHJlbi0+aW5zZXJ0KGluZGV4LCBXVEZNb3ZlKGNoaWxkTm9kZSkpOworICAg IGlmIChpbmRleCA+PSBtX2NoaWxkcmVuLT5zaXplKCkpCisgICAgICAgIG1fY2hpbGRyZW4tPmFw cGVuZChXVEZNb3ZlKGNoaWxkTm9kZSkpOworICAgIGVsc2UKKyAgICAgICAgbV9jaGlsZHJlbi0+ aW5zZXJ0KGluZGV4LCBXVEZNb3ZlKGNoaWxkTm9kZSkpOworICAgIAogICAgIHNldFByb3BlcnR5 Q2hhbmdlZChDaGlsZE5vZGVzKTsKIH0KIAo=