219615 2020-12-07 14:11:22 -0800 Make TextRun::subRun stricter 2021-02-05 21:30:51 -0800 1 1 1 Unclassified WebKit Text WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED https://bugs.webkit.org/show_bug.cgi?id=218488 InRadar P2 Normal --- 1 rbuis rbuis darin ddkilzer koivisto mmaxfield rniwa simon.fraser webkit-bug-importer zalan oldest_to_newest 1712613 0 rbuis 2020-12-07 14:11:22 -0800 Make TextRun::subRun stricter, besides the start offset being less than the run length, the sub run start offset plus sub run length should not exceed the overall run length. 1712614 1 415584 rbuis 2020-12-07 14:14:02 -0800 Created attachment 415584 Patch 1712627 2 415584 darin 2020-12-07 14:52:03 -0800 Comment on attachment 415584 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=415584&action=review > Source/WebCore/platform/graphics/TextRun.h:69 > - ASSERT_WITH_SECURITY_IMPLICATION(startOffset < m_text.length()); > + RELEASE_ASSERT_WITH_SECURITY_IMPLICATION(startOffset < m_text.length() && (startOffset + length) <= m_text.length()); The old assertion is redundant. We already have those assertions in data8 and data16. So we could consider only the new assertion, and not repeating the old assertion. Bold to change this to RELEASE_ASSERT, but strange to change this, and not data8, data16, operator[] or other idioms that are not member functions. Not sure what the rationale is. 1712685 3 415584 rbuis 2020-12-07 21:56:17 -0800 Comment on attachment 415584 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=415584&action=review >> Source/WebCore/platform/graphics/TextRun.h:69 >> + RELEASE_ASSERT_WITH_SECURITY_IMPLICATION(startOffset < m_text.length() && (startOffset + length) <= m_text.length()); > > The old assertion is redundant. We already have those assertions in data8 and data16. So we could consider only the new assertion, and not repeating the old assertion. > > Bold to change this to RELEASE_ASSERT, but strange to change this, and not data8, data16, operator[] or other idioms that are not member functions. Not sure what the rationale is. I believe there will be a style warning when keeping ASSERT_WITH_SECURITY_IMPLICATION. 1712771 4 415636 rbuis 2020-12-08 06:39:05 -0800 Created attachment 415636 Patch 1712773 5 415584 rbuis 2020-12-08 06:40:45 -0800 Comment on attachment 415584 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=415584&action=review >>> Source/WebCore/platform/graphics/TextRun.h:69 >>> + RELEASE_ASSERT_WITH_SECURITY_IMPLICATION(startOffset < m_text.length() && (startOffset + length) <= m_text.length()); >> >> The old assertion is redundant. We already have those assertions in data8 and data16. So we could consider only the new assertion, and not repeating the old assertion. >> >> Bold to change this to RELEASE_ASSERT, but strange to change this, and not data8, data16, operator[] or other idioms that are not member functions. Not sure what the rationale is. > > I believe there will be a style warning when keeping ASSERT_WITH_SECURITY_IMPLICATION. I removed the old assertion and brought back the ASSERT_WITH_SEC. FWIW this is the style error: ERROR: Source/WebCore/platform/graphics/TextRun.h:69: Please replace ASSERT_WITH_SECURITY_IMPLICATION() with RELEASE_ASSERT_WITH_SECURITY_IMPLICATION(). [security/assertion] [5] 1712809 6 415584 darin 2020-12-08 09:07:53 -0800 Comment on attachment 415584 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=415584&action=review >>>> Source/WebCore/platform/graphics/TextRun.h:69 >>>> + RELEASE_ASSERT_WITH_SECURITY_IMPLICATION(startOffset < m_text.length() && (startOffset + length) <= m_text.length()); >>> >>> The old assertion is redundant. We already have those assertions in data8 and data16. So we could consider only the new assertion, and not repeating the old assertion. >>> >>> Bold to change this to RELEASE_ASSERT, but strange to change this, and not data8, data16, operator[] or other idioms that are not member functions. Not sure what the rationale is. >> >> I believe there will be a style warning when keeping ASSERT_WITH_SECURITY_IMPLICATION. > > I removed the old assertion and brought back the ASSERT_WITH_SEC. FWIW this is the style error: > ERROR: Source/WebCore/platform/graphics/TextRun.h:69: Please replace ASSERT_WITH_SECURITY_IMPLICATION() with RELEASE_ASSERT_WITH_SECURITY_IMPLICATION(). [security/assertion] [5] I think David Kilzer created that style warning. Might be worth consulting with him. 1712812 7 ews-feeder 2020-12-08 09:22:14 -0800 Committed r270541: <https://trac.webkit.org/changeset/270541> All reviewed patches have been landed. Closing bug and clearing flags on attachment 415636. 1712813 8 webkit-bug-importer 2020-12-08 09:23:17 -0800 <rdar://problem/72095633> 1712814 9 rbuis 2020-12-08 09:25:33 -0800 @ddkilzer: is the style check maybe too strong for this kind of patch? I chose to ignore it: ERROR: Source/WebCore/platform/graphics/TextRun.h:69: Please replace ASSERT_WITH_SECURITY_IMPLICATION() with RELEASE_ASSERT_WITH_SECURITY_IMPLICATION(). [security/assertion] [5] Is it maybe better to have it as a warning or advice? 1726127 10 ddkilzer 2021-02-05 09:40:56 -0800 (In reply to Rob Buis from comment #9) > @ddkilzer: is the style check maybe too strong for this kind of patch? I > chose to ignore it: > ERROR: Source/WebCore/platform/graphics/TextRun.h:69: Please replace > ASSERT_WITH_SECURITY_IMPLICATION() with > RELEASE_ASSERT_WITH_SECURITY_IMPLICATION(). [security/assertion] [5] > > Is it maybe better to have it as a warning or advice? We want to decrease use of ASSERT_WITH_SECURITY_IMPLICATION() over time because if the security issue is bad enough to assert in a Debug build, then we are likely hiding the issue in Release builds. Notice that git/commit-queue doesn't enforce this, so you were still able to land the patch. The point is that this is to make you think twice about using ASSERT_WITH_SECURITY_IMPLICATION() going forward. 1726361 11 rniwa 2021-02-05 21:30:51 -0800 We probably can't use release assert here since TextRun needs to be lightweight & fast. Might be worth experimenting with trying to use release assert & run all the benchmarks we have to see if it negatively impacts it or not. 415584 2020-12-07 14:14:02 -0800 2020-12-08 06:38:59 -0800 Patch bug-219615-20201207231359.patch text/plain 1468 rbuis U3VidmVyc2lvbiBSZXZpc2lvbjogMjcwNDg2CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggY2JiNzdjYjNjM2I2NWNm ZTg4MjU1M2VkMTRhM2NmZDhlYjI1YzQ2MC4uODI3ZDZhMWVlNjk4MWQ5OWRiZDVlYmYwYTI5MjYw MjMxYTI5MzBkNyAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDE3IEBACisyMDIwLTEyLTA3ICBSb2Ig QnVpcyAgPHJidWlzQGlnYWxpYS5jb20+CisKKyAgICAgICAgTWFrZSBUZXh0UnVuOjpzdWJSdW4g c3RyaWN0ZXIKKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lk PTIxOTYxNQorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAg IE1ha2UgVGV4dFJ1bjo6c3ViUnVuIHN0cmljdGVyLCBiZXNpZGVzIHRoZSBzdGFydCBvZmZzZXQg YmVpbmcgbGVzcyB0aGFuIHRoZSBydW4KKyAgICAgICAgbGVuZ3RoLCB0aGUgc3ViIHJ1biBzdGFy dCBvZmZzZXQgcGx1cyBzdWIgcnVuIGxlbmd0aCBzaG91bGQgbm90IGV4Y2VlZCB0aGUKKyAgICAg ICAgb3ZlcmFsbCBydW4gbGVuZ3RoLgorCisgICAgICAgICogcGxhdGZvcm0vZ3JhcGhpY3MvVGV4 dFJ1bi5oOgorICAgICAgICAoV2ViQ29yZTo6VGV4dFJ1bjo6c3ViUnVuIGNvbnN0KToKKwogMjAy MC0xMi0wNiAgWW91ZW5uIEZhYmxldCAgPHlvdWVubkBhcHBsZS5jb20+CiAKICAgICAgICAgQWRk IHN1cHBvcnQgZm9yIFJUQ1J0cFNlbmRlcjo6c2V0U3RyZWFtcwpkaWZmIC0tZ2l0IGEvU291cmNl L1dlYkNvcmUvcGxhdGZvcm0vZ3JhcGhpY3MvVGV4dFJ1bi5oIGIvU291cmNlL1dlYkNvcmUvcGxh dGZvcm0vZ3JhcGhpY3MvVGV4dFJ1bi5oCmluZGV4IGVjYmM1ZWVhOWI5ZDFlM2UzYzQ1MTNhZjU4 MDY3ZTU3ZTgyYTVkYTcuLjQ5NWYzZTI4ZTk2MGNiZGZiNDc4YzNjMTk3NjVhNmZmZWFlZjM2NWMg MTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJDb3JlL3BsYXRmb3JtL2dyYXBoaWNzL1RleHRSdW4uaAor KysgYi9Tb3VyY2UvV2ViQ29yZS9wbGF0Zm9ybS9ncmFwaGljcy9UZXh0UnVuLmgKQEAgLTY2LDcg KzY2LDcgQEAgcHVibGljOgogCiAgICAgVGV4dFJ1biBzdWJSdW4odW5zaWduZWQgc3RhcnRPZmZz ZXQsIHVuc2lnbmVkIGxlbmd0aCkgY29uc3QKICAgICB7Ci0gICAgICAgIEFTU0VSVF9XSVRIX1NF Q1VSSVRZX0lNUExJQ0FUSU9OKHN0YXJ0T2Zmc2V0IDwgbV90ZXh0Lmxlbmd0aCgpKTsKKyAgICAg ICAgUkVMRUFTRV9BU1NFUlRfV0lUSF9TRUNVUklUWV9JTVBMSUNBVElPTihzdGFydE9mZnNldCA8 IG1fdGV4dC5sZW5ndGgoKSAmJiAoc3RhcnRPZmZzZXQgKyBsZW5ndGgpIDw9IG1fdGV4dC5sZW5n dGgoKSk7CiAKICAgICAgICAgYXV0byByZXN1bHQgeyAqdGhpcyB9OwogCg== 415636 2020-12-08 06:39:05 -0800 2020-12-08 09:22:15 -0800 Patch bug-219615-20201208153903.patch text/plain 1431 rbuis U3VidmVyc2lvbiBSZXZpc2lvbjogMjcwNTM1CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggYjEzNDk4OWQ0M2U2ZTY1 N2Y3NTRjYmQ2YTEwYTRkODA5MGJjYWYyMi4uN2YyMmM1ODJjYzE2NWI3NTNjNWViNTcyMDczNDhh MTU2ZmU5NWYzZiAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDE3IEBACisyMDIwLTEyLTA4ICBSb2Ig QnVpcyAgPHJidWlzQGlnYWxpYS5jb20+CisKKyAgICAgICAgTWFrZSBUZXh0UnVuOjpzdWJSdW4g c3RyaWN0ZXIKKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lk PTIxOTYxNQorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAg IE1ha2UgVGV4dFJ1bjo6c3ViUnVuIHN0cmljdGVyLCBiZXNpZGVzIHRoZSBzdGFydCBvZmZzZXQg YmVpbmcgbGVzcyB0aGFuIHRoZSBydW4KKyAgICAgICAgbGVuZ3RoLCB0aGUgc3ViIHJ1biBzdGFy dCBvZmZzZXQgcGx1cyBzdWIgcnVuIGxlbmd0aCBzaG91bGQgbm90IGV4Y2VlZCB0aGUKKyAgICAg ICAgb3ZlcmFsbCBydW4gbGVuZ3RoLgorCisgICAgICAgICogcGxhdGZvcm0vZ3JhcGhpY3MvVGV4 dFJ1bi5oOgorICAgICAgICAoV2ViQ29yZTo6VGV4dFJ1bjo6c3ViUnVuIGNvbnN0KToKKwogMjAy MC0xMi0wOCAgWmFsYW4gQnVqdGFzICA8emFsYW5AYXBwbGUuY29tPgogCiAgICAgICAgIFtMRkNd W0lGQ10gVHJpbW1hYmxlIGNvbnRlbnQgY2FuIGluY2x1ZGUgPHdicj4KZGlmZiAtLWdpdCBhL1Nv dXJjZS9XZWJDb3JlL3BsYXRmb3JtL2dyYXBoaWNzL1RleHRSdW4uaCBiL1NvdXJjZS9XZWJDb3Jl L3BsYXRmb3JtL2dyYXBoaWNzL1RleHRSdW4uaAppbmRleCBlY2JjNWVlYTliOWQxZTNlM2M0NTEz YWY1ODA2N2U1N2U4MmE1ZGE3Li4xZDQ3ZDY1ZDY4MjQ2MjZlMTU5Mjg4MDBmNDY5N2MyNzZjYjI3 NjM0IDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29yZS9wbGF0Zm9ybS9ncmFwaGljcy9UZXh0UnVu LmgKKysrIGIvU291cmNlL1dlYkNvcmUvcGxhdGZvcm0vZ3JhcGhpY3MvVGV4dFJ1bi5oCkBAIC02 Niw3ICs2Niw3IEBAIHB1YmxpYzoKIAogICAgIFRleHRSdW4gc3ViUnVuKHVuc2lnbmVkIHN0YXJ0 T2Zmc2V0LCB1bnNpZ25lZCBsZW5ndGgpIGNvbnN0CiAgICAgewotICAgICAgICBBU1NFUlRfV0lU SF9TRUNVUklUWV9JTVBMSUNBVElPTihzdGFydE9mZnNldCA8IG1fdGV4dC5sZW5ndGgoKSk7Cisg ICAgICAgIEFTU0VSVF9XSVRIX1NFQ1VSSVRZX0lNUExJQ0FUSU9OKChzdGFydE9mZnNldCArIGxl bmd0aCkgPD0gbV90ZXh0Lmxlbmd0aCgpKTsKIAogICAgICAgICBhdXRvIHJlc3VsdCB7ICp0aGlz IH07CiAK