RESOLVED FIXED 174506
-Wformat-truncation warning in ConfigFile.cpp 
https://bugs.webkit.org/show_bug.cgi?id=174506
Summary -Wformat-truncation warning in ConfigFile.cpp
Michael Catanzaro
Reported 2017-07-14 09:26:20 PDT
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.
Attachments
Patch (1.96 KB, patch)
2017-07-14 10:09 PDT, Michael Catanzaro 		no flags
DetailsFormatted DiffDiff
Patch (1.92 KB, patch)
2017-07-14 12:40 PDT, Michael Catanzaro 		darin: review+
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Michael Catanzaro
Comment 1 2017-07-14 09:41:38 PDT
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.
Michael Catanzaro
Comment 2 2017-07-14 10:05:43 PDT
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.
Michael Catanzaro
Comment 3 2017-07-14 10:09:05 PDT
Created attachment 315438 [details] Patch
Loïc Yhuel
Comment 4 2017-07-14 11:03:43 PDT
Checking the return value of snprintf should avoid the warning. Perhaps we should return ParseError instead of silently truncating the file name.
Michael Catanzaro
Comment 5 2017-07-14 12:38:19 PDT
(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!
Michael Catanzaro
Comment 6 2017-07-14 12:40:03 PDT
Created attachment 315473 [details] Patch
Darin Adler
Comment 7 2017-07-15 08:23:26 PDT
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. Also, ">= s_maxPathLength + 1" is the same thing as "> s_maxPathLength" and the version with >= and +1 seems more oblique.
Michael Catanzaro
Comment 8 2017-07-15 09:20:21 PDT
(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.
Darin Adler
Comment 9 2017-07-15 09:34:55 PDT
(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.
Darin Adler
Comment 10 2017-07-15 09:43:53 PDT
(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.
Michael Catanzaro
Comment 11 2017-07-17 08:26:39 PDT
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.
Michael Catanzaro
Comment 12 2017-07-17 08:31:07 PDT
Committed r219561: <http://trac.webkit.org/changeset/219561>
Radar WebKit Bug Importer
Comment 13 2017-11-17 14:39:55 PST
<rdar://problem/35624450>
Note You need to log in before you can comment on or make changes to this bug.