Re: [PATCH] perf: Incorrect use of snprintf results in SEGV

[Peter Seebach <peter.seebach@windriver.com>]

On Wed, 7 Mar 2012 21:37:25 +0100
Ingo Molnar <mingo@elte.hu> wrote:

> You are missing two important aspects:
> 
> 1) Dynamic reallocation on snprintf() failure is an utterly rare
>    thing - it is used in less than 1% of snprintf() invocations. 
>    (Yes, I just checked a couple of codebases.)

I would agree that it's very rare.  But then, using the return value at
all isn't especially common in my experience -- the only interesting
part, most of the time, is "we're sure this didn't overrun the buffer".
 
>    We *DONT* want to make APIs more fragile just to accomodate a
>    rare, esoteric usecase!

I would view snprintf as an API which already exists.  If it's the
wrong API, by all means, write a different one -- but I would suggest
not using the same name for it.  If a function is going to be called
snprintf, IMO it should have the semantics of snprintf.  If those are
the wrong semantics (and they may well be), then I would say use a
function which has the right semantics, and isn't named snprintf.
 
> 2) It's not even true that should some code want to
>    dynamically allocate the 'required' number of bytes is not
>    available. Some oddball side API could be added for that 1%:
 
> 	size_needed = snprintf_size(...);

That's where the "can write one in terms of the other" argument comes
into play.

If you have snprintf_needed(), it's easy to write snprintf_written()
in terms of it.  If you have only snprintf_written(), it is unreasonably
ugly and/or inefficient to write snprintf_needed() in terms of it.

> So this API could have been designed right but it was messed up 
> out of concern for an insane 1% case - FAIL.

Well, the thing is.  If there *exists* a reallocation case, those
semantics end up being needed.

I do agree that this is a source of errors in usage (and a quick audit
shows that I have at least one use which falls prey to this, as well as
several which check for it correctly).  In practice, I'd guess that
treating probably-negative sizes as an error would likely resolve
things, although that's also a semantics change -- it's just that it's
a semantics change which only affects single snprintf calls that were
expected to write to half the address space.

-s
-- 
Listen, get this.  Nobody with a good compiler needs to be justified.