[binutils-gdb] Memory leaks and ineffective bounds checking in wasm_scan

[gdb-buildbot@sergiodj.net]

*** TEST RESULTS FOR COMMIT 0c0adcc52478ebb707ed780173e18262df6eab7e ***

commit 0c0adcc52478ebb707ed780173e18262df6eab7e
Author:     Alan Modra <amodra@gmail.com>
AuthorDate: Mon Jan 13 08:12:18 2020 +1030
Commit:     Alan Modra <amodra@gmail.com>
CommitDate: Mon Jan 13 12:12:05 2020 +1030

    Memory leaks and ineffective bounds checking in wasm_scan
    
    It's always a bad idea to perform arithmetic on an unknown value read
    from an object file before comparing against bounds.  Code like the
    following attempting to bounds check "len", a 64-bit value, isn't
    effective because the pointer arithmetic ignores the high 32 bits when
    compiled for a 32-bit host.
    
          READ_LEB128 (len, p, end);
          if (p + len < p || p + len > end)
            goto error_return;
    
    Instead, perform any arithmetic on known values where we don't need to
    worry about overflows:
    
          READ_LEB128 (len, p, end);
          if (len > (size_t) (end - p))
            goto error_return;
    
    I'll note that this check does do things the right way:
    
      READ_LEB128 (symcount, p, end);
      /* Sanity check: each symbol has at least two bytes.  */
      if (symcount > payload_size / 2)
        return FALSE;
    
    "symcount * 2 > payload_size" would be wrong since the multiply could
    overflow.
    
            * wasm-module.c (wasm_scan_name_function_section): Formatting.
            Delete asect name check.  Move asect NULL check to wasm_object_p.
            Correct bounds check of sizes against end.  Replace uses of
            bfd_zalloc with bfd_alloc, zeroing only necessary bytes.  Use
            just one bfd_release.
            (wasm_scan): Don't use malloc/strdup for section names,
            bfd_alloc instead.  Simplify code prefixing section name.
            Formatting.  Don't attempt to free memory here..
            (wasm_object_p): ..do so here.  Formatting.

diff --git a/bfd/ChangeLog b/bfd/ChangeLog
index 00d6cd810b..70944d3c80 100644
--- a/bfd/ChangeLog
+++ b/bfd/ChangeLog
@@ -1,3 +1,15 @@
+2020-01-13  Alan Modra  <amodra@gmail.com>
+
+	* wasm-module.c (wasm_scan_name_function_section): Formatting.
+	Delete asect name check.  Move asect NULL check to wasm_object_p.
+	Correct bounds check of sizes against end.  Replace uses of
+	bfd_zalloc with bfd_alloc, zeroing only necessary bytes.  Use
+	just one bfd_release.
+	(wasm_scan): Don't use malloc/strdup for section names,
+	bfd_alloc instead.  Simplify code prefixing section name.
+	Formatting.  Don't attempt to free memory here..
+	(wasm_object_p): ..do so here.
+
 2020-01-10  Szabolcs Nagy  <szabolcs.nagy@arm.com>
 
 	PR ld/22269
diff --git a/bfd/wasm-module.c b/bfd/wasm-module.c
index 06a964c12d..e62f842e12 100644
--- a/bfd/wasm-module.c
+++ b/bfd/wasm-module.c
@@ -246,16 +246,10 @@ wasm_scan_name_function_section (bfd *abfd, sec_ptr asect)
   asymbol *symbols = NULL;
   sec_ptr space_function_index;
 
-  if (! asect)
-    return FALSE;
-
-  if (strcmp (asect->name, WASM_NAME_SECTION) != 0)
-    return FALSE;
-
   p = asect->contents;
   end = asect->contents + asect->size;
 
-  if (! p)
+  if (!p)
     return FALSE;
 
   while (p < end)
@@ -272,7 +266,7 @@ wasm_scan_name_function_section (bfd *abfd, sec_ptr asect)
 
       READ_LEB128 (payload_size, p, end);
 
-      if (p > p + payload_size)
+      if (payload_size > (size_t) (end - p))
 	return FALSE;
 
       p += payload_size;
