Bug 174506 - -Wformat-truncation warning in ConfigFile.cpp
Summary: -Wformat-truncation warning in ConfigFile.cpp
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: Other
Hardware: PC Linux
: P2 Minor
Assignee: Michael Catanzaro
URL:
Keywords: InRadar
Depends on:
Blocks: 174463
  Show dependency treegraph
 
Reported: 2017-07-14 09:26 PDT by Michael Catanzaro
Modified: 2017-11-17 14:39 PST (History)
8 users (show)

See Also:


Attachments
Patch (1.96 KB, patch)
2017-07-14 10:09 PDT, Michael Catanzaro
no flags Details | Formatted Diff | Diff
Patch (1.92 KB, patch)
2017-07-14 12:40 PDT, Michael Catanzaro
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Catanzaro 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.
Comment 1 Michael Catanzaro 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.
Comment 2 Michael Catanzaro 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.
Comment 3 Michael Catanzaro 2017-07-14 10:09:05 PDT
Created attachment 315438 [details]
Patch
Comment 4 Loïc Yhuel 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.
Comment 5 Michael Catanzaro 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!
Comment 6 Michael Catanzaro 2017-07-14 12:40:03 PDT
Created attachment 315473 [details]
Patch
Comment 7 Darin Adler 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.
Comment 8 Michael Catanzaro 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.
Comment 9 Darin Adler 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.
Comment 10 Darin Adler 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.
Comment 11 Michael Catanzaro 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.
Comment 12 Michael Catanzaro 2017-07-17 08:31:07 PDT
Committed r219561: <http://trac.webkit.org/changeset/219561>
Comment 13 Radar WebKit Bug Importer 2017-11-17 14:39:55 PST
<rdar://problem/35624450>