[PATCH] gdb: make is_linked_with_cygwin_dll handle import table not at beginning of .idata section

[PATCH] gdb: make is_linked_with_cygwin_dll handle import table not at beginning of .idata section

[Simon Marchi]

When loading the file C:\Windows\SysWOW64\msvcrt.dll, taken from a
Windows 10 system, into GDB, we get the following warning:

    warning: Failed to parse .idata section: name's virtual address (0x0) is outside .idata section's range [0xb82b8, 0xb97f0[.

This uncovers an issue with how we parse the import table, part of the
.idata section.  Right now, we assume that the import table is located
at the beginning of the section.  That was the case in everything I had
tried so far, but this file is an example where that's not true.

We need to compute the offset of the import table within the .idata
section, and start there, instead of at the beginning of the .idata
section.  Using the file mentioned above, this is the values we have to
work with:

  A) bfd_section_vma (idata_section)    101b8000
  B) Import table's virtual address        b82b8
  C) Image base                         10100000

The virtual address that BFD returns us for the section has the image
base applied, so we need to subtract it first.  The offset of the table
in the section is therefore:

    B - (A - C)

This patch implements that.

gdb/ChangeLog:

	windows-tdep.c (is_linked_with_cygwin_dll): Consider case where
	import table is not at beginning of .idata section.
---
 gdb/windows-tdep.c | 38 +++++++++++++++++++++++++-------------
 1 file changed, 25 insertions(+), 13 deletions(-)

diff --git a/gdb/windows-tdep.c b/gdb/windows-tdep.c
index d1894ca08884..fa5b2fc9c055 100644
--- a/gdb/windows-tdep.c
+++ b/gdb/windows-tdep.c
@@ -999,11 +999,22 @@ is_linked_with_cygwin_dll (bfd *abfd)
   if (idata_section == nullptr)
     return false;
 
-  /* Find the virtual address of the .idata section.  We must subtract this
-     from the RVAs (relative virtual addresses) to obtain an offset in the
-     section. */
-  bfd_vma idata_addr
-    = pe_data (abfd)->pe_opthdr.DataDirectory[PE_IMPORT_TABLE].VirtualAddress;
+  internal_extra_pe_aouthdr *pe_extra = &pe_data (abfd)->pe_opthdr;
+  bfd_vma import_table_va = pe_extra->DataDirectory[PE_IMPORT_TABLE].VirtualAddress;
+  bfd_vma idata_section_va = bfd_section_vma (idata_section);
+  bfd_size_type idata_section_size = bfd_section_size (idata_section);
+
+  /* The section's virtual address as reported by BFD has the image base applied,
+     remove it.  */
+  gdb_assert (idata_section_va >= pe_extra->ImageBase);
+  idata_section_va -= pe_extra->ImageBase;
+
+  /* Assert that the import table is indeed within the .idata section's range.  */
+  gdb_assert (import_table_va >= idata_section_va);
+  gdb_assert (import_table_va < (idata_section_va + idata_section_size));
+
+  /* The import table starts at this offset into the .idata section.  */
+  bfd_vma import_table_offset_in_sect = import_table_va - idata_section_va;
 
   /* Get the section's data.  */
   gdb::byte_vector idata_contents;
@@ -1013,9 +1024,10 @@ is_linked_with_cygwin_dll (bfd *abfd)
       return false;
     }
 
-  size_t idata_size = idata_contents.size ();
-  const gdb_byte *iter = idata_contents.data ();
-  const gdb_byte *end = idata_contents.data () + idata_size;
+  gdb_assert (idata_contents.size () == idata_section_size);
+
+  const gdb_byte *iter = idata_contents.data () + import_table_offset_in_sect;
+  const gdb_byte *end = idata_contents.data () + idata_section_size;
   const pe_import_directory_entry null_dir_entry = { 0 };
 
   /* Iterate through all directory entries.  */
@@ -1036,21 +1048,21 @@ is_linked_with_cygwin_dll (bfd *abfd)
 		  sizeof (pe_import_directory_entry)) == 0)
 	break;
 
-      bfd_vma name_addr = dir_entry->name_rva;
+      bfd_vma name_va = dir_entry->name_rva;
 
       /* If the name's virtual address is smaller than the section's virtual
          address, there's a problem.  */
-      if (name_addr < idata_addr
-	  || name_addr >= (idata_addr + idata_size))
+      if (name_va < idata_section_va
+	  || name_va >= (idata_section_va + idata_section_size))
 	{
 	  warning (_("\
 Failed to parse .idata section: name's virtual address (0x%" BFD_VMA_FMT "x) \
 is outside .idata section's range [0x%" BFD_VMA_FMT "x, 0x%" BFD_VMA_FMT "x[."),
-		   name_addr, idata_addr, idata_addr + idata_size);
+		   name_va, idata_section_va, idata_section_va + idata_section_size);
 	  break;
 	}
 