@@ -283,10 +277,7 @@ wasm_scan_name_function_section (bfd *abfd, sec_ptr asect)
 
   READ_LEB128 (payload_size, p, end);
 
-  if (p > p + payload_size)
-    return FALSE;
-
-  if (p + payload_size > end)
+  if (payload_size > (size_t) (end - p))
     return FALSE;
 
   end = p + payload_size;
@@ -294,22 +285,24 @@ wasm_scan_name_function_section (bfd *abfd, sec_ptr asect)
   READ_LEB128 (symcount, p, end);
 
   /* Sanity check: each symbol has at least two bytes.  */
-  if (symcount > payload_size/2)
+  if (symcount > payload_size / 2)
     return FALSE;
 
   tdata->symcount = symcount;
 
-  space_function_index = bfd_make_section_with_flags
-    (abfd, WASM_SECTION_FUNCTION_INDEX, SEC_READONLY | SEC_CODE);
+  space_function_index
+    = bfd_make_section_with_flags (abfd, WASM_SECTION_FUNCTION_INDEX,
+				   SEC_READONLY | SEC_CODE);
 
-  if (! space_function_index)
-    space_function_index = bfd_get_section_by_name (abfd, WASM_SECTION_FUNCTION_INDEX);
+  if (!space_function_index)
+    space_function_index
+      = bfd_get_section_by_name (abfd, WASM_SECTION_FUNCTION_INDEX);
 
-  if (! space_function_index)
+  if (!space_function_index)
     return FALSE;
 
-  symbols = bfd_zalloc (abfd, tdata->symcount * sizeof (asymbol));
-  if (! symbols)
+  symbols = bfd_alloc2 (abfd, tdata->symcount, sizeof (asymbol));
+  if (!symbols)
     return FALSE;
 
   for (symcount = 0; p < end && symcount < tdata->symcount; symcount++)
@@ -322,14 +315,15 @@ wasm_scan_name_function_section (bfd *abfd, sec_ptr asect)
       READ_LEB128 (idx, p, end);
       READ_LEB128 (len, p, end);
 
-      if (p + len < p || p + len > end)
+      if (len > (size_t) (end - p))
 	goto error_return;
 
-      name = bfd_zalloc (abfd, len + 1);
-      if (! name)
+      name = bfd_alloc (abfd, len + 1);
+      if (!name)
 	goto error_return;
 
       memcpy (name, p, len);
+      name[len] = 0;
       p += len;
 
       sym = &symbols[symcount];
@@ -350,8 +344,6 @@ wasm_scan_name_function_section (bfd *abfd, sec_ptr asect)
   return TRUE;
 
  error_return:
-  while (symcount)
-    bfd_release (abfd, (void *)symbols[--symcount].name);
   bfd_release (abfd, symbols);
   return FALSE;
 }
@@ -386,13 +378,12 @@ wasm_scan (bfd *abfd)
   bfd_vma vma = 0x80000000;
   int section_code;
   unsigned int bytes_read;
-  char *name = NULL;
   asection *bfdsec;
 
   if (bfd_seek (abfd, (file_ptr) 0, SEEK_SET) != 0)
     goto error_return;
 
-  if (! wasm_read_header (abfd, &error))
+  if (!wasm_read_header (abfd, &error))
     goto error_return;
 
   while ((section_code = wasm_read_byte (abfd, &error)) != EOF)
