snprintf still complaints about buffer overflow

Question

When I ran my code through one of the analyzer tool Fortify, it complains about both the snprintf's saying "the format string argument does not properly limit the amount of data the function can write"

I understand snprintf should not result in buffer overflow. But still why does the tool raise this complaint?

Aside: 1) `strftime`, `time` and `localtime` can fail. Check their return values. 2) This is a false positive. — Harith, Feb 09 '23 at 18:20
Well, "the format string argument does not properly limit the amount of data the function can write" is pedantically correct - the format string isn't limiting the amount of data the function can write. It's also completely useless because the function in question is `snprintf()`. I'd file a bug against Fortify for emitting a literally useless warning. — Andrew Henle, Feb 09 '23 at 18:23
`gcc` is more explicit: `warning: ‘%s’ directive output may be truncated writing up to 32 bytes into a region of size between 15 and 34`.You can skip this warning with `volatile size_t size = sizeof(gendata); snprintf(gendata, size, "%s %s", timebuf, text);` — David Ranieri, Feb 09 '23 at 18:30
@TheCcoder it does compiling with `-Wall` flag on (at least on gcc 12.2). — David Ranieri, Feb 09 '23 at 19:06
@DavidRanieri I guess you misunderstood my question. I am not facing compilation issue. But the source code analyzer tool that I am using complains about snprint — The C coder, Feb 09 '23 at 20:07
I've reopened the question. The issues with gcc warning/not warning are not what's being asked about. The question is about "the analyzer tool Fortify". — Andrew Henle, Feb 09 '23 at 20:12
And this question has come up before: [**What could be the possible reason of buffer overflow in snprintf command in C?**](https://stackoverflow.com/questions/38556114/what-could-be-the-possible-reason-of-buffer-overflow-in-snprintf-command-in-c). Note that question has no answer. — Andrew Henle, Feb 09 '23 at 20:18
Presumably, the way to get your code past Fortify would be to add appropriate precision fields to the conversion specifiers in your `snprintf` format to limit the length of the fields they print. Chosen appropriately, these make it redundant to choose `snprintf()` over `printf()`, so Fortify is being overly nitpicky here. On the other hand, defense in depth in this regard provides improved robustness *vs* future changes, and it's worth considering the semantic difference between limiting each field individually and limiting overall buffer length. — John Bollinger, Feb 09 '23 at 20:25
@JohnBollinger But I do have the precision specifier in place as you can see in the code — The C coder, Feb 09 '23 at 21:00
@AndrewHenle I know you reopened this thread. But why do you close my thread in first place when I am still looking for help. — The C coder, Feb 09 '23 at 21:02
No, @TheCcoder, you do not. "[A]ppropriate precision fields [in] the conversion specifiers in your `sprintf` fromat" is about the format string, and the format string `"%s %s"` does not contain any precision fields. With precision fields it would be something like `"%.19s %.14s"` (to accommodate the timestamp and fit in the overall 35-byte array). — John Bollinger, Feb 09 '23 at 21:26
Like most static analyzers, most of its complaints are false positives? — Chris Dodd, Feb 09 '23 at 21:37
@JohnBollinger Thank you. I will try adding the precision fields. Can you check my second snprintf in the last line?. This has the all the specifiers, but still fortify complaints. — The C coder, Feb 09 '23 at 22:47
@TheCcoder, the precision field in a `%d` conversion specifier does not mean the same thing as the precision field in a `%s` specifier. For `%s` it is the *maximum* number of characters to print. But `%d` never truncates values -- for it, the precision gives the *minimum* number of digits to print. The maximum is a characteristic of type `int` (10 for 32-bit `int`s, plus one for a sign). For that one, then, your best bet might be simply to make `rptdata` a bit longer. 36 bytes should do, I think, if you're trying to scrimp. — John Bollinger, Feb 09 '23 at 22:59
@JohnBollinger Isn't it 8 bytes for the int data type. That is the reason I made the buffer size of rptdata as 30. — The C coder, Feb 10 '23 at 00:00
No, @TheCcoder. We're talking about space for the *decimal* representation. The maximum value of a 32-bit signed integer, expressed in decimal, is 2147483648 -- 10 digits. — John Bollinger, Feb 10 '23 at 03:13
@JohnBollinger Got it, Thank you for the details. Will try the solution that you proposed — The C coder, Feb 10 '23 at 03:41
@JohnBollinger Unfortunately fortify still compalints even with all the changes that you proposed. — The C coder, Feb 13 '23 at 12:07

score 7 · Answer 1 · edited Mar 01 '23 at 00:08

I understand snprintf should not result in buffer overflow. But still why does the tool raise this complaint?

Often I have seen analysis tool's complaints are pedantically real, yet the tool points to the wrong original culprit.

Code has at least this weakness

Potential junk in timebuf[]

char timebuf[20];
// Enough space?
strftime(timebuf, sizeof(timebuf),"%Y-%m-%d %H:%M:%S", adjustedtime);

As adjustedtime->tm_year, an int, may have values in the range -2147483648 ... 2147483647, more than size 20 needed.

Avoid under sizing. Recommend:

#define INT_TEXT_LENGTH_MAX 11
char timebuf[6*INT_TEXT_LENGTH_MAX + sizeof "%Y-%m-%d %H:%M:%S"];

Further, it the buffer is not big enough, then:

If the total number of resulting characters including the terminating null character is not more than maxsize, the strftime function returns the number of characters placed into the array pointed to by s not including the terminating null character. Otherwise, zero is returned and the contents of the array are indeterminate. C17dr § 7.27.3.5 Library 8

Thus an analysis tool can assume any content for timebuf[] including a non-string following an unchecked strftime(). That can easily break snprintf(gendata, sizeof(gendata), "%s", timebuf); as "%s" requires a string, which timebuf[] is not guarantied to be. The sizeof(gendata) in snprintf(gendata, sizeof(gendata), ... is not sufficient to prevent UB of an unterminated timebuf[].

Better code would also check the size.

struct tm *adjustedtime = localtime(&t);
if (adjustedtime == NULL) {
  Handle_Error();
}
if (strftime(timebuf, sizeof(timebuf),"%Y-%m-%d %H:%M:%S", adjustedtime) == 0) {
  Handle_Error();
}

Now we can continue with snprintf() code.

snprintf still complaints about buffer overflow

1 Answers1