Re: LD: Invalid memory access when linking a dll (maybe Bug 29006 revisited)

[Alan Modra <amodra@gmail.com>]

On Tue, May 10, 2022 at 07:19:03PM +0200, Roland Schwingel wrote:
> ==23381== Invalid read of size 1
> ==23381==    at 0x508B434: vfprintf (in /lib64/libc-2.17.so)
> ==23381==    by 0x50B3E63: vasprintf (in /lib64/libc-2.17.so)
> ==23381==    by 0x50912F6: asprintf (in /lib64/libc-2.17.so)
> ==23381==    by 0x4432CE: make_runtime_pseudo_reloc (pe-dll.c:2663)
> ==23381==    by 0x443A81: pep_create_import_fixup (pe-dll.c:2838)
> ==23381==    by 0x432CA6: make_import_fixup (ei386pep.c:1129)
> ==23381==    by 0x43F8A5: pe_walk_relocs (pe-dll.c:1349)
> ==23381==    by 0x43FD95: pep_find_data_imports (pe-dll.c:1497)
> ==23381==    by 0x433674: gld_i386pep_after_open (ei386pep.c:1408)

OK, so we are running pep_find_data_imports and happen to call
make_runtime_pseudo_reloc while inpecting relocs.
make_runtime_pseudo_reloc uses dll_symname, which has been cleared

> ==23381==    by 0x428FCB: ldemul_after_open (ldemul.c:65)
> ==23381==    by 0x41D9F2: lang_process (ldlang.c:8162)
> ==23381==    by 0x422440: main (ldmain.c:497)
> ==23381==  Address 0x95e2500 is 0 bytes inside a block of size 20 free'd
> ==23381==    at 0x4C2E10B: free (vg_replace_malloc.c:871)
> ==23381==    by 0x445199: pep_process_import_defs (pe-dll.c:3324)

after finishing pep_process_import_defs.  These two functions are
called in close proximity.  See emultempl/pep.em.  My guess is that
this broke with commit c4a8df19ba0a.

> ==23381==    by 0x433648: gld_i386pep_after_open (ei386pep.c:1405)
> ==23381==    by 0x428FCB: ldemul_after_open (ldemul.c:65)
> ==23381==    by 0x41D9F2: lang_process (ldlang.c:8162)
> ==23381==    by 0x422440: main (ldmain.c:497)

Looks like we need to set up dll_symname earlier now.  Please try
this patch.

	PR 29006
	* pe-dll.c (dll_name): Delete, replacing with..
	(dll_filename): ..this, moved earlier in file.
	(generate_edata): Delete parameters.  Don't set up dll_name here..
	(pe_process_import_defs): ..instead set up dll_filename and
	dll_symname here before returning.
	(dll_symname_len): Delete write-only variable.
	(pe_dll_generate_implib): Don't set up dll_symname here.

diff --git a/ld/pe-dll.c b/ld/pe-dll.c
index ed68f66a95b..4cf8ed23672 100644
--- a/ld/pe-dll.c
+++ b/ld/pe-dll.c
@@ -505,7 +505,7 @@ static int export_table_size;
 static int count_exported;
 static int count_exported_byname;
 static int count_with_ordinals;
-static const char *dll_name;
+static const char *dll_filename;
 static int min_ordinal, max_ordinal;
 static int *exported_symbols;
 
@@ -1066,25 +1066,13 @@ build_filler_bfd (int include_edata)
 /* Gather all the exported symbols and build the .edata section.  */
 
 static void
