lkml.org 
[lkml]   [2015]   [Dec]   [16]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRe: snprintf, overlapping destination and source
Date
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


\
 
 \ /
  Last update: 2015-12-16 14:41    [W:0.150 / U:0.488 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site