[PATCH 1/5] lib/vsprintf.c: make sure vsnprintf() never returns a negative value
xypron.glpk at gmx.de
Fri May 21 17:43:24 CEST 2021
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
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.
> 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()
More information about the U-Boot