This is the mail archive of the binutils@sourceware.org mailing list for the binutils project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

[PATCH]: Fix for broken PE auto-importing.


Hi all,

Refering to the thread at:
http://sourceware.org/ml/binutils/2006-09/msg00119.html

Kai Tietz wrote:
> Hallo,
>
> I found a memory violation in the function "make_singleton_name_thunk" of
> pe_dll.c file. There is allocated a heap buffer of 4 bytes and afterwards
> memset this pointer with length of 8 bytes,
>
> --- src/ld/pe-dll.c 2006-08-21 10:12:46.000000000 +0200
> +++ src_n/ld/pe-dll.c 2006-09-15 12:07:39.000000000 +0200
> @@ -2036,7 +2036,7 @@
> quick_symbol (abfd, U ("_nm_"), import, "", UNDSEC, BSF_GLOBAL, 0);
> > bfd_set_section_size (abfd, id4, 8);
> - d4 = xmalloc (4);
> + d4 = xmalloc (8);
> id4->contents = d4;
> memset (d4, 0, 8);
> quick_reloc (abfd, 0, BFD_RELOC_RVA, 2);
>
>
> Regards,
> i.A. Kai Tietz
>
>


> PS: This piece of code brought me to the question, why this thunk gets an
> empty one plus ?
>
>
>


Its was a required NULL terminator.

Kai Tietz also wrote:
> Hallo Dave,
>
> I meant by an empty one the fact, that the idata4 element (called thumb)
> has under i(3456)86 PE a size of 4 bytes. Therefore, if there are stored 8
> bytes - as it is - it means there is an additional empty (zero) thumb
> present.
>


Right, but it is required.

> In the patch for PE+ for x86_64 I modified this code to the size of one
> thumb element and I didn't found any problems for PE. In my pending patch,
> I replaced the IDATA4 and IDATA5 sizes by constances, because it is ugly
> to read.
>
> Regards,
> i.A. Kai Tietz
>
> PS: Under the x86_64 target one thumb element gets really the size of 8
> bytes.
>
>
>


But it was removed with the pe+ support patch (PE_IDATA4_SIZE == 4 in 32bit archs):

-  bfd_set_section_size (abfd, id4, 8);
-  d4 = xmalloc (4);
+  bfd_set_section_size (abfd, id4, PE_IDATA4_SIZE);
+  d4 = xmalloc (PE_IDATA4_SIZE);
  id4->contents = d4;
-  memset (d4, 0, 8);
+  memset (d4, 0, PE_IDATA4_SIZE);
  quick_reloc (abfd, 0, BFD_RELOC_RVA, 2);
  save_relocs (id4);

bfd_set_symtab (abfd, symtab, symptr);

-  bfd_set_section_contents (abfd, id4, d4, 0, 8);
+  bfd_set_section_contents (abfd, id4, d4, 0, PE_IDATA4_SIZE);

-----

Refer to this comment from dlltool.c:

  .idata$4 = Import Lookup Table
  = array of array of pointers to hint name table.
  There is one for each dll being imported from, and each dll's set is
  terminated by a trailing NULL.

The extra 4 bytes where that trailing NULL.

The effect of removing it is something like this:
This is the dump of the import table for the auto-import thunk. It
should only contain one entry, the auto import of appUniqueID ...

    DLL Name: QtCored4.dll
       vma:  Hint/Ord Member-Name Bound-To
       5d549c   2701  appUniqueID
       5d54cc   2705  qt_cever
       80000300          768  <none>
       80000575         1397  <none>
       80000417         1047  <none>
       800002ee          750  <none>
       (snip a couple hundred broken imports.)

... like this, with attached patch applied:
       DLL Name: QtCored4.dll
       vma:  Hint/Ord Member-Name Bound-To
       5d54a4   2701  appUniqueID

--

This can only be triggered in some combinations of mixing MSFT's import libs
with ld generated ones, that end up placing the singleton thunk name entry in the middle of idata$4.
If you are lucky, they are placed at the end of idata$4, and nothing bad happens, as the loader must
be checking for the end of the Import Lookup Table.


With WinCE I had an app refusing to load with the nasty 193L
(ERROR_BAD_EXE_FORMAT) error.
I didn't try reproducing with Cygwin or MinGW, but the error should be the same.


Attached is a patch to fix it. Please review and commit.

Cheers,
Pedro Alves

---
ld/ChangeLog

2006-10-29 Pedro Alves <pedro_alves@portugalmail.pt>

* pe-dll.c (make_singleton_name_thunk): Re-add the NULL terminator.


Index: pe-dll.c
===================================================================
RCS file: /cvs/src/src/ld/pe-dll.c,v
retrieving revision 1.90
diff -u -p -r1.90 pe-dll.c
--- pe-dll.c	3 Oct 2006 10:06:26 -0000	1.90
+++ pe-dll.c	29 Oct 2006 15:57:21 -0000
@@ -2080,16 +2080,17 @@ make_singleton_name_thunk (const char *i
   quick_symbol (abfd, U ("_nm_thnk_"), import, "", id4, BSF_GLOBAL, 0);
   quick_symbol (abfd, U ("_nm_"), import, "", UNDSEC, BSF_GLOBAL, 0);
 
-  bfd_set_section_size (abfd, id4, PE_IDATA4_SIZE);
-  d4 = xmalloc (PE_IDATA4_SIZE);
+  /* We need space for the real thunk and for the null terminator.  */
+  bfd_set_section_size (abfd, id4, PE_IDATA4_SIZE * 2);
+  d4 = xmalloc (PE_IDATA4_SIZE * 2);
   id4->contents = d4;
-  memset (d4, 0, PE_IDATA4_SIZE);
+  memset (d4, 0, PE_IDATA4_SIZE * 2);
   quick_reloc (abfd, 0, BFD_RELOC_RVA, 2);
   save_relocs (id4);
 
   bfd_set_symtab (abfd, symtab, symptr);
 
-  bfd_set_section_contents (abfd, id4, d4, 0, PE_IDATA4_SIZE);
+  bfd_set_section_contents (abfd, id4, d4, 0, PE_IDATA4_SIZE * 2);
 
   bfd_make_readable (abfd);
   return abfd;

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]