-      const gdb_byte *name = &idata_contents[name_addr - idata_addr];
+      const gdb_byte *name = &idata_contents[name_va - idata_section_va];
 
       /* Make sure we don't overshoot the end of the section with the streq.  */
       if (name + sizeof (CYGWIN_DLL_NAME) > end)
-- 
2.26.0

Re: [PATCH] gdb: make is_linked_with_cygwin_dll handle import table not at beginning of .idata section

[Pedro Alves]

On 4/16/20 4:45 PM, Simon Marchi via Gdb-patches wrote:
> +  /* The section's virtual address as reported by BFD has the image base applied,
> +     remove it.  */
> +  gdb_assert (idata_section_va >= pe_extra->ImageBase);
> +  idata_section_va -= pe_extra->ImageBase;
> +
> +  /* Assert that the import table is indeed within the .idata section's range.  */
> +  gdb_assert (import_table_va >= idata_section_va);
> +  gdb_assert (import_table_va < (idata_section_va + idata_section_size));

Couldn't these be triggered with malformed files?  I.e., seems like an assertion
is too strong, and we should maybe warn and return false instead.

Thanks,
Pedro Alves

Re: [PATCH] gdb: make is_linked_with_cygwin_dll handle import table not at beginning of .idata section

[Simon Marchi]

On 2020-04-16 12:27 p.m., Pedro Alves wrote:
> On 4/16/20 4:45 PM, Simon Marchi via Gdb-patches wrote:
>> +  /* The section's virtual address as reported by BFD has the image base applied,
>> +     remove it.  */
>> +  gdb_assert (idata_section_va >= pe_extra->ImageBase);
>> +  idata_section_va -= pe_extra->ImageBase;
>> +
>> +  /* Assert that the import table is indeed within the .idata section's range.  */
>> +  gdb_assert (import_table_va >= idata_section_va);
>> +  gdb_assert (import_table_va < (idata_section_va + idata_section_size));
> 
> Couldn't these be triggered with malformed files?  I.e., seems like an assertion
> is too strong, and we should maybe warn and return false instead.

Hmm yes, that's right, I'll fix it.

Simon

[PATCH v2 1/2] gdb: make is_linked_with_cygwin_dll handle import table not at beginning of .idata section

[Simon Marchi]

When loading the file C:\Windows\SysWOW64\msvcrt.dll, taken from a
Windows 10 system, into GDB, we get the following warning:

    warning: Failed to parse .idata section: name's virtual address (0x0) is outside .idata section's range [0xb82b8, 0xb97f0[.

This uncovers an issue with how we parse the import table, part of the
.idata section.  Right now, we assume that the import table is located
at the beginning of the section.  That was the case in everything I had
tried so far, but this file is an example where that's not true.

We need to compute the offset of the import table within the .idata
section, and start there, instead of at the beginning of the .idata
section.  Using the file mentioned above, this is the values we have to
work with:

  A) bfd_section_vma (idata_section)    101b8000
  B) Import table's virtual address        b82b8
  C) Image base                         10100000

The virtual address that BFD returns us for the section has the image
base applied, so we need to subtract it first.  The offset of the table
in the section is therefore:

    B - (A - C)

This patch implements that.

gdb/ChangeLog:

	windows-tdep.c (is_linked_with_cygwin_dll): Consider case where
	import table is not at beginning of .idata section.

---

New in v2:

 - Display warning and return false if the import table address does not
   fall within the .idata section range.
---
 gdb/windows-tdep.c | 47 +++++++++++++++++++++++++++++++++-------------
 1 file changed, 34 insertions(+), 13 deletions(-)

diff --git a/gdb/windows-tdep.c b/gdb/windows-tdep.c
index d1894ca08884..50bb9591f931 100644
--- a/gdb/windows-tdep.c
+++ b/gdb/windows-tdep.c
@@ -999,11 +999,32 @@ is_linked_with_cygwin_dll (bfd *abfd)
   if (idata_section == nullptr)
     return false;
 
