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

[Heinrich Schuchardt]

On 21.05.21 16:40, Rasmus Villemoes wrote:
> 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 at 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 at 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,

As you pointed out in patch 5 any usage of (v)snprintf assuming that the
return value is the number of characters copied to the buffer has to be
corrected anyways.

plus the unknown number of places that assume the
> output buffer is a valid nul-terminated string after vsnprintf() returns?

Ensuring that the buffer is always nul-terminated would not contradict
the C99 specification.

Best regards

Heinrich

>
> 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
>