217583 2020-10-11 12:58:51 -0700 -Wsign-compare warnings in URL.cpp and URLParser.cpp 2020-10-12 10:40:17 -0700 1 1 1 Unclassified WebKit Web Template Framework WebKit Nightly Build PC Linux RESOLVED FIXED https://bugs.webkit.org/show_bug.cgi?id=215762 InRadar P2 Normal --- 1 mcatanzaro mcatanzaro achristensen benjamin cdumez cmarcelo darin ews-watchlist mcatanzaro webkit-bug-importer oldest_to_newest 1696681 0 mcatanzaro 2020-10-11 12:58:51 -0700 Integer literals are signed, so anything + 1 is signed: [205/6600] Building CXX object Source/WTF/wtf/CMakeFiles/WTF.dir/URL.cpp.o ../../Source/WTF/wtf/URL.cpp: In member function ‘unsigned int WTF::URL::pathStart() const’: ../../Source/WTF/wtf/URL.cpp:110:15: warning: comparison of integer expressions of different signedness: ‘unsigned int’ and ‘int’ [-Wsign-compare] 110 | if (start == m_schemeEnd + 1 | ~~~~~~^~~~~~~~~~~~~~~~~~ [206/6600] Building CXX object Source/WTF/wtf/CMakeFiles/WTF.dir/URLParser.cpp.o ../../Source/WTF/wtf/URLParser.cpp: In member function ‘bool WTF::URLParser::needsNonSpecialDotSlash() const’: ../../Source/WTF/wtf/URLParser.cpp:2501:22: warning: comparison of integer expressions of different signedness: ‘unsigned int’ and ‘int’ [-Wsign-compare] 2501 | && pathStart == m_url.m_schemeEnd + 1 | ~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~ We could use unsigned literals: diff --git a/Source/WTF/wtf/URL.cpp b/Source/WTF/wtf/URL.cpp index ae0324da9fc3..cf2d6bf3538c 100644 --- a/Source/WTF/wtf/URL.cpp +++ b/Source/WTF/wtf/URL.cpp @@ -107,7 +107,7 @@ bool URL::hasSpecialScheme() const unsigned URL::pathStart() const { unsigned start = m_hostEnd + m_portLength; - if (start == m_schemeEnd + 1 + if (start == m_schemeEnd + 1u && start + 1 < m_string.length() && m_string[start] == '/' && m_string[start + 1] == '.') start += 2; diff --git a/Source/WTF/wtf/URLParser.cpp b/Source/WTF/wtf/URLParser.cpp index 8bbb2c0149ca..22db53f93761 100644 --- a/Source/WTF/wtf/URLParser.cpp +++ b/Source/WTF/wtf/URLParser.cpp @@ -2498,7 +2498,7 @@ bool URLParser::needsNonSpecialDotSlash() const { auto pathStart = m_url.m_hostEnd + m_url.m_portLength; return !m_urlIsSpecial - && pathStart == m_url.m_schemeEnd + 1 + && pathStart == m_url.m_schemeEnd + 1u && pathStart + 1 < m_url.m_string.length() && m_url.m_string[pathStart] == '/' && m_url.m_string[pathStart + 1] == '/'; Or we could use casts: diff --git a/Source/WTF/wtf/URL.cpp b/Source/WTF/wtf/URL.cpp index ae0324da9fc3..b94e99759fb2 100644 --- a/Source/WTF/wtf/URL.cpp +++ b/Source/WTF/wtf/URL.cpp @@ -107,7 +107,7 @@ bool URL::hasSpecialScheme() const unsigned URL::pathStart() const { unsigned start = m_hostEnd + m_portLength; - if (start == m_schemeEnd + 1 + if (start == static_cast<unsigned>(m_schemeEnd + 1) && start + 1 < m_string.length() && m_string[start] == '/' && m_string[start + 1] == '.') start += 2; diff --git a/Source/WTF/wtf/URLParser.cpp b/Source/WTF/wtf/URLParser.cpp index 8bbb2c0149ca..be7ab18c4ca3 100644 --- a/Source/WTF/wtf/URLParser.cpp +++ b/Source/WTF/wtf/URLParser.cpp @@ -2498,7 +2498,7 @@ bool URLParser::needsNonSpecialDotSlash() const { auto pathStart = m_url.m_hostEnd + m_url.m_portLength; return !m_urlIsSpecial - && pathStart == m_url.m_schemeEnd + 1 + && pathStart == static_cast<unsigned>(m_url.m_schemeEnd + 1) && pathStart + 1 < m_url.m_string.length() && m_url.m_string[pathStart] == '/' && m_url.m_string[pathStart + 1] == '/'; No strong preference from me, but I'll attach a patch that uses casts since I think it's slightly more clear. 1696682 1 411060 mcatanzaro 2020-10-11 13:01:24 -0700 Created attachment 411060 Patch 1696778 2 411060 darin 2020-10-11 19:33:46 -0700 Comment on attachment 411060 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=411060&action=review > Source/WTF/wtf/URL.cpp:110 > + if (start == static_cast<unsigned>(m_schemeEnd + 1) Doesn’t make sense to me. Isn’t m_schemeEnd an unsigned already? Is there something about being a bitfield that makes things become signed? 1696779 3 411060 darin 2020-10-11 19:34:45 -0700 Comment on attachment 411060 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=411060&action=review >> Source/WTF/wtf/URL.cpp:110 >> + if (start == static_cast<unsigned>(m_schemeEnd + 1) > > Doesn’t make sense to me. Isn’t m_schemeEnd an unsigned already? Is there something about being a bitfield that makes things become signed? Oh, you said that using "1" rather than "1U" makes it signed. I prefer 1U over 1u and certainly over static_cast<unsigned>. 1696842 4 mcatanzaro 2020-10-12 07:39:46 -0700 1U it is, then 1696843 5 411112 mcatanzaro 2020-10-12 07:40:24 -0700 Created attachment 411112 Patch 1696891 6 411112 darin 2020-10-12 10:11:53 -0700 Comment on attachment 411112 Patch Thanks. 1696917 7 ews-feeder 2020-10-12 10:39:56 -0700 Committed r268362: <https://trac.webkit.org/changeset/268362> All reviewed patches have been landed. Closing bug and clearing flags on attachment 411112. 1696918 8 webkit-bug-importer 2020-10-12 10:40:17 -0700 <rdar://problem/70213551> 411060 2020-10-11 13:01:24 -0700 2020-10-12 07:40:22 -0700 Patch bug-217583-20201011150123.patch text/plain 1974 mcatanzaro U3VidmVyc2lvbiBSZXZpc2lvbjogMjY4MjcwCmRpZmYgLS1naXQgYS9Tb3VyY2UvV1RGL0NoYW5n ZUxvZyBiL1NvdXJjZS9XVEYvQ2hhbmdlTG9nCmluZGV4IDIzZWVlNDE0ODg4NjkyOGViZTcxNTJm N2JiMDVhOGZmZmY1MWIxNGQuLmY1NWNlYTI3YTkxM2NmMmY3Yzg1NGVhYTc3YjIzM2VhNWQwOWE5 NGUgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XVEYvQ2hhbmdlTG9nCisrKyBiL1NvdXJjZS9XVEYvQ2hh bmdlTG9nCkBAIC0xLDMgKzEsMTcgQEAKKzIwMjAtMTAtMTEgIE1pY2hhZWwgQ2F0YW56YXJvICA8 bWNhdGFuemFyb0Bnbm9tZS5vcmc+CisKKyAgICAgICAgLVdzaWduLWNvbXBhcmUgd2FybmluZ3Mg aW4gVVJMLmNwcCBhbmQgVVJMUGFyc2VyLmNwcAorICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0 Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MjE3NTgzCisKKyAgICAgICAgQWRkIG1pc3NpbmcgY2FzdHMu CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgKiB3dGYv VVJMLmNwcDoKKyAgICAgICAgKFdURjo6VVJMOjpwYXRoU3RhcnQgY29uc3QpOgorICAgICAgICAq IHd0Zi9VUkxQYXJzZXIuY3BwOgorICAgICAgICAoV1RGOjpVUkxQYXJzZXI6Om5lZWRzTm9uU3Bl Y2lhbERvdFNsYXNoIGNvbnN0KToKKwogMjAyMC0xMC0wOCAgWXVzdWtlIFN1enVraSAgPHlzdXp1 a2lAYXBwbGUuY29tPgogCiAgICAgICAgIFVucmV2aWV3ZWQsIHJlbGFuZCByMjY4MTcwCmRpZmYg LS1naXQgYS9Tb3VyY2UvV1RGL3d0Zi9VUkwuY3BwIGIvU291cmNlL1dURi93dGYvVVJMLmNwcApp bmRleCBhZTAzMjRkYTlmYzNiNTBmM2I3MTliNGY3OGFlMzI4N2VhMDk2ZjYyLi5iOTRlOTk3NTlm YjJlOWQ5ZGMxNmEzNThlMjRjODBjNTAwNmYyN2I0IDEwMDY0NAotLS0gYS9Tb3VyY2UvV1RGL3d0 Zi9VUkwuY3BwCisrKyBiL1NvdXJjZS9XVEYvd3RmL1VSTC5jcHAKQEAgLTEwNyw3ICsxMDcsNyBA QCBib29sIFVSTDo6aGFzU3BlY2lhbFNjaGVtZSgpIGNvbnN0CiB1bnNpZ25lZCBVUkw6OnBhdGhT dGFydCgpIGNvbnN0CiB7CiAgICAgdW5zaWduZWQgc3RhcnQgPSBtX2hvc3RFbmQgKyBtX3BvcnRM ZW5ndGg7Ci0gICAgaWYgKHN0YXJ0ID09IG1fc2NoZW1lRW5kICsgMQorICAgIGlmIChzdGFydCA9 PSBzdGF0aWNfY2FzdDx1bnNpZ25lZD4obV9zY2hlbWVFbmQgKyAxKQogICAgICAgICAmJiBzdGFy dCArIDEgPCBtX3N0cmluZy5sZW5ndGgoKQogICAgICAgICAmJiBtX3N0cmluZ1tzdGFydF0gPT0g Jy8nICYmIG1fc3RyaW5nW3N0YXJ0ICsgMV0gPT0gJy4nKQogICAgICAgICBzdGFydCArPSAyOwpk aWZmIC0tZ2l0IGEvU291cmNlL1dURi93dGYvVVJMUGFyc2VyLmNwcCBiL1NvdXJjZS9XVEYvd3Rm L1VSTFBhcnNlci5jcHAKaW5kZXggOGJiYjJjMDE0OWNhNDYxYjUxMzkwZTgyMjRhM2RkMzU1MTdk YTVhZS4uYmU3YWIxOGM0Y2EzNmIxYTViOGFlZGFlNDY4OGEyMTA3NmIzZDc0OCAxMDA2NDQKLS0t IGEvU291cmNlL1dURi93dGYvVVJMUGFyc2VyLmNwcAorKysgYi9Tb3VyY2UvV1RGL3d0Zi9VUkxQ YXJzZXIuY3BwCkBAIC0yNDk4LDcgKzI0OTgsNyBAQCBib29sIFVSTFBhcnNlcjo6bmVlZHNOb25T cGVjaWFsRG90U2xhc2goKSBjb25zdAogewogICAgIGF1dG8gcGF0aFN0YXJ0ID0gbV91cmwubV9o b3N0RW5kICsgbV91cmwubV9wb3J0TGVuZ3RoOwogICAgIHJldHVybiAhbV91cmxJc1NwZWNpYWwK LSAgICAgICAgJiYgcGF0aFN0YXJ0ID09IG1fdXJsLm1fc2NoZW1lRW5kICsgMQorICAgICAgICAm JiBwYXRoU3RhcnQgPT0gc3RhdGljX2Nhc3Q8dW5zaWduZWQ+KG1fdXJsLm1fc2NoZW1lRW5kICsg MSkKICAgICAgICAgJiYgcGF0aFN0YXJ0ICsgMSA8IG1fdXJsLm1fc3RyaW5nLmxlbmd0aCgpCiAg ICAgICAgICYmIG1fdXJsLm1fc3RyaW5nW3BhdGhTdGFydF0gPT0gJy8nCiAgICAgICAgICYmIG1f dXJsLm1fc3RyaW5nW3BhdGhTdGFydCArIDFdID09ICcvJzsK 411112 2020-10-12 07:40:24 -0700 2020-10-12 10:39:56 -0700 Patch bug-217583-20201012094023.patch text/plain 2009 mcatanzaro U3VidmVyc2lvbiBSZXZpc2lvbjogMjY4MzMwCmRpZmYgLS1naXQgYS9Tb3VyY2UvV1RGL0NoYW5n ZUxvZyBiL1NvdXJjZS9XVEYvQ2hhbmdlTG9nCmluZGV4IGU2MzQ3OTAxNGY4MzY1OGI0ZDVhY2E5 MmJiYTVhYmZhOWM3MTMwZjEuLjJjMDE5MzU0MjNhOTMzMDQ2NzBmNDlkNWQ2MmMxZTBlMDM3MzI5 YzIgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XVEYvQ2hhbmdlTG9nCisrKyBiL1NvdXJjZS9XVEYvQ2hh bmdlTG9nCkBAIC0xLDMgKzEsMTcgQEAKKzIwMjAtMTAtMTIgIE1pY2hhZWwgQ2F0YW56YXJvICA8 bWNhdGFuemFyb0Bnbm9tZS5vcmc+CisKKyAgICAgICAgLVdzaWduLWNvbXBhcmUgd2FybmluZ3Mg aW4gVVJMLmNwcCBhbmQgVVJMUGFyc2VyLmNwcAorICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0 Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MjE3NTgzCisKKyAgICAgICAgVXNpbmcgdW5zaWduZWQgbGl0 ZXJhbHMgd2hlcmUgcmVxdWlyZWQuCisKKyAgICAgICAgUmV2aWV3ZWQgYnkgRGFyaW4gQWRsZXIu CisKKyAgICAgICAgKiB3dGYvVVJMLmNwcDoKKyAgICAgICAgKFdURjo6VVJMOjpwYXRoU3RhcnQg Y29uc3QpOgorICAgICAgICAqIHd0Zi9VUkxQYXJzZXIuY3BwOgorICAgICAgICAoV1RGOjpVUkxQ YXJzZXI6Om5lZWRzTm9uU3BlY2lhbERvdFNsYXNoIGNvbnN0KToKKwogMjAyMC0xMC0xMSAgTHVt aW5nIFlpbiAgPGx1bWluZ195aW5AYXBwbGUuY29tPgogCiAgICAgICAgIFN0cmlwIHBhdGNoIHZl cnNpb24gZnJvbSBUQVJHRVRfTUFDX09TX1hfVkVSU0lPTl9NQUpPUiB3aGVuIGJ1aWxkaW5nIGZv ciBtYWNPUyBCaWcgU3VyIApkaWZmIC0tZ2l0IGEvU291cmNlL1dURi93dGYvVVJMLmNwcCBiL1Nv dXJjZS9XVEYvd3RmL1VSTC5jcHAKaW5kZXggYWUwMzI0ZGE5ZmMzYjUwZjNiNzE5YjRmNzhhZTMy ODdlYTA5NmY2Mi4uM2UyMTRjMjZjMGNlYTJkZDM4YTc5MmEyYWQ2YjQ0NDVlMWRhMjhhNiAxMDA2 NDQKLS0tIGEvU291cmNlL1dURi93dGYvVVJMLmNwcAorKysgYi9Tb3VyY2UvV1RGL3d0Zi9VUkwu Y3BwCkBAIC0xMDcsNyArMTA3LDcgQEAgYm9vbCBVUkw6Omhhc1NwZWNpYWxTY2hlbWUoKSBjb25z dAogdW5zaWduZWQgVVJMOjpwYXRoU3RhcnQoKSBjb25zdAogewogICAgIHVuc2lnbmVkIHN0YXJ0 ID0gbV9ob3N0RW5kICsgbV9wb3J0TGVuZ3RoOwotICAgIGlmIChzdGFydCA9PSBtX3NjaGVtZUVu ZCArIDEKKyAgICBpZiAoc3RhcnQgPT0gbV9zY2hlbWVFbmQgKyAxVQogICAgICAgICAmJiBzdGFy dCArIDEgPCBtX3N0cmluZy5sZW5ndGgoKQogICAgICAgICAmJiBtX3N0cmluZ1tzdGFydF0gPT0g Jy8nICYmIG1fc3RyaW5nW3N0YXJ0ICsgMV0gPT0gJy4nKQogICAgICAgICBzdGFydCArPSAyOwpk aWZmIC0tZ2l0IGEvU291cmNlL1dURi93dGYvVVJMUGFyc2VyLmNwcCBiL1NvdXJjZS9XVEYvd3Rm L1VSTFBhcnNlci5jcHAKaW5kZXggOGJiYjJjMDE0OWNhNDYxYjUxMzkwZTgyMjRhM2RkMzU1MTdk YTVhZS4uZjllN2RmZjdhOTg2YTU0NzAzOWQwMGRlZjdmYTQzNDJkMmZkMWU3NSAxMDA2NDQKLS0t IGEvU291cmNlL1dURi93dGYvVVJMUGFyc2VyLmNwcAorKysgYi9Tb3VyY2UvV1RGL3d0Zi9VUkxQ YXJzZXIuY3BwCkBAIC0yNDk4LDcgKzI0OTgsNyBAQCBib29sIFVSTFBhcnNlcjo6bmVlZHNOb25T cGVjaWFsRG90U2xhc2goKSBjb25zdAogewogICAgIGF1dG8gcGF0aFN0YXJ0ID0gbV91cmwubV9o b3N0RW5kICsgbV91cmwubV9wb3J0TGVuZ3RoOwogICAgIHJldHVybiAhbV91cmxJc1NwZWNpYWwK LSAgICAgICAgJiYgcGF0aFN0YXJ0ID09IG1fdXJsLm1fc2NoZW1lRW5kICsgMQorICAgICAgICAm JiBwYXRoU3RhcnQgPT0gbV91cmwubV9zY2hlbWVFbmQgKyAxVQogICAgICAgICAmJiBwYXRoU3Rh cnQgKyAxIDwgbV91cmwubV9zdHJpbmcubGVuZ3RoKCkKICAgICAgICAgJiYgbV91cmwubV9zdHJp bmdbcGF0aFN0YXJ0XSA9PSAnLycKICAgICAgICAgJiYgbV91cmwubV9zdHJpbmdbcGF0aFN0YXJ0 ICsgMV0gPT0gJy8nOwo=