Messages in this thread | | | Date | Wed, 7 Mar 2012 15:19:51 -0600 | From | Peter Seebach <> | Subject | Re: [PATCH] perf: Incorrect use of snprintf results in SEGV |
| |
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.
| |