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

[Heinrich Schuchardt]

On 11/25/2017 11:34 PM, Simon Glass wrote:
> 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
There are two sides of you question:

1) Does the printing function have a return type that can be used to 
signal an error?
2) Can the printing function called with an argument that can result in 
an error?

1) In lib/vsprintf all printing functions return int. So we could return 
a negative number if an error situation arises.

2) Currently we ignore error situations:
%ps replaces illegal code points by question marks.
If we would follow Wolfgang's reasoning we should create an error for
sprintf("%ps\n", ptr) if ptr contains an unconvertible code.

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

Does this imply:

as none of the current conversion functions in lib/vsprintf.c is 
creating an error value we only have to care about future callers 
printing device paths?

Best regards

Heinrich

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