@@ -401,14 +392,13 @@ wasm_scan (bfd *abfd)
 	{
 	  const char *sname = wasm_section_code_to_name (section_code);
 
-	  if (! sname)
+	  if (!sname)
 	    goto error_return;
 
-	  name = strdup (sname);
-	  bfdsec = bfd_make_section_anyway_with_flags (abfd, name, SEC_HAS_CONTENTS);
+	  bfdsec = bfd_make_section_anyway_with_flags (abfd, sname,
+						       SEC_HAS_CONTENTS);
 	  if (bfdsec == NULL)
 	    goto error_return;
-	  name = NULL;
 
 	  bfdsec->vma = vma;
 	  bfdsec->lma = vma;
@@ -423,9 +413,9 @@ wasm_scan (bfd *abfd)
 	  bfd_vma payload_len;
 	  file_ptr section_start;
 	  bfd_vma namelen;
+	  char *name;
 	  char *prefix = WASM_SECTION_PREFIX;
-	  char *p;
-	  int ret;
+	  size_t prefixlen = strlen (prefix);
 
 	  payload_len = wasm_read_leb128 (abfd, &error, &bytes_read, FALSE);
 	  if (error)
@@ -434,21 +424,18 @@ wasm_scan (bfd *abfd)
 	  namelen = wasm_read_leb128 (abfd, &error, &bytes_read, FALSE);
 	  if (error || namelen > payload_len)
 	    goto error_return;
-	  name = bfd_zmalloc (namelen + strlen (prefix) + 1);
-	  if (! name)
-	    goto error_return;
-	  p = name;
-	  ret = sprintf (p, "%s", prefix);
-	  if (ret < 0 || (bfd_vma) ret != strlen (prefix))
+	  name = bfd_alloc (abfd, namelen + prefixlen + 1);
+	  if (!name)
 	    goto error_return;
-	  p += ret;
-	  if (bfd_bread (p, namelen, abfd) != namelen)
+	  memcpy (name, prefix, prefixlen);
+	  if (bfd_bread (name + prefixlen, namelen, abfd) != namelen)
 	    goto error_return;
+	  name[prefixlen + namelen] = 0;
 
-	  bfdsec = bfd_make_section_anyway_with_flags (abfd, name, SEC_HAS_CONTENTS);
+	  bfdsec = bfd_make_section_anyway_with_flags (abfd, name,
+						       SEC_HAS_CONTENTS);
 	  if (bfdsec == NULL)
 	    goto error_return;
-	  name = NULL;
 
 	  bfdsec->vma = vma;
 	  bfdsec->lma = vma;
@@ -459,8 +446,8 @@ wasm_scan (bfd *abfd)
 
       if (bfdsec->size != 0)
 	{
-	  bfdsec->contents = bfd_zalloc (abfd, bfdsec->size);
-	  if (! bfdsec->contents)
+	  bfdsec->contents = bfd_alloc (abfd, bfdsec->size);
+	  if (!bfdsec->contents)
 	    goto error_return;
 
 	  if (bfd_bread (bfdsec->contents, bfdsec->size, abfd) != bfdsec->size)
@@ -478,12 +465,6 @@ wasm_scan (bfd *abfd)
   return TRUE;
 
  error_return:
-  if (name)
-    free (name);
-
-  for (bfdsec = abfd->sections; bfdsec; bfdsec = bfdsec->next)
-    free ((void *) bfdsec->name);
-
   return FALSE;
 }
 
@@ -750,23 +731,30 @@ static const bfd_target *
 wasm_object_p (bfd *abfd)
 {
   bfd_boolean error;
+  asection *s;
 
   if (bfd_seek (abfd, (file_ptr) 0, SEEK_SET) != 0)
     return NULL;
 
-  if (! wasm_read_header (abfd, &error))
+  if (!wasm_read_header (abfd, &error))
     {
       bfd_set_error (bfd_error_wrong_format);
       return NULL;
     }
 
-  if (! wasm_mkobject (abfd) || ! wasm_scan (abfd))
+  if (!wasm_mkobject (abfd))
     return NULL;
 
-  if (! bfd_default_set_arch_mach (abfd, bfd_arch_wasm32, 0))
-    return NULL;
+  if (!wasm_scan (abfd)
+      || !bfd_default_set_arch_mach (abfd, bfd_arch_wasm32, 0))
+    {
+      bfd_release (abfd, abfd->tdata.any);
+      abfd->tdata.any = NULL;
+      return NULL;
+    }
 
-  if (wasm_scan_name_function_section (abfd, bfd_get_section_by_name (abfd, WASM_NAME_SECTION)))
+  s = bfd_get_section_by_name (abfd, WASM_NAME_SECTION);
+  if (s != NULL && wasm_scan_name_function_section (abfd, s))
     abfd->flags |= HAS_SYMS;
 
   return abfd->xvec;