Re: snprintf, overlapping destination and source

[Rasmus Villemoes <linux@rasmusvillemoes.dk>]

On Mon, Dec 07 2015, Kees Cook <keescook@chromium.org> wrote:

> On Sat, Dec 5, 2015 at 12:38 PM, Rasmus Villemoes
> <linux@rasmusvillemoes.dk> wrote:
>> I did a search for code doing
>>
>>   s[n]printf(buf, "...", ..., buf, ...)
>>
>> and found a few instances. They all do it with the format string
>> beginning with "%s" and buf being passed as the corresponding parameter
>> (obviously to append to the existing string). That works (AFAICT), both
>> with the current printf implementation and with the string()
>> modification which is now in -mm. It would obviously go horribly wrong
>> if anything, even non-specifiers, precede the "%s" in the format
>> string.
[...]
>
> If the replacement isn't ugly/complex/error-prone, we should fix it
> and find a way to detect the issue. Otherwise, we should leave it and
> add it to the printf test cases so we'll notice if it ever regresses.

The usual pattern is to keep the length so far in a variable and do

  len += snprintf(buf + len, bufsize - len, ...)

So this fails if/when overflow is actually possible (we'd end up with a
huge positive size being passed, trigger the WARN_ONCE in vsnprintf and
get a return value of 0, and that would then repeat). But scnprintf has
the property that if you pass a positive bufsize, the return value is
strictly less than that; so if one subtracts said return value from the
available buffer size, one still has a positive buffer size. (Maybe that
invariant should be spelled out somewhere.)

IOW, I think that most users of repeated snprintf(buf, bufsize, "%s...",
buf, ...) could be replaced by 'int len = 0; ... len += scnprintf(buf +
len, bufsize - len, "...", ...);'. Let me go see how that would look.

When sprintf is being used I think one can just directly convert to the
"len +=" model; one would overflow the buffer if and only if the other
would (I think).

Rasmus