Bug 174506 – -Wformat-truncation warning in ConfigFile.cpp

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Michael Catanzaro]

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.

[Michael Catanzaro]

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]

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]

Created attachment 315438 [details]
Patch

[Loïc Yhuel]

Checking the return value of snprintf should avoid the warning.
Perhaps we should return ParseError instead of silently truncating the file name.

[Michael Catanzaro]

(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]

Created attachment 315473 [details]
Patch

[Darin Adler]

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]

(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]

(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]

(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]

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]

Committed r219561: <http://trac.webkit.org/changeset/219561>

[Radar WebKit Bug Importer]

<rdar://problem/35624450>