[U-Boot] [PATCH 0/4] Buffer overruns in printf

[Albert ARIBAUD]

Hi Simon,

Le 25/09/2011 16:50, Simon Glass a écrit :

>> Basically, printf family functions which do not have the 'n' are *know* by
>> all -- experienced enough :) -- programmers to be *unsafe* (but to require
>> less from the caller) and it should remain so: no programmer should ever
>> encounter an implementation of printf that pretends to be even somewhat
>> safe, because it might bite him/her elsewhere, in another project based on
>> another C library where printf is just the beartrap it usually is.
>>
>> IOW, programmers already have assumptions about *printf(), including how to
>> deal with length limitations and what happens if you don't, and it is best
>> that these assumption remain true whatever project they work with.
>
> I don't quite understand this. You are saying that we shouldn't modify
> the existing printf(), serial_printf() etc. so live within the buffer
> they use? If I call printf() and my resulting string is too long for
> the library then I would much prefer to get truncated output than have
> to hunt for a wierd crash.

I understand the preference for truncated output, but the output length 
is primarily in the hands of the caller, through the format stringand 
possibly also through *nprintf() -- i.e. the caller can only get an 
output overrun if (s)he does not take the required steps to avoid it.

> It sounds like you are asking for a new printf(), serial_printf()
> which provides the facility of limiting the output to n characters,
> and leave these ones alone. But these functions are not passed a
> buffer - they use their own and they set the site of it. So I think
> they should be responsible for enforcing that size.

The existing contract for printf family functions (think wider than 
U-Boot) does not enforce this responsibility on them.

> If you are asking for a new set of functions - nprintf(),
> serial_nprintf(), etc. then I wonder how you can pass the 'n' but not
> the actual buffer. In my mind you should not do one without the other.
>
> So please can you clarify?

I should clarify indeed. My opinion, expressed as a single general rule, 
is "keep the known semantics of *printf() function family unchanged in 
U-Boot wrt all other printif implementations around". Thus I think that:

- the printf() function should *not* attempt anything to mitigate length 
issues beyond what the standard mandates. If a user calls printf(), 
(s)he is expected to be aware of the risks of overruns,  and to take -- 
if (s)he so decides -- steps to avoid it; besides, the function does not 
have a way. Conversively, implementers of the printf() function need not 
consider any specific recovery action with regard to size issues. For 
instance, why do we have an internal buffer for printf to begin with? 
The implementation does not need them (and besides, I guess the buffer 
does not respect the single C99 environmental constraint for printf, 
that any field should be able to be at least 4095 bytes -- no kidding).

- users who actually wisht to limit outpout ca use either

-- a crafted format string with maximum-sized format specifiers, or

-- a call tp a *nprintf() function into a local buffer of known size 
followed by a call to fputs().

>>> By the way, printf() ends up calling the same code, but without limit
>>> checking in place. The alternative is to duplicate all the format
>>> string processing code (a limit-checking version and an unchecked
>>> version) which would be worse.
>>
>> I don't intend to dictate the way things can be implemented, so the degree
>> of code reuse is an open question as far as I am concerned. I am only
>> voicing my opinion that *printf() APIs and their contracts should remain
>> identical across all implementations of *printf(), and thus that providing
>> *nprintf() where they don't exist is commandable, but hardening printf() is
>> not, since you basically cannot do it without somewhat departing from the de
>> facto standard.
>
> OK fair enough. People are used to printf() just working, no matter
> what the size. I haven't looked at the implementation but I suspect
> when the buffer fills it outputs it and starts the buffer again .In
> any case you don't have to worry about it with the GNU C library.

You may also find implementations where the buffer is flushed after each 
literal part in the format string and after each format specifier 
output. Or some which emit serial characters as soon as they are 
produced and use a buffer only for those formats where the ASCII 
representation cannot be easily constructed left-to-right.

> We probably don't need to go that far in U-Boot, but some simple
> checking would avoid a nasty surprise for the user. It is obvious from
> the result that something is truncated, and we can WARN if that helps.

A user who does not expect a nasty surprise from calling printf() with a 
fair chance of overflowing the output buffer deserves the surprise. :)

> Regards,
> Simon

Amicalement,
-- 
Albert.