Gcc 8 string truncation warning

Developers
1

Starting with gcc 8, it will emit a warning if it can determine if a strn* call could be truncated. An example would be:
char buf1[1024];
char buf2[128];
char buf3[1024]
snprintf(buf1, sizeof(buf1), “buf2: %s buf3: %s”, buf2, buf3);

The compiler can determine that buf2 is of size 128, and the format string is adding 10 bytes to it and buf3 is 1024 more bytes. It is possible to put 1162 bytes into a 1024 buffer and force snprintf() to truncate it.

@mkaro has done a wonderful job silencing all of gcc’s warnings. We are having a disagreement on how to silence this warning though. His solution is to force the truncation via string precision.

So you have a situation like the following:
snprintf(buf1, sizeof(buf1), “buf2: %s buf3: %*s”, buf2, sizeof(buf1) - strlen(buf2) - strlen(buf3) - 10, buf3)

This basically moves where the truncation is happening. The buffer will still be truncated. We’ve done the math and forced the field width to be the max size it can be. It will be truncated there, instead of letting snprintf() do its thing. It is the same result, a bit more expensive and harder to program (not to mention it is uglier).

We have turned on -Werror, so this warning needs to be silenced in some way.

I find the warning not too helpful. As a developer, I usually oversize my buffers, but have a general idea of what I’m putting into them. The thing I put into buf2 or buf3 is generally much smaller than the size of the buffer. The truncation that snprintf() will do is to catch me if I am wrong. I don’t need to have the compiler telling me that it is possible that I could truncate. If that happens, it happens.

My solution is to just to ignore the warning. It’s easy enough to tell gcc to not emit this warning.
@mkaro’s opinion (he can correct me if I am wrong) is that we should not ignore any of gcc -Wall’s warnings. They are there for a reason.

I want to hear more opinions. Is this warning of great benefit to the codebase? Is it worth the hassle of silencing it in the code? Or should we just ignore the warning and leave things alone.

Bhroam

2

Thanks @bhroam! Incidentally, the pull request is located here: https://github.com/PBSPro/pbspro/pull/806

It would be nice if you could disable the warning for snprintf() calls and leave it enabled for sprintf(). Perhaps any gcc folks out there would be kind enough to explain the benefit of this warning when snprintf() is being used.

3

I did a little searching and found an article on this particular warning (actually there are 2 warnings). It’s worth a read

The gist of the article is that they think every form of string truncation is bad. They gave two examples. The first is creating a path. In that case, truncating a path would be bad. They also gave the example of a log message. They said that truncating a log message would confuse the user.

The first example can be avoided by creating your destination buffer of MAXPATHLEN. The second I don’t see as too much of an issue. It shouldn’t happen often, and when it does, I don’t think users will be that confused because of it. Our usual log buffer is >4096 bytes. This would be one huge log message. A message of that size alone would be more confusing than the log message being truncated.

Even after reading the article, I’m still of the opinion that we should ignore the warning.

4

Thanks @bhroam, that was a very useful article. The vast majority of cases where we were seeing the truncation warnings fell into two categories:

  1. Assembling a path name where the source components are larger than the destination (like the example in the article) and
  2. Printing long strings (like path names) to the log buffer

Looking at the suggested ways to mitigate the warning, I don’t think any of them are particularly attractive solutions. I’m not suggesting string precision is attractive either, but I like it more than the other options. While we do care if a path gets truncated, I don’t think there are many cases where people are using path names even close to the limit. As far as log messages, I think we’re doing admins a favor by truncating very long messages.

After some thought, I haven’t changed my mind about the approach to use string precision to avoid the warnings. I think the warnings will actually help developers be more mindful when it comes to manipulating strings.

Alas, we are still at an impasse.

5

I don’t think string precision helps people be more mindful on how they manipulate strings. All it does is force string truncation in a different way. The article said this warning is there because they think string truncation is bad and should be avoided. If that is the case, then let’s avoid it. If we are just going to find some other way of truncating the string, then I think the warning does us no good. We’re just jumping through a hoop to silence it.

How about stopping using stack buffers and use asprintf() instead? It won’t be available on windows, so we’ll have to write our own version. It shouldn’t be that hard. You just let snprintf() do the work for you.

Bhroam

6

While asprintf() is an option, I think it opens a whole new can of worms related to memory leaks. Sure, valgrind will help us find them, but not introducing them in the first place would be even better.

Just so I’m clear, you’re suggesting we use asprintf() only for the cases where the compiler complains. We would still use the static log_buffer for the vast majority of log messages. Is that correct?

7

If we want to fix the warning, I personally think that we should do something that will prevent the string from being truncated (like moving away from static buffers). Forcing truncation just to make gcc happy doesn’t make sense to me, either we should heed the warning, or ask gcc to ignore it.

8

I’m looking into @bhroam’s asprintf() suggestion now.

9

Thank you for your feedback. The pbs_asprintf() function has been announced here: Introduction of pbs_asprintf()

I believe this discussion is now concluded.