Re: [edk2-devel] [PATCH v2 2/3] UefiPayloadPkg: Add PayloadLoaderPeim which can load ELF payload

["Marvin Häuser" <mhaeuser@posteo.de>]

On 10.06.21 05:40, Ni, Ray wrote:
>>> Without the ParseStatus field, callee cannot know whether ParseElfImage()
>> is called.
>>
>> It can by function contracts, the caller guarantees it. I.e. with the PE
>> library I linked, no other function must be called before the init function.
>> Your "ParseElfImage" function is very similar. The context is
>> initialized by it, i.e. it is trash if it is not called, i.e. it must be
>> called before other functions.
>> If it is called, which we know, the caller has the return status. For
>> PE, it means the caller must not proceed with any further PE processing
>> and abort immediately.
>> Is there any scenario where this does not work for ELF? Sorry if I
>> missed something.
> Caller might call LoadElfImage() without firstly calling ParseElfImage() by mistake.
> ParseStatus is added to catch such mistake.

If ParseElfImage() is not called, nothing will initialize ParseStatus 
and the load function will read random data. If AllocateZeroPool was 
used for the context, a common pattern throughout the codebase to harden 
against memory initialization bugs, it would even report success at all 
times anyway. Sorry, but I think this is dead code.

Maybe for some context, my main issue at first was that the checks are 
all proper runtime checks with no ASSERTs at all, so I got confused how 
this situation could happen in a realistic scenario. I needed to trace 
the ParseStatus data flow to understand the idea is basically the same 
as in the PE library. Code in a way is self-documenting, and this 
personally gave me a hard time understanding why it is written this way. 
But thanks for clarifying your intention! :)

Best regards,
Marvin

>
> I don't trust the caller would follow the contracts properly😊.