Re: [PATCH v2 02/11] x86/mkreloc: fix obtaining PE image base address

[Jan Beulich <jbeulich@xxxxxxxx>]

On 10.04.2025 09:20, Roger Pau Monné wrote:
> On Tue, Apr 08, 2025 at 02:34:48PM +0200, Jan Beulich wrote:
>> On 08.04.2025 13:21, Roger Pau Monné wrote:
>>> On Wed, Apr 02, 2025 at 09:46:53AM +0200, Jan Beulich wrote:
>>>> @@ -54,31 +56,40 @@ static unsigned int load(const char *nam
>>>>  
>>>>      if ( lseek(in, mz_hdr.peaddr, SEEK_SET) < 0 ||
>>>>           read(in, &pe_hdr, sizeof(pe_hdr)) != sizeof(pe_hdr) ||
>>>> -         read(in, &pe32_opt_hdr, sizeof(pe32_opt_hdr)) != 
>>>> sizeof(pe32_opt_hdr) ||
>>>> -         read(in, &base, sizeof(base)) != sizeof(base) ||
>>>> -         /*
>>>> -          * Luckily the image size field lives at the
>>>> -          * same offset for both formats.
>>>> -          */
>>>> -         lseek(in, 24, SEEK_CUR) < 0 ||
>>>> -         read(in, image_size, sizeof(*image_size)) != sizeof(*image_size) 
>>>> )
>>>> +         (read(in, &pe32_opt_hdr.pe, sizeof(pe32_opt_hdr.pe)) !=
>>>> +          sizeof(pe32_opt_hdr.pe)) )
>>>>      {
>>>>          perror(name);
>>>>          exit(3);
>>>>      }
>>>>  
>>>>      switch ( (pe_hdr.magic == PE_MAGIC &&
>>>> -              pe_hdr.opt_hdr_size > sizeof(pe32_opt_hdr)) *
>>>> -              pe32_opt_hdr.magic )
>>>> +              pe_hdr.opt_hdr_size > sizeof(pe32_opt_hdr.pe)) *
>>>> +              pe32_opt_hdr.pe.magic )
>>>>      {
>>>>      case PE_OPT_MAGIC_PE32:
>>>>          *width = 32;
>>>> -        *image_base = base;
>>>> +        *image_base = pe32_opt_hdr.pe.image_base;
>>>> +        *image_size = pe32_opt_hdr.pe.image_size;
>>>>          break;
>>>>      case PE_OPT_MAGIC_PE32PLUS:
>>>> -        *width = 64;
>>>> -        *image_base = ((uint64_t)base << 32) | pe32_opt_hdr.data_base;
>>>> -        break;
>>>> +        if ( pe_hdr.opt_hdr_size > sizeof(pe32_opt_hdr.pep) )
>>>> +        {
>>>> +            if ( read(in,
>>>> +                      &pe32_opt_hdr.pe + 1,
>>>> +                      sizeof(pe32_opt_hdr.pep) - sizeof(pe32_opt_hdr.pe)) 
>>>> !=
>>>> +                 sizeof(pe32_opt_hdr.pep) - sizeof(pe32_opt_hdr.pe) )
>>>> +            {
>>>> +                perror(name);
>>>> +                exit(3);
>>>> +            }
>>>> +
>>>> +            *width = 64;
>>>> +            *image_base = pe32_opt_hdr.pep.image_base;
>>>> +            *image_size = pe32_opt_hdr.pep.image_size;
>>>> +            break;
>>>> +        }
>>>
>>> Since you are already refactoring much of this code, won't it be
>>> clearer to fetch the header inside of the switch cases.  So that
>>> there's a single read call for each header type?
>>
>> Except that the switch() itself uses not only pe_hdr, but also
>> pe32_opt_hdr. That could be re-arranged, but I'm a little reluctant to
>> do so.
> 
> Hm, I see, the magic field checked here is in the extended header, so
> we would need to fetch it ahead of the switch in any case.  How
> unhelpful.
> 
> One thing that I find weird about this code is the obfuscation of the
> switch condition, won't it be easier to read as:
> 
> if ( pe_hdr.magic != PE_MAGIC ||
>      pe_hdr.opt_hdr_size < sizeof(pe32_opt_hdr) )
>     fprintf(stderr,
>             "%s: Wrong PE magic or missing optional header\n", name);
>     exit(3);
> }
> 
> switch ( pe32_opt_hdr.magic )
> {
> ...
> 
> I would assume the current arrangement is done as to reuse the
> `default` error label, but IMO that switch condition is too hard to
> parse.

Well, yes, I have a tendency to code things like this to re-use code
where possible, but I (meanwhile) understand many people don't like
the result. Doing this differently would be a separate patch though, I
think. Anyway - to catch the maintainers' attention I guess I'll (re-)
submit the patch outside of this thread.

Jan