Messages in this thread | | | From | Rasmus Villemoes <> | Subject | Re: snprintf, overlapping destination and source | Date | Wed, 16 Dec 2015 14:14:38 +0100 |
| |
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
| |