Re: [PATCH 1/5] lib/vsprintf.c: make sure vsnprintf() never returns a negative value

[Rasmus Villemoes <rasmus.villemoes@prevas.dk>]

On 21/05/2021 16.15, Heinrich Schuchardt wrote:
> On 21.05.21 14:53, Rasmus Villemoes wrote:
>> On 20/05/2021 19.51, Simon Glass wrote:
>>> Hi Rasmus,
>>>
>>> On Thu, 20 May 2021 at 04:05, Rasmus Villemoes
>>> <rasmus.villemoes@prevas.dk> wrote:
>>>>
>>>> Most callers (or callers of callers, etc.) of vsnprintf() are not
>>>> prepared for it to return a negative value.
>>>>
>>>> The only case where that can currently happen is %pD, and it's IMO
>>>> more user-friendly to produce some output that clearly shows that some
>>>> "impossible" thing happened instead of having the message completely
>>>> ignored - or mishandled as for example log.c would currently do.
>>>>
>>>> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
>>>> ---
>>>>  lib/vsprintf.c | 10 +---------
>>>>  1 file changed, 1 insertion(+), 9 deletions(-)
>>>
>>> I think that is debatable. If we want the calling code to be fixed,
>>> then it needs to get an error code back. Otherwise the error will be
>>> apparent to the user but (perhaps) not ever debugged.
>>
>> But it is not the calling code that is at fault for the vsnprintf()
>> implementation (1) being able to fail and (2) actually encountering an
>> ENOMEM situation. There's _nothing_ the calling code can do about that.
> 
> include/vsnprintf.h states:
> 
> "This function follows C99 vsnprintf, but has some extensions:".

Happy to update that comment (which is copied from linux BTW, and in the
kernel there's a very simple rule: "This is printk. We want it to work."
- that extends to the workhorse vsnprintf() which is not allowed to take
locks or do allocations) with an amendment "... among which is that it
never returns a negative value."

> It is obvious that the calling code needs to be fixed if it cannot
> handle negative return values.
> 
> So NAK to the patch.

So you'd rather fix the ~200 places that use the return value assuming
it's non-negative, plus the unknown number of places that assume the
output buffer is a valid nul-terminated string after vsnprintf() returns?

And, again taking that %pD example, do you really rather want _nothing_
printed (which is the only thing log.c could sensibly do if vsnprintf()
returned something negative) in the rare case of allocation failure?

I must admit I completely fail to see how this can possibly be better
than improving the guarantees provided by U-Boot's vsnprintf()
implementation.

Rasmus