[U-Boot] [PATCH 1/1] vsprintf.c: add EFI device path printing

[Heinrich Schuchardt]

On 11/21/2017 03:16 PM, Wolfgang Denk wrote:
> Dear Heinrich Schuchardt,
> 
> In message <d643b22b-2511-9b40-772b-5f7f4486c289 at gmx.de> you wrote:
>>
>>>>>> +	u16 *str = efi_dp_str((struct efi_device_path *)dp);
>>>>>> +
>>>>>> +	buf = string16(buf, end, str, field_width, precision, flags);
>>>>>> +	efi_free_pool(str);
>>>>>
>>>>> efi_dp_str() can return NULL. Should this not be handled?
>>>>
>>>> Thanks for reviewing.
>>>>
>>>> This situation is caught in string16:
>>>> u16 *str = s ? s : L"<NULL>";
>>>
>>> This is crash-preventing cosmetics, but no real error handling.  I
>>> mean, should we not wave a big red flag and shout at the user: "Hey,
>>> your system is going berserk here!" ?
>>
>> Why do you think that the error handling should be in THIS function?
> 
> It should be somewhere - but you cannot handle an error condition
> that you don't report to the caller.
> 
> Instead of returning from the function with a clear error message
> and an error return code, you ignore it.
> 
> The minimum action to take here would be that device_path_string()
> returns NULL when the error occurs, if there was a chance for the
> caller to handle that.
> 
>> I can understand that we might improve error handling in the EFI coding 
>> but I do not believe we should do it here.
> 
> But if you don't even return an error code you have no chance to
> handle it elsewhere.
> 
>>> Yes, and running out of memory at such a place is likely to be
>>> unrecoverable. So we should terminate all further processing baed on
>>> this result, and not continue with bogus data.
>>
>> printf() does not have any return value.
> 
> This is incorrect. printf() is declared as
> 
> 	int    printf(const char *fmt, ...);
> 
> as usual.

You are absolutely right. The C standard defines printf as returning a
negative number if an error arises.

Unfortunately the consumers of printf behave quite differently:

xasprintf(), PyString_FromFormat() correctly identify negative values as
an error.

set_config_filename, dbg_snprintf_key(), bootstage_mark_code() - to name
a few - will access illegal memory addresses.

As long as we cannot assure that each and every caller of a printf
function handles negative return values correctly the only safe handling
of errors is to return 0 or panic().

> 
>> So here we could only panic().
>>
>> Is this really what you have in mind?
> 
> Well, which other options do you see when you run out of memory?
> I can't see how you could recover from this, so if you don't abort
> here with a clear error message you will either do the same
> elsewhere (with a misleading error, as the actual problem happened
> eventually long before), or you will just crash, or run into
> undefined behaviour.

So I will resend the patch with panic().

Best regards

Heinrich

> 
> Ignoring error condistions is always a Bad thing (TM).
> 
> Best regards,
> 
> Wolfgang Denk
>