-generate_edata (bfd *abfd, struct bfd_link_info *info ATTRIBUTE_UNUSED)
+generate_edata (void)
 {
   int i, next_ordinal;
   int name_table_size = 0;
-  const char *dlnp;
 
   /* First, we need to know how many exported symbols there are,
      and what the range of ordinals is.  */
-  if (pe_def_file->name)
-    dll_name = pe_def_file->name;
-  else
-    {
-      dll_name = bfd_get_filename (abfd);
-
-      for (dlnp = dll_name; *dlnp; dlnp++)
-	if (*dlnp == '\\' || *dlnp == '/' || *dlnp == ':')
-	  dll_name = dlnp + 1;
-    }
-
   if (count_with_ordinals && max_ordinal > count_exported)
     {
       if (min_ordinal > max_ordinal - count_exported + 1)
@@ -1159,7 +1147,7 @@ generate_edata (bfd *abfd, struct bfd_link_info *info ATTRIBUTE_UNUSED)
 	      + 4 * export_table_size		/* addresses */
 	      + 4 * count_exported_byname	/* name ptrs */
 	      + 2 * count_exported_byname	/* ordinals */
-	      + name_table_size + strlen (dll_name) + 1);
+	      + name_table_size + strlen (dll_filename) + 1);
 }
 
 /* Fill the exported symbol offsets. The preliminary work has already
@@ -1232,7 +1220,7 @@ fill_edata (bfd *abfd, struct bfd_link_info *info ATTRIBUTE_UNUSED)
     }
 
   bfd_put_32 (abfd, ERVA (enamestr), edata_d + 12);
-  strcpy (enamestr, dll_name);
+  strcpy (enamestr, dll_filename);
   enamestr += strlen (enamestr) + 1;
   bfd_put_32 (abfd, min_ordinal, edata_d + 16);
   bfd_put_32 (abfd, export_table_size, edata_d + 20);
@@ -1971,9 +1959,7 @@ pe_dll_generate_def_file (const char *pe_out_def_filename)
 static asymbol **symtab;
 static int symptr;
 static int tmp_seq;
-static const char *dll_filename;
 static char *dll_symname;
-static int dll_symname_len;
 
 #define UNDSEC bfd_und_section_ptr
 
@@ -2835,7 +2821,8 @@ pe_create_import_fixup (arelent *rel, asection *s, bfd_vma addend, char *name,
 	printf ("creating runtime pseudo-reloc entry for %s (addend=%d)\n",
 		fixup_name, (int) addend);
 
-      b = make_runtime_pseudo_reloc (name, fixup_name, addend, rel->howto->bitsize,
+      b = make_runtime_pseudo_reloc (name, fixup_name, addend,
+				     rel->howto->bitsize,
 				     link_info.output_bfd);
       add_bfd_to_link (b, bfd_get_filename (b), &link_info);
 
@@ -2861,13 +2848,6 @@ pe_dll_generate_implib (def_file *def, const char *impfilename, struct bfd_link_
   bfd *ibfd;
   bfd *head = 0;
 
-  dll_filename = (def->name) ? def->name : dll_name;
-  dll_symname = xstrdup (dll_filename);
-  dll_symname_len = strlen (dll_symname);
-  for (i = 0; dll_symname[i]; i++)
-    if (!ISALNUM (dll_symname[i]))
-      dll_symname[i] = '_';
-
   unlink_if_ordinary (impfilename);
 
   outarch = bfd_openw (impfilename, 0);
@@ -2995,8 +2975,7 @@ pe_dll_generate_implib (def_file *def, const char *impfilename, struct bfd_link_
 	  }
       }
 
-      n = make_one (def->exports + i, outarch,
-		    ! (def->exports + i)->flag_data);
+      n = make_one (def->exports + i, outarch, !(def->exports + i)->flag_data);
       n->archive_next = head;
       head = n;
       def->exports[i].internal_name = internal;
@@ -3208,128 +3187,145 @@ add_bfd_to_link (bfd *abfd, const char *name, struct bfd_link_info *linfo)
 void
 pe_process_import_defs (bfd *output_bfd, struct bfd_link_info *linfo)
 {
-  int i, j;
-  def_file_module *module;
-  def_file_import *imp;
-
   pe_dll_id_target (bfd_get_target (output_bfd));
 
-  if (!pe_def_file)
-    return;
-
-  imp = pe_def_file->imports;
-
-  pe_create_undef_table ();
-
-  for (module = pe_def_file->modules; module; module = module->next)
+  if (pe_def_file)
     {
-      int do_this_dll = 0;
+      int i, j;
+      def_file_module *module;
+      def_file_import *imp;
 
-      for (i = 0; i < pe_def_file->num_imports && imp[i].module != module; i++)
-	;
-      if (i >= pe_def_file->num_imports)
-	continue;
+      imp = pe_def_file->imports;
 
-      dll_filename = module->name;
-      dll_symname = xstrdup (module->name);
-      dll_symname_len = strlen (dll_symname);
-      for (j = 0; dll_symname[j]; j++)
-	if (!ISALNUM (dll_symname[j]))
-	  dll_symname[j] = '_';
+      pe_create_undef_table ();
 
-      for (; i < pe_def_file->num_imports && imp[i].module == module; i++)
+      for (module = pe_def_file->modules; module; module = module->next)
 	{
-	  def_file_export exp;
-	  struct bfd_link_hash_entry *blhe;
-	  int lead_at = (*imp[i].internal_name == '@');
-	  /* See if we need this import.  */
-	  size_t len = strlen (imp[i].internal_name);
-	  char *name = xmalloc (len + 2 + 6);
-	  bool include_jmp_stub = false;
-	  bool is_cdecl = false;
-	  bool is_undef = false;
-
-	  if (!lead_at && strchr (imp[i].internal_name, '@') == NULL)
-	      is_cdecl = true;
-
-	  if (lead_at)
-	    sprintf (name, "%s", imp[i].internal_name);
-	  else
-	    sprintf (name, "%s%s",U (""), imp[i].internal_name);
+	  int do_this_dll = 0;
 
