| Message ID | 20221130000320.287976-3-viktor@daynix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | contrib/elf2dmp: Windows Server 2022 support | expand |
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; > } >
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; > > } > >
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); - - 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); - - 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; }
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(-)