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

[Simon Glass]

Hi Heinrich,

On 23 November 2017 at 14:29, Heinrich Schuchardt <xypron.glpk at gmx.de> wrote:
> 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.

I think you are tarring with too broad a brush. I don't see how
sprintf() can return an error. What would an 'output error' be in that
case?

>
> 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().

I suppose that is OK, but we should try to return the expected error
from standard functions. If that causes other code to fail, then we
need to fix that other code.

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

Regards,
Simon