-	  blhe = bfd_link_hash_lookup (linfo->hash, name,
-				       false, false, false);
+	  for (i = 0; i < pe_def_file->num_imports; i++)
+	    if (imp[i].module == module)
+	      break;
+	  if (i >= pe_def_file->num_imports)
+	    continue;
+
+	  dll_filename = module->name;
+	  dll_symname = xstrdup (module->name);
+	  for (j = 0; dll_symname[j]; j++)
+	    if (!ISALNUM (dll_symname[j]))
+	      dll_symname[j] = '_';
 
-	  /* Include the jump stub for <sym> only if the <sym>
-	     is undefined.  */
-	  if (!blhe || (blhe && blhe->type != bfd_link_hash_undefined))
+	  for (; i < pe_def_file->num_imports && imp[i].module == module; i++)
 	    {
+	      def_file_export exp;
+	      struct bfd_link_hash_entry *blhe;
+	      int lead_at = (*imp[i].internal_name == '@');
+	      /* See if we need this import.  */
+	      size_t len = strlen (imp[i].internal_name);
+	      char *name = xmalloc (len + 2 + 6);
+	      bool include_jmp_stub = false;
+	      bool is_cdecl = false;
+	      bool is_undef = false;
+
+	      if (!lead_at && strchr (imp[i].internal_name, '@') == NULL)
+		is_cdecl = true;
+
 	      if (lead_at)
-		sprintf (name, "%s%s", "__imp_", imp[i].internal_name);
+		sprintf (name, "%s", imp[i].internal_name);
 	      else
-		sprintf (name, "%s%s%s", "__imp_", U (""),
-			 imp[i].internal_name);
+		sprintf (name, "%s%s",U (""), imp[i].internal_name);
 
 	      blhe = bfd_link_hash_lookup (linfo->hash, name,
 					   false, false, false);
-	      if (blhe)
-		is_undef = (blhe->type == bfd_link_hash_undefined);
-	    }
-	  else
-	    {
-	      include_jmp_stub = true;
-	      is_undef = (blhe->type == bfd_link_hash_undefined);
-	    }
 
-	  if (is_cdecl && (!blhe || (blhe && blhe->type != bfd_link_hash_undefined)))
-	    {
-	      sprintf (name, "%s%s",U (""), imp[i].internal_name);
-	      blhe = pe_find_cdecl_alias_match (linfo, name);
-	      include_jmp_stub = true;
-	      if (blhe)
-		is_undef = (blhe->type == bfd_link_hash_undefined);
-	    }
+	      /* Include the jump stub for <sym> only if the <sym>
+		 is undefined.  */
+	      if (!blhe || (blhe && blhe->type != bfd_link_hash_undefined))
+		{
+		  if (lead_at)
+		    sprintf (name, "%s%s", "__imp_", imp[i].internal_name);
+		  else
+		    sprintf (name, "%s%s%s", "__imp_", U (""),
+			     imp[i].internal_name);
+
+		  blhe = bfd_link_hash_lookup (linfo->hash, name,
+					       false, false, false);
+		  if (blhe)
+		    is_undef = (blhe->type == bfd_link_hash_undefined);
+		}
+	      else
+		{
+		  include_jmp_stub = true;
+		  is_undef = (blhe->type == bfd_link_hash_undefined);
+		}
+
+	      if (is_cdecl
+		  && (!blhe || (blhe && blhe->type != bfd_link_hash_undefined)))
+		{
+		  sprintf (name, "%s%s",U (""), imp[i].internal_name);
+		  blhe = pe_find_cdecl_alias_match (linfo, name);
+		  include_jmp_stub = true;
+		  if (blhe)
+		    is_undef = (blhe->type == bfd_link_hash_undefined);
+		}
 
