On 11/21/2017 03:16 PM, Wolfgang Denk wrote: > Dear Heinrich Schuchardt, > > In message <d643b22b-2511-9b40-772b-5f7f4486c...@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 > _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