-  /* Find the virtual address of the .idata section.  We must subtract this
-     from the RVAs (relative virtual addresses) to obtain an offset in the
-     section. */
-  bfd_vma idata_addr
-    = pe_data (abfd)->pe_opthdr.DataDirectory[PE_IMPORT_TABLE].VirtualAddress;
+  bfd_size_type idata_section_size = bfd_section_size (idata_section);
+  internal_extra_pe_aouthdr *pe_extra = &pe_data (abfd)->pe_opthdr;
+  bfd_vma import_table_va = pe_extra->DataDirectory[PE_IMPORT_TABLE].VirtualAddress;
+  bfd_vma idata_section_va = bfd_section_vma (idata_section);
+
+  /* The section's virtual address as reported by BFD has the image base applied,
+     remove it.  */
+  gdb_assert (idata_section_va >= pe_extra->ImageBase);
+  idata_section_va -= pe_extra->ImageBase;
+
+  bfd_vma idata_section_end_va = idata_section_va + idata_section_size;
+
+  /* Make sure that the import table is indeed within the .idata section's range.  */
+  if (import_table_va < idata_section_va
+      || import_table_va >= idata_section_end_va)
+    {
+      warning (_("\
+%s: import table's virtual address (0x%" BFD_VMA_FMT "x) is outside .idata \
+section's range [0x%" BFD_VMA_FMT "x, 0x%" BFD_VMA_FMT "x[."),
+	       bfd_get_filename (abfd), import_table_va, idata_section_va,
+	       idata_section_end_va);
+      return false;
+    }
+
+  /* The import table starts at this offset into the .idata section.  */
+  bfd_vma import_table_offset_in_sect = import_table_va - idata_section_va;
 
   /* Get the section's data.  */
   gdb::byte_vector idata_contents;
@@ -1013,9 +1034,10 @@ is_linked_with_cygwin_dll (bfd *abfd)
       return false;
     }
 
-  size_t idata_size = idata_contents.size ();
-  const gdb_byte *iter = idata_contents.data ();
-  const gdb_byte *end = idata_contents.data () + idata_size;
+  gdb_assert (idata_contents.size () == idata_section_size);
+
+  const gdb_byte *iter = idata_contents.data () + import_table_offset_in_sect;
+  const gdb_byte *end = idata_contents.data () + idata_section_size;
   const pe_import_directory_entry null_dir_entry = { 0 };
 
   /* Iterate through all directory entries.  */
@@ -1036,21 +1058,20 @@ is_linked_with_cygwin_dll (bfd *abfd)
 		  sizeof (pe_import_directory_entry)) == 0)
 	break;
 
-      bfd_vma name_addr = dir_entry->name_rva;
+      bfd_vma name_va = dir_entry->name_rva;
 
       /* If the name's virtual address is smaller than the section's virtual
          address, there's a problem.  */
-      if (name_addr < idata_addr
-	  || name_addr >= (idata_addr + idata_size))
+      if (name_va < idata_section_va || name_va >= idata_section_end_va)
 	{
 	  warning (_("\
 Failed to parse .idata section: name's virtual address (0x%" BFD_VMA_FMT "x) \
 is outside .idata section's range [0x%" BFD_VMA_FMT "x, 0x%" BFD_VMA_FMT "x[."),
-		   name_addr, idata_addr, idata_addr + idata_size);
+		   name_va, idata_section_va, idata_section_end_va);
 	  break;
 	}
 
-      const gdb_byte *name = &idata_contents[name_addr - idata_addr];
+      const gdb_byte *name = &idata_contents[name_va - idata_section_va];
 
       /* Make sure we don't overshoot the end of the section with the streq.  */
       if (name + sizeof (CYGWIN_DLL_NAME) > end)
-- 
2.26.0

[PATCH v2 2/2] gdb: is_linked_with_cygwin_dll: mention filename in warning messages

[Simon Marchi]

When a warning is displayed, it isn't clear to the user which file is
the cause of the warning.  Add the filename in there.  Remove the
"Failed to parse .idata section" part, since the .idata section is
always mentioned one way or another anyway, so it just contributes to
make the message longer than it needs to be.

gdb/ChangeLog:

	* windows-tdep.c (is_linked_with_cygwin_dll): Add filename to
	warning messages.
---
 gdb/windows-tdep.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/gdb/windows-tdep.c b/gdb/windows-tdep.c
index 50bb9591f931..13eaf8f1cabd 100644
--- a/gdb/windows-tdep.c
+++ b/gdb/windows-tdep.c
@@ -1030,7 +1030,8 @@ section's range [0x%" BFD_VMA_FMT "x, 0x%" BFD_VMA_FMT "x[."),
   gdb::byte_vector idata_contents;
   if (!gdb_bfd_get_full_section_contents (abfd, idata_section, &idata_contents))
     {
-      warning (_("Failed to get content of .idata section."));
+      warning (_("%s: failed to get contents of .idata section."),
+	       bfd_get_filename (abfd));
       return false;
     }
 