-	  free (name);
+	      free (name);
 
-	  if (is_undef)
-	    {
-	      bfd *one;
-	      /* We do.  */
-	      if (!do_this_dll)
+	      if (is_undef)
 		{
-		  bfd *ar_head = make_head (output_bfd);
-		  add_bfd_to_link (ar_head, bfd_get_filename (ar_head), linfo);
-		  do_this_dll = 1;
+		  bfd *one;
+		  /* We do.  */
+		  if (!do_this_dll)
+		    {
+		      bfd *ar_head = make_head (output_bfd);
+		      add_bfd_to_link (ar_head, bfd_get_filename (ar_head),
+				       linfo);
+		      do_this_dll = 1;
+		    }
+		  exp.internal_name = imp[i].internal_name;
+		  exp.name = imp[i].name;
+		  exp.its_name = imp[i].its_name;
+		  exp.ordinal = imp[i].ordinal;
+		  exp.hint = exp.ordinal >= 0 ? exp.ordinal : 0;
+		  exp.flag_private = 0;
+		  exp.flag_constant = 0;
+		  exp.flag_data = imp[i].data;
+		  exp.flag_noname = exp.name ? 0 : 1;
+		  one = make_one (&exp, output_bfd,
+				  !exp.flag_data && include_jmp_stub);
+		  add_bfd_to_link (one, bfd_get_filename (one), linfo);
 		}
-	      exp.internal_name = imp[i].internal_name;
-	      exp.name = imp[i].name;
-	      exp.its_name = imp[i].its_name;
-	      exp.ordinal = imp[i].ordinal;
-	      exp.hint = exp.ordinal >= 0 ? exp.ordinal : 0;
-	      exp.flag_private = 0;
-	      exp.flag_constant = 0;
-	      exp.flag_data = imp[i].data;
-	      exp.flag_noname = exp.name ? 0 : 1;
-	      one = make_one (&exp, output_bfd, (! exp.flag_data) && include_jmp_stub);
-	      add_bfd_to_link (one, bfd_get_filename (one), linfo);
 	    }
+	  if (do_this_dll)
+	    {
+	      bfd *ar_tail = make_tail (output_bfd);
+	      add_bfd_to_link (ar_tail, bfd_get_filename (ar_tail), linfo);
+	    }
+
+	  free (dll_symname);
 	}
-      if (do_this_dll)
+
+      while (undef_count)
 	{
-	  bfd *ar_tail = make_tail (output_bfd);
-	  add_bfd_to_link (ar_tail, bfd_get_filename (ar_tail), linfo);
+	  --undef_count;
+	  free (udef_table[undef_count].key);
 	}
-
-      free (dll_symname);
+      free (udef_table);
     }
 
-  while (undef_count)
+  if (pe_def_file && pe_def_file->name)
+    dll_filename = pe_def_file->name;
+  else
     {
-      --undef_count;
-      free (udef_table[undef_count].key);
+      dll_filename = bfd_get_filename (output_bfd);
+      for (const char *p = dll_filename; *p; p++)
+	if (*p == '\\' || *p == '/' || *p == ':')
+	  dll_filename = p + 1;
     }
-  free (udef_table);
+  dll_symname = xstrdup (dll_filename);
+  for (int i = 0; dll_symname[i]; i++)
+    if (!ISALNUM (dll_symname[i]))
+      dll_symname[i] = '_';
 }
 
 /* We were handed a *.DLL file.  Parse it and turn it into a set of
@@ -3629,7 +3625,7 @@ pe_dll_build_sections (bfd *abfd, struct bfd_link_info *info)
       return;
     }
 
-  generate_edata (abfd, info);
+  generate_edata ();
   build_filler_bfd (1);
   pe_output_file_set_long_section_names (filler_bfd);
 }

-- 
Alan Modra
Australia Development Lab, IBM