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

[Annie.li]

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?

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