202155 2019-09-24 12:25:41 -0700 Address static analysis warning in UTextProviderLatin1.cpp: Array access results in a null pointer dereference 2019-09-25 23:41:23 -0700 1 1 1 Unclassified WebKit Web Template Framework WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED InRadar P2 Normal --- 1 krollin krollin benjamin cdumez cmarcelo commit-queue darin dbates ews-watchlist ggaren mmaxfield webkit-bug-importer youennf oldest_to_newest 1573745 0 krollin 2019-09-24 12:25:41 -0700 Xcode's static analysis reports: UTextProviderLatin1.cpp -> darin, myles /Volumes/Data/dev/webkit/branches/static-analysis/OpenSource/Source/WTF/wtf/text/icu/UTextProviderLatin1.cpp:185:22: warning: Array access (from variable 'dest') results in a null pointer dereference dest[length] = 0; ~~~~ ^ This seems to be a false positive. Earlier checks involving "destCapacity" will exit the function if "destCapacity" is negative, or if "dest" is NULL and "destCapacity" is positive. Therefore, if "dest" is NULL, we will only enter if "destCapacity" is exactly zero. Further, there is a test just above the cited assignment that will never be true if "destCapacity" is zero. Therefore, the cited assignment will not be executed if "dest" is NULL. Address this by convincing the static analyzer of this with an ASSERT statement. 1573746 1 webkit-bug-importer 2019-09-24 12:26:02 -0700 <rdar://problem/55672422> 1573747 2 379466 krollin 2019-09-24 12:28:14 -0700 Created attachment 379466 Patch 1573795 3 379466 ggaren 2019-09-24 13:32:23 -0700 Comment on attachment 379466 Patch Is this really a false positive, or does it indicate that the preceding "if (destCapacity > 0 && !dest)" clause is incorrect? (It seems like the early check for "|| (!dest && destCapacity > 0)" should make the check for "if (destCapacity > 0 && !dest)" impossible. 1573945 4 krollin 2019-09-24 21:44:16 -0700 Huh. Going over email that's been sitting in my Inbox for a while, I reviewed a Coverity Scan that had been sent to me, and saw that it says exactly the same thing: 176 if (destCapacity > 0 && !dest) { CID 190890: Control flow issues (DEADCODE) Execution cannot reach this statement: "trimmedLength = static_cast...". 177 int32_t trimmedLength = static_cast<int32_t>(length); 1574337 5 krollin 2019-09-25 20:26:47 -0700 I've gone over the function in order to try to understand it. From what you've mentioned and from the Coverity error message, it seems like the if-block protected by the "if (destCapacity > 0 && !dest)" will never be executed. Since this is where the actual characters are being copied, it seems like this function never performed one of its primary functions, which is to extract a sub-string. Indeed, if the if-block *were* executed, the call to StringImpl::copyCharacters would fail since we'd be passing a NULL dest to it. Examination of the code indicates to me that it's true that uTextLatin1Extract will in fact never be called. That function is part of a family of latin-1 related functions. That family of functions is only used in the context of the ICU function ubrk_setUText, which won't be extracting text. So it looks like uTextLatin1Extract was implemented in the name of completeness only. Getting back to fixing the function, I think that that if-conditional should instead be "if (destCapacity > 0 && dest)". Or even just "if (dest)". I've made that change, along with adding an "if (dest)" in front of the the "dest[length] = 0" that triggered this whole task. And I took out the ASSERT that I'd added in order to quiet the static analyzer. With those changes, the code compiles and analyzes cleanly. However, it's not clear if it actually works. It looks like it should work. But it looks like this function was never really used. And also never really tested. Some unit testing should probably be added for UTextProviderLatin1, UTextProviderUTF16, and UTextProvider. But since doing that could generate a whole series of follow-on tasks, I'd like that effort to be part of its own bug and leave this one to addressing the static-analyzer error. Perhaps this whole function should just be replaced and implemented along the lines of its UTF16 analog? I'll just upload a patch that keeps the existing function largely intact, but could rip it out if people think that's the best approach. static int32_t uTextUTF16ContextAwareExtract(UText*, int64_t, int64_t, UChar*, int32_t, UErrorCode* errorCode) { // In the present context, this text provider is used only with ICU functions // that do not perform an extract operation. ASSERT_NOT_REACHED(); *errorCode = U_UNSUPPORTED_ERROR; return 0; } 1574338 6 379617 krollin 2019-09-25 20:27:25 -0700 Created attachment 379617 Patch 1574339 7 379617 ggaren 2019-09-25 20:33:15 -0700 Comment on attachment 379617 Patch r=me 1574368 8 379617 commit-queue 2019-09-25 23:41:22 -0700 Comment on attachment 379617 Patch Clearing flags on attachment: 379617 Committed r250380: <https://trac.webkit.org/changeset/250380> 1574369 9 commit-queue 2019-09-25 23:41:23 -0700 All reviewed patches have been landed. Closing bug. 379466 2019-09-24 12:28:14 -0700 2019-09-25 20:27:23 -0700 Patch bug-202155-20190924122814.patch text/plain 2315 krollin U3VidmVyc2lvbiBSZXZpc2lvbjogMjUwMjQzCmRpZmYgLS1naXQgYS9Tb3VyY2UvV1RGL0NoYW5n ZUxvZyBiL1NvdXJjZS9XVEYvQ2hhbmdlTG9nCmluZGV4IDIwZmNlNWExNjY3N2UwZWE3MjAyNzQ2 NDVkYWMyNjNmNmZkMTYxMTcuLmU2Y2VmYjdiMzQxNTk0MzBmZmE1MzY5OTM1MDBiZWI4MTYzY2I4 YWMgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XVEYvQ2hhbmdlTG9nCisrKyBiL1NvdXJjZS9XVEYvQ2hh bmdlTG9nCkBAIC0xLDMgKzEsMjkgQEAKKzIwMTktMDktMjQgIEtlaXRoIFJvbGxpbiAgPGtyb2xs aW5AYXBwbGUuY29tPgorCisgICAgICAgIEFkZHJlc3Mgc3RhdGljIGFuYWx5c2lzIHdhcm5pbmcg aW4gVVRleHRQcm92aWRlckxhdGluMS5jcHA6IEFycmF5IGFjY2VzcyByZXN1bHRzIGluIGEgbnVs bCBwb2ludGVyIGRlcmVmZXJlbmNlCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3No b3dfYnVnLmNnaT9pZD0yMDIxNTUKKyAgICAgICAgPHJkYXI6Ly9wcm9ibGVtLzU1NjcyNDIyPgor CisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIFhjb2RlJ3Mg c3RhdGljIGFuYWx5c2lzIHJlcG9ydHM6CisKKyAgICAgICAgICAgIC4uLi9PcGVuU291cmNlL1Nv dXJjZS9XVEYvd3RmL3RleHQvaWN1L1VUZXh0UHJvdmlkZXJMYXRpbjEuY3BwOjE4NToyMjogd2Fy bmluZzogQXJyYXkgYWNjZXNzIChmcm9tIHZhcmlhYmxlICdkZXN0JykgcmVzdWx0cyBpbiBhIG51 bGwgcG9pbnRlciBkZXJlZmVyZW5jZQorICAgICAgICAgICAgICAgICAgICBkZXN0W2xlbmd0aF0g PSAwOworICAgICAgICAgICAgICAgICAgICB+fn5+ICAgICAgICAgXgorCisgICAgICAgIFRoaXMg c2VlbXMgdG8gYmUgYSBmYWxzZSBwb3NpdGl2ZS4gRWFybGllciBjaGVja3MgaW52b2x2aW5nCisg ICAgICAgICJkZXN0Q2FwYWNpdHkiIHdpbGwgZXhpdCB0aGUgZnVuY3Rpb24gaWYgImRlc3RDYXBh Y2l0eSIgaXMgbmVnYXRpdmUsCisgICAgICAgIG9yIGlmICJkZXN0IiBpcyBOVUxMIGFuZCAiZGVz dENhcGFjaXR5IiBpcyBwb3NpdGl2ZS4gVGhlcmVmb3JlLCBpZgorICAgICAgICAiZGVzdCIgaXMg TlVMTCwgd2Ugd2lsbCBvbmx5IGVudGVyIGlmICJkZXN0Q2FwYWNpdHkiIGlzIGV4YWN0bHkgemVy by4KKyAgICAgICAgRnVydGhlciwgdGhlcmUgaXMgYSB0ZXN0IGp1c3QgYWJvdmUgdGhlIGNpdGVk IGFzc2lnbm1lbnQgdGhhdCB3aWxsCisgICAgICAgIG5ldmVyIGJlIHRydWUgaWYgImRlc3RDYXBh Y2l0eSIgaXMgemVyby4gVGhlcmVmb3JlLCB0aGUgY2l0ZWQKKyAgICAgICAgYXNzaWdubWVudCB3 aWxsIG5vdCBiZSBleGVjdXRlZCBpZiAiZGVzdCIgaXMgTlVMTC4gQWRkcmVzcyB0aGlzIGJ5Cisg ICAgICAgIGNvbnZpbmNpbmcgdGhlIHN0YXRpYyBhbmFseXplciBvZiB0aGlzIHdpdGggYW4gQVNT RVJUIHN0YXRlbWVudC4KKworICAgICAgICAqIHd0Zi90ZXh0L2ljdS9VVGV4dFByb3ZpZGVyTGF0 aW4xLmNwcDoKKyAgICAgICAgKFdURjo6dVRleHRMYXRpbjFFeHRyYWN0KToKKwogMjAxOS0wOS0y NCAgS2VpdGggUm9sbGluICA8a3JvbGxpbkBhcHBsZS5jb20+CiAKICAgICAgICAgQWRkcmVzcyBz dGF0aWMgYW5hbHlzaXMgd2FybmluZyBpbiBQYXJraW5nTG90LmNwcDogQWNjZXNzIHRvIGZpZWxk ICdzaXplJyByZXN1bHRzIGluIGEgZGVyZWZlcmVuY2Ugb2YgYSBudWxsIHBvaW50ZXIKZGlmZiAt LWdpdCBhL1NvdXJjZS9XVEYvd3RmL3RleHQvaWN1L1VUZXh0UHJvdmlkZXJMYXRpbjEuY3BwIGIv U291cmNlL1dURi93dGYvdGV4dC9pY3UvVVRleHRQcm92aWRlckxhdGluMS5jcHAKaW5kZXggNzE5 MDQyZmU2MjQ0M2ZhNzAzZDY5MWU0ZjlkZmE5ODA4Y2MzYTMxMC4uZjMwYzZjODMzZGJhOWIwYjY5 ZDQ1MWRkYmIzMzNjYWYyNjM5Zjg3MSAxMDA2NDQKLS0tIGEvU291cmNlL1dURi93dGYvdGV4dC9p Y3UvVVRleHRQcm92aWRlckxhdGluMS5jcHAKKysrIGIvU291cmNlL1dURi93dGYvdGV4dC9pY3Uv VVRleHRQcm92aWRlckxhdGluMS5jcHAKQEAgLTE4Miw2ICsxODIsNyBAQCBzdGF0aWMgaW50MzJf dCB1VGV4dExhdGluMUV4dHJhY3QoVVRleHQqIHVUZXh0LCBpbnQ2NF90IHN0YXJ0LCBpbnQ2NF90 IGxpbWl0LCBVQwogICAgIH0KIAogICAgIGlmIChsZW5ndGggPCBkZXN0Q2FwYWNpdHkpIHsKKyAg ICAgICAgUkVMRUFTRV9BU1NFUlQoZGVzdCk7CiAgICAgICAgIGRlc3RbbGVuZ3RoXSA9IDA7CiAg ICAgICAgIGlmICgqc3RhdHVzID09IFVfU1RSSU5HX05PVF9URVJNSU5BVEVEX1dBUk5JTkcpCiAg ICAgICAgICAgICAqc3RhdHVzID0gVV9aRVJPX0VSUk9SOwo= 379617 2019-09-25 20:27:25 -0700 2019-09-25 23:41:22 -0700 Patch bug-202155-20190925202724.patch text/plain 2594 krollin U3VidmVyc2lvbiBSZXZpc2lvbjogMjUwMzc1CmRpZmYgLS1naXQgYS9Tb3VyY2UvV1RGL0NoYW5n ZUxvZyBiL1NvdXJjZS9XVEYvQ2hhbmdlTG9nCmluZGV4IGQyZmRlMTg2ZGM1NzdhMTcyNzE2YzI0 ZGRiMWMyZjRmYTc3ZTI2YjIuLmVmOGQ2Mzk0NjFkNDAzZWUzMjkwZmM3YzEzZTQ4ZjNkMmY0Mzli ZWQgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XVEYvQ2hhbmdlTG9nCisrKyBiL1NvdXJjZS9XVEYvQ2hh bmdlTG9nCkBAIC0xLDMgKzEsMjkgQEAKKzIwMTktMDktMjUgIEtlaXRoIFJvbGxpbiAgPGtyb2xs aW5AYXBwbGUuY29tPgorCisgICAgICAgIEFkZHJlc3Mgc3RhdGljIGFuYWx5c2lzIHdhcm5pbmcg aW4gVVRleHRQcm92aWRlckxhdGluMS5jcHA6IEFycmF5IGFjY2VzcyByZXN1bHRzIGluIGEgbnVs bCBwb2ludGVyIGRlcmVmZXJlbmNlCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3No b3dfYnVnLmNnaT9pZD0yMDIxNTUKKyAgICAgICAgPHJkYXI6Ly9wcm9ibGVtLzU1NjcyNDIyPgor CisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIFhjb2RlJ3Mg c3RhdGljIGFuYWx5c2lzIHJlcG9ydHM6CisKKyAgICAgICAgICAgIC4uLi9PcGVuU291cmNlL1Nv dXJjZS9XVEYvd3RmL3RleHQvaWN1L1VUZXh0UHJvdmlkZXJMYXRpbjEuY3BwOjE4NToyMjogd2Fy bmluZzogQXJyYXkgYWNjZXNzIChmcm9tIHZhcmlhYmxlICdkZXN0JykgcmVzdWx0cyBpbiBhIG51 bGwgcG9pbnRlciBkZXJlZmVyZW5jZQorICAgICAgICAgICAgICAgICAgICBkZXN0W2xlbmd0aF0g PSAwOworICAgICAgICAgICAgICAgICAgICB+fn5+ICAgICAgICAgXgorCisgICAgICAgIFRoaXMg ZXJyb3IgaXMgZHVlIHRvIGFuIGVhcmxpZXIgImlmIiBzdGF0ZW1lbnQgdGhhdCBjYXVzZWQgdGhl IHN0YXRpYworICAgICAgICBhbmFseXplciB0byBpbmZlciB0aGF0ICJkZXN0IiBjb3VsZCBiZSBO VUxMLiBJdCB0dXJucyBvdXQgdGhhdCB0aGF0CisgICAgICAgIHByZXZpb3VzIGNoZWNrIHdhcyBp biBlcnJvciwgaW4gdGhhdCBpdCB0ZXN0ZWQgZm9yICIhZGVzdCIgd2hlbiBpdAorICAgICAgICBz aG91bGQgaGF2ZSB0ZXN0ZWQgZm9yICJkZXN0Ii4gU28gdGhpcyBwYXRjaCBmaXhlcyB0aGF0Lgor CisgICAgICAgIEV2ZW4gd2l0aCB0aGF0IGVycm9yIGZpeGVkLCB0aGUgc3RhdGljIGFuYWx5emVy IHdpbGwgc3RpbGwgaW5mZXIgdGhhdAorICAgICAgICAiZGVzdCIgY291bGQgYmUgTlVMTCBhdCB0 aGUgcG9pbnQgc2hvd24gYWJvdmUuIFRoZXJlZm9yZSwgYWRkIGEgImRlc3QiCisgICAgICAgIHRl c3QganVzdCBiZWZvcmUgdGhlIGFzc2lnbm1lbnQuCisKKyAgICAgICAgKiB3dGYvdGV4dC9pY3Uv VVRleHRQcm92aWRlckxhdGluMS5jcHA6CisgICAgICAgIChXVEY6OnVUZXh0TGF0aW4xRXh0cmFj dCk6CisKIDIwMTktMDktMjUgIFdlbnNvbiBIc2llaCAgPHdlbnNvbl9oc2llaEBhcHBsZS5jb20+ CiAKICAgICAgICAgW2lQYWRPU10gW0RhdGFBY3RpdmF0aW9uXSBGb2N1cyBtb3ZlcyBhd2F5IGFm dGVyIGZvY3VzaW5nIGlucHV0IGZpZWxkcyBvbiB3d3cuYXR0LmNvbQpkaWZmIC0tZ2l0IGEvU291 cmNlL1dURi93dGYvdGV4dC9pY3UvVVRleHRQcm92aWRlckxhdGluMS5jcHAgYi9Tb3VyY2UvV1RG L3d0Zi90ZXh0L2ljdS9VVGV4dFByb3ZpZGVyTGF0aW4xLmNwcAppbmRleCA3MTkwNDJmZTYyNDQz ZmE3MDNkNjkxZTRmOWRmYTk4MDhjYzNhMzEwLi5kYWY4MWM1Y2EwOWE0M2NkYTdjZWU0MDcyYTI0 MDQxYzU3MDM2NzE3IDEwMDY0NAotLS0gYS9Tb3VyY2UvV1RGL3d0Zi90ZXh0L2ljdS9VVGV4dFBy b3ZpZGVyTGF0aW4xLmNwcAorKysgYi9Tb3VyY2UvV1RGL3d0Zi90ZXh0L2ljdS9VVGV4dFByb3Zp ZGVyTGF0aW4xLmNwcApAQCAtMTczLDcgKzE3Myw3IEBAIHN0YXRpYyBpbnQzMl90IHVUZXh0TGF0 aW4xRXh0cmFjdChVVGV4dCogdVRleHQsIGludDY0X3Qgc3RhcnQsIGludDY0X3QgbGltaXQsIFVD CiAgICAgaWYgKCFsZW5ndGgpCiAgICAgICAgIHJldHVybiAwOwogCi0gICAgaWYgKGRlc3RDYXBh Y2l0eSA+IDAgJiYgIWRlc3QpIHsKKyAgICBpZiAoZGVzdCkgewogICAgICAgICBpbnQzMl90IHRy aW1tZWRMZW5ndGggPSBzdGF0aWNfY2FzdDxpbnQzMl90PihsZW5ndGgpOwogICAgICAgICBpZiAo dHJpbW1lZExlbmd0aCA+IGRlc3RDYXBhY2l0eSkKICAgICAgICAgICAgIHRyaW1tZWRMZW5ndGgg PSBkZXN0Q2FwYWNpdHk7CkBAIC0xODIsNyArMTgyLDggQEAgc3RhdGljIGludDMyX3QgdVRleHRM YXRpbjFFeHRyYWN0KFVUZXh0KiB1VGV4dCwgaW50NjRfdCBzdGFydCwgaW50NjRfdCBsaW1pdCwg VUMKICAgICB9CiAKICAgICBpZiAobGVuZ3RoIDwgZGVzdENhcGFjaXR5KSB7Ci0gICAgICAgIGRl c3RbbGVuZ3RoXSA9IDA7CisgICAgICAgIGlmIChkZXN0KQorICAgICAgICAgICAgZGVzdFtsZW5n dGhdID0gMDsKICAgICAgICAgaWYgKCpzdGF0dXMgPT0gVV9TVFJJTkdfTk9UX1RFUk1JTkFURURf V0FSTklORykKICAgICAgICAgICAgICpzdGF0dXMgPSBVX1pFUk9fRVJST1I7CiAgICAgfSBlbHNl IGlmIChsZW5ndGggPT0gZGVzdENhcGFjaXR5KQo=