@@ -1046,8 +1047,8 @@ section's range [0x%" BFD_VMA_FMT "x, 0x%" BFD_VMA_FMT "x[."),
       /* Is there enough space left in the section for another entry?  */
       if (iter + sizeof (pe_import_directory_entry) > end)
 	{
-	  warning (_("Failed to parse .idata section: unexpected end of "
-		     ".idata section."));
+	  warning (_("%s: unexpected end of .idata section."),
+		   bfd_get_filename (abfd));
 	  break;
 	}
 
@@ -1065,9 +1066,10 @@ section's range [0x%" BFD_VMA_FMT "x, 0x%" BFD_VMA_FMT "x[."),
       if (name_va < idata_section_va || name_va >= idata_section_end_va)
 	{
 	  warning (_("\
-Failed to parse .idata section: name's virtual address (0x%" BFD_VMA_FMT "x) \
-is outside .idata section's range [0x%" BFD_VMA_FMT "x, 0x%" BFD_VMA_FMT "x[."),
-		   name_va, idata_section_va, idata_section_end_va);
+%s: name's virtual address (0x%" BFD_VMA_FMT "x) is outside .idata section's \
+range [0x%" BFD_VMA_FMT "x, 0x%" BFD_VMA_FMT "x[."),
+		   bfd_get_filename (abfd), name_va, idata_section_va,
+		   idata_section_end_va);
 	  break;
 	}
 
-- 
2.26.0

Re: [PATCH v2 1/2] gdb: make is_linked_with_cygwin_dll handle import table not at beginning of .idata section

[Pedro Alves]

On 4/16/20 7:47 PM, Simon Marchi via Gdb-patches wrote:
> When loading the file C:\Windows\SysWOW64\msvcrt.dll, taken from a
> Windows 10 system, into GDB, we get the following warning:
> 
>     warning: Failed to parse .idata section: name's virtual address (0x0) is outside .idata section's range [0xb82b8, 0xb97f0[.
> 
> This uncovers an issue with how we parse the import table, part of the
> .idata section.  Right now, we assume that the import table is located
> at the beginning of the section.  That was the case in everything I had
> tried so far, but this file is an example where that's not true.
> 
> We need to compute the offset of the import table within the .idata
> section, and start there, instead of at the beginning of the .idata
> section.  Using the file mentioned above, this is the values we have to
> work with:
> 
>   A) bfd_section_vma (idata_section)    101b8000
>   B) Import table's virtual address        b82b8
>   C) Image base                         10100000
> 
> The virtual address that BFD returns us for the section has the image
> base applied, so we need to subtract it first.  The offset of the table
> in the section is therefore:
> 
>     B - (A - C)
> 
> This patch implements that.
> 
> gdb/ChangeLog:
> 
> 	windows-tdep.c (is_linked_with_cygwin_dll): Consider case where
> 	import table is not at beginning of .idata section.

Note the missing leading '*'.

This version looks good to me.  Both patches.

Thanks,
Pedro Alves

Re: [PATCH v2 1/2] gdb: make is_linked_with_cygwin_dll handle import table not at beginning of .idata section

[Simon Marchi]

On 2020-04-16 3:21 p.m., Pedro Alves wrote:
> On 4/16/20 7:47 PM, Simon Marchi via Gdb-patches wrote:
>> When loading the file C:\Windows\SysWOW64\msvcrt.dll, taken from a
>> Windows 10 system, into GDB, we get the following warning:
>>
>>     warning: Failed to parse .idata section: name's virtual address (0x0) is outside .idata section's range [0xb82b8, 0xb97f0[.
>>
>> This uncovers an issue with how we parse the import table, part of the
>> .idata section.  Right now, we assume that the import table is located
>> at the beginning of the section.  That was the case in everything I had
>> tried so far, but this file is an example where that's not true.
>>
>> We need to compute the offset of the import table within the .idata
>> section, and start there, instead of at the beginning of the .idata
>> section.  Using the file mentioned above, this is the values we have to
>> work with:
>>
>>   A) bfd_section_vma (idata_section)    101b8000
>>   B) Import table's virtual address        b82b8
>>   C) Image base                         10100000
>>
>> The virtual address that BFD returns us for the section has the image
>> base applied, so we need to subtract it first.  The offset of the table
>> in the section is therefore:
>>
>>     B - (A - C)
>>
>> This patch implements that.
>>
>> gdb/ChangeLog:
>>
>> 	windows-tdep.c (is_linked_with_cygwin_dll): Consider case where
>> 	import table is not at beginning of .idata section.
> 
> Note the missing leading '*'.

Oops, fixed.

> This version looks good to me.  Both patches.

Both patches pushed, thanks.

Simon