Suggest a fix?

Suggest a fix?

[msnyder]

I see two instances of this code fragment in elflink.c:

  name = h->root.root.string;
  p = strchr (name, ELF_VER_CHR);
  if (p != NULL)
    {
      alc = bfd_malloc (p - name + 1);
      memcpy (alc, name, p - name);
      alc[p - name] = '\0';
      name = alc;
    }

One is in elf_collect_hash_codes, and one in elf_collect_gnu_hash_codes.
The issue is that the bfd_malloc is not checked for null return, and I'm
not sure what best to do if it returns null.

By any chance, is there an upper limit to the length of 'name',
and could we just use a static or local buffer?  I know you guys
don't like to use alloca...

Re: Suggest a fix?

[Alan Modra]

On Fri, Jul 27, 2007 at 03:35:56PM -0700, msnyder@sonic.net wrote:
> I see two instances of this code fragment in elflink.c:
> 
>   name = h->root.root.string;
>   p = strchr (name, ELF_VER_CHR);
>   if (p != NULL)
>     {
>       alc = bfd_malloc (p - name + 1);
>       memcpy (alc, name, p - name);
>       alc[p - name] = '\0';
>       name = alc;
>     }
> 
> One is in elf_collect_hash_codes, and one in elf_collect_gnu_hash_codes.
> The issue is that the bfd_malloc is not checked for null return, and I'm
> not sure what best to do if it returns null.

I suggested a fix in
http://sourceware.org/ml/binutils/2007-07/msg00141.html

Another would be to simply overwrite the '@' with a zero then restore
after calculating the hash.  Despite h->root.root.string being const,
it is always in read/write memory.

> By any chance, is there an upper limit to the length of 'name',

Nope.

-- 
Alan Modra
Australia Development Lab, IBM

Various malloc related fixes

[Alan Modra]

On Wed, Aug 01, 2007 at 05:42:29PM +0930, Alan Modra wrote:
> On Fri, Jul 27, 2007 at 03:35:56PM -0700, msnyder@sonic.net wrote:
> > I see two instances of this code fragment in elflink.c:
> > 
> >   name = h->root.root.string;
> >   p = strchr (name, ELF_VER_CHR);
> >   if (p != NULL)
> >     {
> >       alc = bfd_malloc (p - name + 1);
> >       memcpy (alc, name, p - name);
> >       alc[p - name] = '\0';
> >       name = alc;
> >     }
> > 
> > One is in elf_collect_hash_codes, and one in elf_collect_gnu_hash_codes.
> > The issue is that the bfd_malloc is not checked for null return, and I'm
> > not sure what best to do if it returns null.
> 
> I suggested a fix in
> http://sourceware.org/ml/binutils/2007-07/msg00141.html

This fixes these problems and other similar cases.

	* elf.c (bfd_section_from_shdr): Check bfd_alloc return.
	(elfcore_write_note): Check realloc return.
	* elflink.c (_bfd_elf_link_find_version_dependencies): Check
	bfd_zalloc return.
	(_bfd_elf_link_assign_sym_version): Check bfd_malloc return.
	(elf_link_add_object_symbols): Likewise.
	(struct hash_codes_info): New.
	(elf_collect_hash_codes): Return bfd_malloc error.
	(struct collect_gnu_hash_codes): Add "error".
	(elf_collect_gnu_hash_codes): Return bfd_malloc error.
	(bfd_elf_size_dynamic_sections): Check return status of
	_bfd_elf_link_find_version_dependencies.
	(bfd_elf_size_dynsym_hash_dynstr): Adjust for elf_collect_hash_codes
	and elf_collect_gnu_hash_codes changes.
	(elf_sym_name_compare): Formatting.
	(elf_fixup_link_order): Use bfd_malloc, not xmalloc.

Index: bfd/elf.c
===================================================================
RCS file: /cvs/src/src/bfd/elf.c,v
retrieving revision 1.416
diff -u -p -r1.416 elf.c
--- bfd/elf.c	18 Sep 2007 00:25:07 -0000	1.416
+++ bfd/elf.c	18 Sep 2007 07:40:32 -0000
@@ -1784,6 +1784,8 @@ bfd_section_from_shdr (bfd *abfd, unsign
 	    BFD_ASSERT (elf_section_data (target_sect)->rel_hdr2 == NULL);
 	    amt = sizeof (*hdr2);
 	    hdr2 = bfd_alloc (abfd, amt);
+	    if (hdr2 == NULL)
+	      return FALSE;
 	    elf_section_data (target_sect)->rel_hdr2 = hdr2;
 	  }
 	*hdr2 = *hdr;
