174506 2017-07-14 09:26:20 -0700 -Wformat-truncation warning in ConfigFile.cpp 2017-11-17 14:39:55 -0800 1 1 1 Unclassified WebKit JavaScriptCore Other PC Linux RESOLVED FIXED InRadar P2 Minor --- 174463 1 mcatanzaro mcatanzaro buildbot darin keith_miller loic.yhuel mark.lam mcatanzaro msaboff saam oldest_to_newest 1328800 0 mcatanzaro 2017-07-14 09:26:20 -0700 This is another new GCC 7 warning. [978/5861] Building CXX object Source/...criptCore.dir/runtime/ConfigFile.cpp.o ../../Source/JavaScriptCore/runtime/ConfigFile.cpp: In lambda function: ../../Source/JavaScriptCore/runtime/ConfigFile.cpp:281:62: warning: ‘snprintf’ output may be truncated before the last format character [-Wformat-truncation=] auto parseLogFile = [&](StatementNesting statementNesting) { ^ ../../Source/JavaScriptCore/runtime/ConfigFile.cpp:286:29: note: ‘snprintf’ output 2 or more bytes (assuming 4098) into a destination of size 4097 snprintf(logPathname, s_maxPathLength + 1, "%s/%s", m_configDirectory, filename); ~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ^ The warning means that m_configDirectory could potentially consume as much as s_maxPathLength, followed by the trailing NULL, and then the / and the filename might not get printed. That seems like the most reasonable thing to do in this case, so I think that's fine and the warning should be suppressed. Unfortunately, compilers warn when they see unknown warning suppression pragmas, so the pattern for this is not pretty. I would almost prefer to use WTF::String instead, except all the rest of the code in this function uses cstring functions. 1328812 1 mcatanzaro 2017-07-14 09:41:38 -0700 Really strange, it cannot be suppressed by the usual pattern: #if COMPILER(GCC) && GCC_VERSION_AT_LEAST(7, 0, 0) #pragma GCC diagnostic push #pragma GCC diagnostic ignored "-Wformat-truncation" #endif snprintf(logPathname, s_maxPathLength + 1, "%s/%s", m_configDirectory, filename); #if COMPILER(GCC) && GCC_VERSION_AT_LEAST(7, 0, 0) #pragma GCC diagnostic pop #endif No idea why. GCC does read and understand the pragma (else it would warn), it just doesn't work. Odd. 1328823 2 mcatanzaro 2017-07-14 10:05:43 -0700 I wound up rewriting the code to avoid the warning. Unfortunately the code is messier now and has the same behavior, but the warning seems useful so I don't really want to turn it off. 1328827 3 315438 mcatanzaro 2017-07-14 10:09:05 -0700 Created attachment 315438 Patch 1328866 4 loic.yhuel 2017-07-14 11:03:43 -0700 Checking the return value of snprintf should avoid the warning. Perhaps we should return ParseError instead of silently truncating the file name. 1328945 5 mcatanzaro 2017-07-14 12:38:19 -0700 (In reply to Loïc Yhuel from comment #4) > Checking the return value of snprintf should avoid the warning. > Perhaps we should return ParseError instead of silently truncating the file > name. Much better, thanks for the suggestion! 1328947 6 315473 mcatanzaro 2017-07-14 12:40:03 -0700 Created attachment 315473 Patch 1329185 7 315473 darin 2017-07-15 08:23:26 -0700 Comment on attachment 315473 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=315473&action=review > Source/JavaScriptCore/runtime/ConfigFile.cpp:288 > + if (written < 0 || static_cast<unsigned>(written) >= s_maxPathLength + 1) The "written < 0" check here is redundant. If it is true, then the other check is also guaranteed to be true. Also, ">= s_maxPathLength + 1" is the same thing as "> s_maxPathLength" and the version with >= and +1 seems more oblique. 1329190 8 mcatanzaro 2017-07-15 09:20:21 -0700 (In reply to Darin Adler from comment #7) > Comment on attachment 315473 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=315473&action=review > > > Source/JavaScriptCore/runtime/ConfigFile.cpp:288 > > + if (written < 0 || static_cast<unsigned>(written) >= s_maxPathLength + 1) > > The "written < 0" check here is redundant. If it is true, then the other > check is also guaranteed to be true. What, why would that be? In practice, yes, a negative return value (probably -1) is almost surely going to be greater than s_maxPathLength + 1 when cast to an unsigned int. But it would be silly to rely on that instead of writing out the check properly. written < 0 checks to see if the function call failed. written >= s_maxPathLength + 1 checks to see if the result was truncated. I suppose "written" is a terrible name for this variable, because it actually returns the number of bytes that would have been written if the value were not truncated. I'll choose a better name. > Also, ">= s_maxPathLength + 1" is the same thing as "> s_maxPathLength" and > the version with >= and +1 seems more oblique. I almost changed it to > s_maxPathLength, but I think it's more clear to read this way because it exactly matches the second argument to snprintf. 1329196 9 darin 2017-07-15 09:34:55 -0700 (In reply to Michael Catanzaro from comment #8) > In practice, yes, a negative return value (probably -1) is almost surely > going to be greater than s_maxPathLength + 1 when cast to an unsigned int. Not "almost surely", absolutely 100% guaranteed. > But it would be silly to rely on that instead of writing out the check > properly. OK. 1329199 10 darin 2017-07-15 09:43:53 -0700 (In reply to Michael Catanzaro from comment #8) > > Also, ">= s_maxPathLength + 1" is the same thing as "> s_maxPathLength" and > > the version with >= and +1 seems more oblique. > > I almost changed it to > s_maxPathLength, but I think it's more clear to > read this way because it exactly matches the second argument to snprintf. Yes the code makes sense if you just read the snprintf manual page and are expecting code implementing "if the return value is greater than or equal to the size argument, the string was too short". Even then, I think it would be easier to understand if we had a constant named something like bufferSize rather than "s_maxPathLength + 1". Otherwise, I think it’s a lot easier to understand the ">" version: If the length of the string that would have been printed is larger than the maximum length we can handle, then we have a problem. Rather than the ">=" version: If the length of the string that would have been printed is larger than or equal to the size of the buffer, allocated for the maximum string length plus one for the null terminator, then we have a problem. The reason it's a problem when it's only equal and not greater than the size of the buffer is that in that case the string fits in the buffer but the null terminator doesn't. I’m not trying to exaggerate anything here, just explain why I think it’s so much harder to know the ">=" version is correct. 1329565 11 mcatanzaro 2017-07-17 08:26:39 -0700 I still think the >= version is clearer, because even though the thought process involved is much longer, you really *must* think about the null terminator when working with C strings, and the >= version seems clearer to me when considering that null terminator. But I'll change it to > as you prefer. (In reply to Darin Adler from comment #9) > (In reply to Michael Catanzaro from comment #8) > > In practice, yes, a negative return value (probably -1) is almost surely > > going to be greater than s_maxPathLength + 1 when cast to an unsigned int. > > Not "almost surely", absolutely 100% guaranteed. OK, I see you are right. 1329567 12 mcatanzaro 2017-07-17 08:31:07 -0700 Committed r219561: <http://trac.webkit.org/changeset/219561> 1373307 13 webkit-bug-importer 2017-11-17 14:39:55 -0800 <rdar://problem/35624450> 315438 2017-07-14 10:09:05 -0700 2017-07-14 12:40:00 -0700 Patch bug-174506-20170714120904.patch text/plain 2008 mcatanzaro U3VidmVyc2lvbiBSZXZpc2lvbjogMjE5NDk3CmRpZmYgLS1naXQgYS9Tb3VyY2UvSmF2YVNjcmlw dENvcmUvQ2hhbmdlTG9nIGIvU291cmNlL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwppbmRleCA2 OTFlNDlmNzZlNTUyZmVhMzRiMjJjODJiZWIyYTUwYWU2MTEwYmM1Li5hMTZhYTIwMTc4ZTEyYjEz NGQwMjg4Yjc1ZTkxYzQ1YzFmOTNiZGVjIDEwMDY0NAotLS0gYS9Tb3VyY2UvSmF2YVNjcmlwdENv cmUvQ2hhbmdlTG9nCisrKyBiL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cKQEAgLTEs MyArMSwxMyBAQAorMjAxNy0wNy0xNCAgTWljaGFlbCBDYXRhbnphcm8gIDxtY2F0YW56YXJvQGln YWxpYS5jb20+CisKKyAgICAgICAgU2lsZW5jZSAtV2Zvcm1hdC10cnVuY2F0aW9uIHdhcm5pbmcg aW4gQ29uZmlnRmlsZS5jcHAKKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19i dWcuY2dpP2lkPTE3NDUwNgorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgor CisgICAgICAgICogcnVudGltZS9Db25maWdGaWxlLmNwcDoKKyAgICAgICAgKEpTQzo6Q29uZmln RmlsZTo6cGFyc2UpOgorCiAyMDE3LTA3LTEzICBNaWNoYWVsIENhdGFuemFybyAgPG1jYXRhbnph cm9AaWdhbGlhLmNvbT4KIAogICAgICAgICBGaXggY29tcGlsZXIgd2FybmluZ3Mgd2hlbiBidWls ZGluZyB3aXRoIEdDQyA3CmRpZmYgLS1naXQgYS9Tb3VyY2UvSmF2YVNjcmlwdENvcmUvcnVudGlt ZS9Db25maWdGaWxlLmNwcCBiL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9ydW50aW1lL0NvbmZpZ0Zp bGUuY3BwCmluZGV4IDA5MTViMWJkZTYzNTI5YzFkMjNlYjM1NTg4OGRlNmZlOGQ3MTljNmEuLjdj YjQ1ZmViOWVmZDcxYjllMTdiMTQ0NTIwYzk4MzAwZjdkN2IzMmYgMTAwNjQ0Ci0tLSBhL1NvdXJj ZS9KYXZhU2NyaXB0Q29yZS9ydW50aW1lL0NvbmZpZ0ZpbGUuY3BwCisrKyBiL1NvdXJjZS9KYXZh U2NyaXB0Q29yZS9ydW50aW1lL0NvbmZpZ0ZpbGUuY3BwCkBAIC0yODIsOSArMjgyLDE1IEBAIHZv aWQgQ29uZmlnRmlsZTo6cGFyc2UoKQogICAgICAgICBjaGFyKiBmaWxlbmFtZSA9IG51bGxwdHI7 CiAgICAgICAgIGlmIChzY2FubmVyLnRyeUNvbnN1bWUoJz0nKSAmJiAoZmlsZW5hbWUgPSBzY2Fu bmVyLnRyeUNvbnN1bWVTdHJpbmcoKSkpIHsKICAgICAgICAgICAgIGlmIChzdGF0ZW1lbnROZXN0 aW5nICE9IE5lc3RlZFN0YXRlbWVudEZhaWxlZENyaXRlcmlhKSB7Ci0gICAgICAgICAgICAgICAg aWYgKGZpbGVuYW1lWzBdICE9ICcvJykKLSAgICAgICAgICAgICAgICAgICAgc25wcmludGYobG9n UGF0aG5hbWUsIHNfbWF4UGF0aExlbmd0aCArIDEsICIlcy8lcyIsIG1fY29uZmlnRGlyZWN0b3J5 LCBmaWxlbmFtZSk7Ci0gICAgICAgICAgICAgICAgZWxzZQorICAgICAgICAgICAgICAgIGlmIChm aWxlbmFtZVswXSAhPSAnLycpIHsKKyAgICAgICAgICAgICAgICAgICAgLy8gQXBwZW5kIGZpbGVu YW1lIHRvIG1fY29uZmlnRGlyZWN0b3J5LCBzZXBhcmF0ZWQgYnkgYSAvLgorICAgICAgICAgICAg ICAgICAgICBzdHJuY3B5KGxvZ1BhdGhuYW1lLCBtX2NvbmZpZ0RpcmVjdG9yeSwgc19tYXhQYXRo TGVuZ3RoKTsKKyAgICAgICAgICAgICAgICAgICAgc2l6ZV90IHNwYWNlRm9yRmlsZW5hbWUgPSBz X21heFBhdGhMZW5ndGggLSBzdHJsZW4obG9nUGF0aG5hbWUpIC0gMTsKKyAgICAgICAgICAgICAg ICAgICAgaWYgKHNwYWNlRm9yRmlsZW5hbWUgPiAwKSB7CisgICAgICAgICAgICAgICAgICAgICAg ICBzdHJuY2F0KGxvZ1BhdGhuYW1lLCAiLyIsIDEpOworICAgICAgICAgICAgICAgICAgICAgICAg c3RybmNhdChsb2dQYXRobmFtZSwgZmlsZW5hbWUsIHNwYWNlRm9yRmlsZW5hbWUpOworICAgICAg ICAgICAgICAgICAgICB9CisgICAgICAgICAgICAgICAgfSBlbHNlCiAgICAgICAgICAgICAgICAg ICAgIHN0cm5jcHkobG9nUGF0aG5hbWUsIGZpbGVuYW1lLCBzX21heFBhdGhMZW5ndGgpOwogICAg ICAgICAgICAgfQogCg== 315473 2017-07-14 12:40:03 -0700 2017-07-15 08:23:26 -0700 Patch bug-174506-20170714144002.patch text/plain 1971 mcatanzaro U3VidmVyc2lvbiBSZXZpc2lvbjogMjE5NTE2CmRpZmYgLS1naXQgYS9Tb3VyY2UvSmF2YVNjcmlw dENvcmUvQ2hhbmdlTG9nIGIvU291cmNlL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwppbmRleCBk ZTIwYzM1NTE5YmI2ZDU0OGFiNjg1NTFkMmRkZmFlNmZiZTBjMGJiLi44MzgxMTI5MWEwN2E4YzVh YTU0ZDQwNDNjYjNiOTk1ZDQyOTQwYTk4IDEwMDY0NAotLS0gYS9Tb3VyY2UvSmF2YVNjcmlwdENv cmUvQ2hhbmdlTG9nCisrKyBiL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cKQEAgLTEs MyArMSwxNiBAQAorMjAxNy0wNy0xNCAgTWljaGFlbCBDYXRhbnphcm8gIDxtY2F0YW56YXJvQGln YWxpYS5jb20+CisKKyAgICAgICAgLVdmb3JtYXQtdHJ1bmNhdGlvbiB3YXJuaW5nIGluIENvbmZp Z0ZpbGUuY3BwCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9p ZD0xNzQ1MDYKKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAg ICBDaGVjayBpZiB0aGUgSlNDIGNvbmZpZyBmaWxlbmFtZSB3b3VsZCBiZSB0cnVuY2F0ZWQgZHVl IHRvIGV4Y2VlZGluZyBtYXggcGF0aCBsZW5ndGguIElmIHNvLAorICAgICAgICByZXR1cm4gUGFy c2VFcnJvci4KKworICAgICAgICAqIHJ1bnRpbWUvQ29uZmlnRmlsZS5jcHA6CisgICAgICAgIChK U0M6OkNvbmZpZ0ZpbGU6OnBhcnNlKToKKwogMjAxNy0wNy0xNCAgWXVzdWtlIFN1enVraSAgPHV0 YXRhbmUudGVhQGdtYWlsLmNvbT4KIAogICAgICAgICBbV1RGXSBVc2Ugc3RkOjp1bmlxdWVfcHRy IGZvciBTdGFja1RyYWNlCmRpZmYgLS1naXQgYS9Tb3VyY2UvSmF2YVNjcmlwdENvcmUvcnVudGlt ZS9Db25maWdGaWxlLmNwcCBiL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9ydW50aW1lL0NvbmZpZ0Zp bGUuY3BwCmluZGV4IDA5MTViMWJkZTYzNTI5YzFkMjNlYjM1NTg4OGRlNmZlOGQ3MTljNmEuLjFh NmFhNjYzZDhlYWQwNmZjNWQ5N2U0MzdkYzQ5NDkxYmIwNjRmMzMgMTAwNjQ0Ci0tLSBhL1NvdXJj ZS9KYXZhU2NyaXB0Q29yZS9ydW50aW1lL0NvbmZpZ0ZpbGUuY3BwCisrKyBiL1NvdXJjZS9KYXZh U2NyaXB0Q29yZS9ydW50aW1lL0NvbmZpZ0ZpbGUuY3BwCkBAIC0yODIsOSArMjgyLDEyIEBAIHZv aWQgQ29uZmlnRmlsZTo6cGFyc2UoKQogICAgICAgICBjaGFyKiBmaWxlbmFtZSA9IG51bGxwdHI7 CiAgICAgICAgIGlmIChzY2FubmVyLnRyeUNvbnN1bWUoJz0nKSAmJiAoZmlsZW5hbWUgPSBzY2Fu bmVyLnRyeUNvbnN1bWVTdHJpbmcoKSkpIHsKICAgICAgICAgICAgIGlmIChzdGF0ZW1lbnROZXN0 aW5nICE9IE5lc3RlZFN0YXRlbWVudEZhaWxlZENyaXRlcmlhKSB7Ci0gICAgICAgICAgICAgICAg aWYgKGZpbGVuYW1lWzBdICE9ICcvJykKLSAgICAgICAgICAgICAgICAgICAgc25wcmludGYobG9n UGF0aG5hbWUsIHNfbWF4UGF0aExlbmd0aCArIDEsICIlcy8lcyIsIG1fY29uZmlnRGlyZWN0b3J5 LCBmaWxlbmFtZSk7Ci0gICAgICAgICAgICAgICAgZWxzZQorICAgICAgICAgICAgICAgIGlmIChm aWxlbmFtZVswXSAhPSAnLycpIHsKKyAgICAgICAgICAgICAgICAgICAgaW50IHdyaXR0ZW4gPSBz bnByaW50Zihsb2dQYXRobmFtZSwgc19tYXhQYXRoTGVuZ3RoICsgMSwgIiVzLyVzIiwgbV9jb25m aWdEaXJlY3RvcnksIGZpbGVuYW1lKTsKKyAgICAgICAgICAgICAgICAgICAgLy8gRmlsZW5hbWUg dHJ1bmNhdGVkPworICAgICAgICAgICAgICAgICAgICBpZiAod3JpdHRlbiA8IDAgfHwgc3RhdGlj X2Nhc3Q8dW5zaWduZWQ+KHdyaXR0ZW4pID49IHNfbWF4UGF0aExlbmd0aCArIDEpCisgICAgICAg ICAgICAgICAgICAgICAgICByZXR1cm4gUGFyc2VFcnJvcjsKKyAgICAgICAgICAgICAgICB9IGVs c2UKICAgICAgICAgICAgICAgICAgICAgc3RybmNweShsb2dQYXRobmFtZSwgZmlsZW5hbWUsIHNf bWF4UGF0aExlbmd0aCk7CiAgICAgICAgICAgICB9CiAK