Hello,
On Wed, Feb 22, 2023 at 10:07 PM Annie.li <annie...@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 <vik...@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;
> > }
> >