@@ -8105,6 +8107,8 @@ elfcore_write_note (bfd *abfd,
   newspace = 12 + ((namesz + 3) & -4) + ((size + 3) & -4);
 
   buf = realloc (buf, *bufsiz + newspace);
+  if (buf == NULL)
+    return buf;
   dest = buf + *bufsiz;
   *bufsiz += newspace;
   xnp = (Elf_External_Note *) dest;
Index: bfd/elflink.c
===================================================================
RCS file: /cvs/src/src/bfd/elflink.c,v
retrieving revision 1.277
diff -u -p -r1.277 elflink.c
--- bfd/elflink.c	18 Sep 2007 00:25:07 -0000	1.277
+++ bfd/elflink.c	18 Sep 2007 07:40:41 -0000
@@ -1807,6 +1807,11 @@ _bfd_elf_link_find_version_dependencies 
 
   amt = sizeof *a;
   a = bfd_zalloc (rinfo->output_bfd, amt);
+  if (a == NULL)
+    {
+      rinfo->failed = TRUE;
+      return FALSE;
+    }
 
   /* Note that we are copying a string pointer here, and testing it
      above.  If bfd_elf_string_from_elf_section is ever changed to
@@ -1901,7 +1906,10 @@ _bfd_elf_link_assign_sym_version (struct
 	      len = p - h->root.root.string;
 	      alc = bfd_malloc (len);
 	      if (alc == NULL)
-		return FALSE;
+		{
+		  sinfo->failed = TRUE;
+		  return FALSE;
+		}
 	      memcpy (alc, h->root.root.string, len - 1);
 	      alc[len - 1] = '\0';
 	      if (alc[len - 2] == ELF_VER_CHR)
@@ -4278,6 +4286,8 @@ elf_link_add_object_symbols (bfd *abfd, 
 		      amt = ((isymend - isym + 1)
 			     * sizeof (struct elf_link_hash_entry *));
 		      nondeflt_vers = bfd_malloc (amt);
+		      if (!nondeflt_vers)
+			goto error_free_vers;
 		    }
 		  nondeflt_vers[nondeflt_vers_cnt++] = h;
 		}
@@ -4436,6 +4446,8 @@ elf_link_add_object_symbols (bfd *abfd, 
 
 	  amt = p - h->root.root.string;
 	  shortname = bfd_malloc (amt + 1);
+	  if (!shortname)
+	    goto error_free_vers;
 	  memcpy (shortname, h->root.root.string, amt);
 	  shortname[amt] = '\0';
 
@@ -4980,13 +4992,19 @@ bfd_elf_link_add_symbols (bfd *abfd, str
     }
 }
 \f
+struct hash_codes_info
+{
+  unsigned long *hashcodes;
+  bfd_boolean error;
+};
+  
 /* This function will be called though elf_link_hash_traverse to store
    all hash value of the exported symbols in an array.  */
 
 static bfd_boolean
 elf_collect_hash_codes (struct elf_link_hash_entry *h, void *data)
 {
-  unsigned long **valuep = data;
+  struct hash_codes_info *inf = data;
   const char *name;
   char *p;
   unsigned long ha;
@@ -5004,6 +5022,11 @@ elf_collect_hash_codes (struct elf_link_
   if (p != NULL)
     {
       alc = bfd_malloc (p - name + 1);
+      if (alc == NULL)
+	{
+	  inf->error = TRUE;
+	  return FALSE;
+	}
       memcpy (alc, name, p - name);
       alc[p - name] = '\0';
       name = alc;
@@ -5013,7 +5036,7 @@ elf_collect_hash_codes (struct elf_link_
   ha = bfd_elf_hash (name);
 
   /* Store the found hash value in the array given as the argument.  */
-  *(*valuep)++ = ha;
+  *(inf->hashcodes)++ = ha;
 
   /* And store it in the struct so that we can put it in the hash table
      later.  */
@@ -5043,6 +5066,7 @@ struct collect_gnu_hash_codes
   long int local_indx;
   long int shift1, shift2;
   unsigned long int mask;
+  bfd_boolean error;
 };
 
 /* This function will be called though elf_link_hash_traverse to store
@@ -5073,6 +5097,11 @@ elf_collect_gnu_hash_codes (struct elf_l
   if (p != NULL)
     {
       alc = bfd_malloc (p - name + 1);
+      if (alc == NULL)
+	{
+	  s->error = TRUE;
+	  return FALSE;
+	}
       memcpy (alc, name, p - name);
       alc[p - name] = '\0';
       name = alc;
@@ -5943,6 +5972,8 @@ bfd_elf_size_dynamic_sections (bfd *outp
 	elf_link_hash_traverse (elf_hash_table (info),
 				_bfd_elf_link_find_version_dependencies,
 				&sinfo);
+	if (sinfo.failed)
+	  return FALSE;
 
 	if (elf_tdata (output_bfd)->verref == NULL)
 	  s->flags |= SEC_EXCLUDE;
@@ -6164,7 +6195,7 @@ bfd_elf_size_dynsym_hash_dynstr (bfd *ou
       if (info->emit_hash)
 	{
 	  unsigned long int *hashcodes;
-	  unsigned long int *hashcodesp;
+	  struct hash_codes_info hashinf;
 	  bfd_size_type amt;
 	  unsigned long int nsyms;
 	  size_t bucketcount;
@@ -6177,13 +6208,16 @@ bfd_elf_size_dynsym_hash_dynstr (bfd *ou
 	  hashcodes = bfd_malloc (amt);
 	  if (hashcodes == NULL)
 	    return FALSE;
-	  hashcodesp = hashcodes;
+	  hashinf.hashcodes = hashcodes;
+	  hashinf.error = FALSE;
 
 	  /* Put all hash values in HASHCODES.  */
 	  elf_link_hash_traverse (elf_hash_table (info),
-				  elf_collect_hash_codes, &hashcodesp);
+				  elf_collect_hash_codes, &hashinf);
+	  if (hashinf.error)
+	    return FALSE;
 
-	  nsyms = hashcodesp - hashcodes;
+	  nsyms = hashinf.hashcodes - hashcodes;
 	  bucketcount
 	    = compute_bucket_count (info, hashcodes, nsyms, 0);
 	  free (hashcodes);
@@ -6232,6 +6266,8 @@ bfd_elf_size_dynsym_hash_dynstr (bfd *ou
 	  /* Put all hash values in HASHCODES.  */
 	  elf_link_hash_traverse (elf_hash_table (info),
 				  elf_collect_gnu_hash_codes, &cinfo);
+	  if (cinfo.error)
+	    return FALSE;
 
 	  bucketcount
 	    = compute_bucket_count (info, cinfo.hashcodes, cinfo.nsyms, 1);
@@ -6795,12 +6831,12 @@ elf_sym_name_compare (const void *arg1, 
 static struct elf_symbuf_head *
 elf_create_symbuf (bfd_size_type symcount, Elf_Internal_Sym *isymbuf)
 {
-  Elf_Internal_Sym **ind, **indbufend, **indbuf
-    = bfd_malloc2 (symcount, sizeof (*indbuf));
+  Elf_Internal_Sym **ind, **indbufend, **indbuf;
   struct elf_symbuf_symbol *ssym;
   struct elf_symbuf_head *ssymbuf, *ssymhead;
   bfd_size_type i, shndx_count;
 
+  indbuf = bfd_malloc2 (symcount, sizeof (*indbuf));
   if (indbuf == NULL)
     return NULL;
 
@@ -9948,7 +9984,9 @@ elf_fixup_link_order (bfd *abfd, asectio
     return TRUE;
 
   sections = (struct bfd_link_order **)
-    xmalloc (seen_linkorder * sizeof (struct bfd_link_order *));
+    bfd_malloc (seen_linkorder * sizeof (struct bfd_link_order *));
+  if (sections == NULL)
+    return FALSE;
   seen_linkorder = 0;
 
   for (p = o->map_head.link_order; p != NULL; p = p->next)

-- 
Alan Modra
Australia Development Lab, IBM