Re: [PATCH v1 2/3] contrib/elf2dmp: move PE dir search to pe_get_data_dir_entry

[Viktor Prutyanov]

Hello,

On Wed, Feb 22, 2023 at 10:07 PM Annie.li <annie.li@oracle.com> wrote:
>
> Hello Viktor,
>
> See my following comments inline,
>
> On 11/29/2022 7:03 PM, Viktor Prutyanov wrote:
> > Move out PE directory search functionality to be reused not only
> > for Debug Directory processing but for arbitrary PE directory.
> >
> > Signed-off-by: Viktor Prutyanov <viktor@daynix.com>
> > ---
> >   contrib/elf2dmp/main.c | 66 +++++++++++++++++++++++-------------------
> >   1 file changed, 37 insertions(+), 29 deletions(-)
> >
> > diff --git a/contrib/elf2dmp/main.c b/contrib/elf2dmp/main.c
> > index 9224764239..f3052b3c64 100644
> > --- a/contrib/elf2dmp/main.c
> > +++ b/contrib/elf2dmp/main.c
> > @@ -333,6 +333,40 @@ static int fill_context(KDDEBUGGER_DATA64 *kdbg,
> >       return 0;
> >   }
> >
> > +static int pe_get_data_dir_entry(uint64_t base, void *start_addr, int idx,
> > +        void *entry, size_t size, struct va_space *vs)
> > +{
> > +    const char e_magic[2] = "MZ";
> > +    const char Signature[4] = "PE\0\0";
> > +    IMAGE_DOS_HEADER *dos_hdr = start_addr;
> > +    IMAGE_NT_HEADERS64 nt_hdrs;
> > +    IMAGE_FILE_HEADER *file_hdr = &nt_hdrs.FileHeader;
> > +    IMAGE_OPTIONAL_HEADER64 *opt_hdr = &nt_hdrs.OptionalHeader;
> > +    IMAGE_DATA_DIRECTORY *data_dir = nt_hdrs.OptionalHeader.DataDirectory;
> > +
> > +    if (memcmp(&dos_hdr->e_magic, e_magic, sizeof(e_magic))) {
> > +        return 1;
> > +    }
> > +
> > +    if (va_space_rw(vs, base + dos_hdr->e_lfanew,
> > +                &nt_hdrs, sizeof(nt_hdrs), 0)) {
> > +        return 1;
> > +    }
> > +
> > +    if (memcmp(&nt_hdrs.Signature, Signature, sizeof(Signature)) ||
> > +            file_hdr->Machine != 0x8664 || opt_hdr->Magic != 0x020b) {
> > +        return 1;
> > +    }
> > +
> > +    if (va_space_rw(vs,
> > +                base + data_dir[idx].VirtualAddress,
> > +                entry, size, 0)) {
> > +        return 1;
> > +    }
> > +
> > +    return 0;
> > +}
> > +
> >   static int write_dump(struct pa_space *ps,
> >           WinDumpHeader64 *hdr, const char *name)
> >   {
> > @@ -369,42 +403,16 @@ static int write_dump(struct pa_space *ps,
> >   static int pe_get_pdb_symstore_hash(uint64_t base, void *start_addr,
> >           char *hash, struct va_space *vs)
> >   {
> > -    const char e_magic[2] = "MZ";
> > -    const char Signature[4] = "PE\0\0";
> >       const char sign_rsds[4] = "RSDS";
> > -    IMAGE_DOS_HEADER *dos_hdr = start_addr;
> > -    IMAGE_NT_HEADERS64 nt_hdrs;
> > -    IMAGE_FILE_HEADER *file_hdr = &nt_hdrs.FileHeader;
> > -    IMAGE_OPTIONAL_HEADER64 *opt_hdr = &nt_hdrs.OptionalHeader;
> > -    IMAGE_DATA_DIRECTORY *data_dir = nt_hdrs.OptionalHeader.DataDirectory;
> >       IMAGE_DEBUG_DIRECTORY debug_dir;
> >       OMFSignatureRSDS rsds;
> >       char *pdb_name;
> >       size_t pdb_name_sz;
> >       size_t i;
> >
> > -    QEMU_BUILD_BUG_ON(sizeof(*dos_hdr) >= ELF2DMP_PAGE_SIZE);
>
> This BUG_ON gets removed due to encapsulating the code into function
> pe_get_data_dir_entry.
>
> Any reason of not keeping this check in pe_get_data_dir_entry?
> > -
> > -    if (memcmp(&dos_hdr->e_magic, e_magic, sizeof(e_magic))) {
> > -        return 1;
> > -    }
> > -
> > -    if (va_space_rw(vs, base + dos_hdr->e_lfanew,
> > -                &nt_hdrs, sizeof(nt_hdrs), 0)) {
> > -        return 1;
> > -    }
> > -
> > -    if (memcmp(&nt_hdrs.Signature, Signature, sizeof(Signature)) ||
> > -            file_hdr->Machine != 0x8664 || opt_hdr->Magic != 0x020b) {
> > -        return 1;
> > -    }
> > -
> > -    printf("Debug Directory RVA = 0x%08"PRIx32"\n",
> > -            (uint32_t)data_dir[IMAGE_FILE_DEBUG_DIRECTORY].VirtualAddress);
>
> Or add common log for both Debug and PE directory instead of removing it?

Sounds reasonable, I will send a new version.

Best regards,
Viktor Prutyanov

>
> Thanks
>
> Annie
>
> > -
> > -    if (va_space_rw(vs,
> > -                base + data_dir[IMAGE_FILE_DEBUG_DIRECTORY].VirtualAddress,
> > -                &debug_dir, sizeof(debug_dir), 0)) {
> > +    if (pe_get_data_dir_entry(base, start_addr, IMAGE_FILE_DEBUG_DIRECTORY,
> > +                &debug_dir, sizeof(debug_dir), vs)) {
> > +        eprintf("Failed to get Debug Directory\n");
> >           return 1;
> >       }
> >