readelf section reading

This is a followup to git commit 8ff66993e0b5, a patch aimed at
segfaults found invoking readelf multiple times with fuzzed objects.
In that patch I added code to clear more stashed data early in
process_section_headers, along with any stashed section headers.  This
patch instead relies on clearing out the stash at the end of
process_object, making sure that process_object doesn't exit early.

The patch also introduces some new wrapper functions.

	* readelf.c (GET_ELF_SYMBOLS): Delete.  Replace with..
	(get_elf_symbols): ..this new function throughout.
	(get_32bit_section_headers): Don't free section_headers.
	(get_64bit_section_headers): Likewise.
	(get_section_headers): New function, use throughout in place of
	32bit and 64bit variants.
	(get_dynamic_section): Similarly.
	(process_section_headers): Don't free filedata memory here.
	(get_file_header): Don't get section headers here..
	(process_object): ..Read them here instead.  Don't exit without
	freeing filedata memory.

diff --git a/binutils/ChangeLog b/binutils/ChangeLog
index 76324997e9d3bec03edda764bdc95b8ed29ed583..bd91a3911d22b2c82f47b0f9606751603a32886c 100644 (file)
--- a/binutils/ChangeLog
+++ b/binutils/ChangeLog
@@ -1,3 +1,17 @@
+2021-06-11  Alan Modra  <amodra@gmail.com>
+
+       * readelf.c (GET_ELF_SYMBOLS): Delete.  Replace with..
+       (get_elf_symbols): ..this new function throughout.
+       (get_32bit_section_headers): Don't free section_headers.
+       (get_64bit_section_headers): Likewise.
+       (get_section_headers): New function, use throughout in place of
+       32bit and 64bit variants.
+       (get_dynamic_section): Similarly.
+       (process_section_headers): Don't free filedata memory here.
+       (get_file_header): Don't get section headers here..
+       (process_object): ..Read them here instead.  Don't exit without
+       freeing filedata memory.
+
 2021-06-09  Nick Clifton  <nickc@redhat.com>
 
        * MAINTAINERS: Remove Daniel Jacobwitz from the maintainers list.

diff --git a/binutils/readelf.c b/binutils/readelf.c
index ebb93b7dc1adad80e4145ed35698301711f66445..52d5302d07b59d19d747f5ccfd09621032b3dfb4 100644 (file)
--- a/binutils/readelf.c
+++ b/binutils/readelf.c
@@ -357,10 +357,6 @@ static const char * get_symbol_version_string
 
 #define DT_VERSIONTAGIDX(tag)  (DT_VERNEEDNUM - (tag)) /* Reverse order!  */
 
-#define GET_ELF_SYMBOLS(file, section, sym_count)                      \
-  (is_32bit_elf ? get_32bit_elf_symbols (file, section, sym_count)     \
-   : get_64bit_elf_symbols (file, section, sym_count))
-
 #define VALID_SYMBOL_NAME(strtab, strtab_size, offset) \
    (strtab != NULL && offset < strtab_size)
 #define VALID_DYNAMIC_NAME(filedata, offset) \
@@ -5756,7 +5752,6 @@ get_32bit_section_headers (Filedata * filedata, bool probe)
   if (shdrs == NULL)
     return false;
 
-  free (filedata->section_headers);
   filedata->section_headers = (Elf_Internal_Shdr *)
     cmalloc (num, sizeof (Elf_Internal_Shdr));
   if (filedata->section_headers == NULL)
@@ -5823,7 +5818,6 @@ get_64bit_section_headers (Filedata * filedata, bool probe)
   if (shdrs == NULL)
     return false;
 
-  free (filedata->section_headers);
   filedata->section_headers = (Elf_Internal_Shdr *)
     cmalloc (num, sizeof (Elf_Internal_Shdr));
   if (filedata->section_headers == NULL)
@@ -5858,6 +5852,21 @@ get_64bit_section_headers (Filedata * filedata, bool probe)
   return true;
 }
 
+static bool
+get_section_headers (Filedata *filedata, bool probe)
+{
+  if (filedata->section_headers != NULL)
+    return true;
+
+  if (filedata->file_header.e_shoff == 0)
+    return true;
+
+  if (is_32bit_elf)
+    return get_32bit_section_headers (filedata, probe);
+  else
+    return get_64bit_section_headers (filedata, probe);
+}
+
 static Elf_Internal_Sym *
 get_32bit_elf_symbols (Filedata *           filedata,
                       Elf_Internal_Shdr *  section,
@@ -6094,6 +6103,17 @@ get_64bit_elf_symbols (Filedata *           filedata,
   return isyms;
 }
 
+static Elf_Internal_Sym *
+get_elf_symbols (Filedata *filedata,
+                Elf_Internal_Shdr *section,
+                unsigned long *num_syms_return)
+{
+  if (is_32bit_elf)
+    return get_32bit_elf_symbols (filedata, section, num_syms_return);
+  else
+    return get_64bit_elf_symbols (filedata, section, num_syms_return);
+}
+
 static const char *
 get_elf_section_flags (Filedata * filedata, bfd_vma sh_flags)
 {
@@ -6451,23 +6471,6 @@ process_section_headers (Filedata * filedata)
   Elf_Internal_Shdr * section;
   unsigned int i;
 
-  free (filedata->section_headers);
-  filedata->section_headers = NULL;
-  free (filedata->dynamic_symbols);
-  filedata->dynamic_symbols = NULL;
-  filedata->num_dynamic_syms = 0;
-  free (filedata->dynamic_strings);
-  filedata->dynamic_strings = NULL;
-  filedata->dynamic_strings_length = 0;
-  free (filedata->dynamic_syminfo);
-  filedata->dynamic_syminfo = NULL;
-  while (filedata->symtab_shndx_list != NULL)
-    {
-      elf_section_list *next = filedata->symtab_shndx_list->next;
-      free (filedata->symtab_shndx_list);
-      filedata->symtab_shndx_list = next;
-    }
-
   if (filedata->file_header.e_shnum == 0)
     {
       /* PR binutils/12467.  */
@@ -6497,16 +6500,8 @@ process_section_headers (Filedata * filedata)
                (unsigned long) filedata->file_header.e_shoff);
     }
 
-  if (is_32bit_elf)
-    {
-      if (! get_32bit_section_headers (filedata, false))
-       return false;
-    }
-  else
-    {
-      if (! get_64bit_section_headers (filedata, false))
-       return false;
-    }
+  if (!get_section_headers (filedata, false))
+    return false;
 
   /* Read in the string table, so that we have names to display.  */
   if (filedata->file_header.e_shstrndx != SHN_UNDEF
@@ -6615,7 +6610,7 @@ process_section_headers (Filedata * filedata)
 
          CHECK_ENTSIZE (section, i, Sym);
          filedata->dynamic_symbols
-           = GET_ELF_SYMBOLS (filedata, section, &filedata->num_dynamic_syms);
+           = get_elf_symbols (filedata, section, &filedata->num_dynamic_syms);
          filedata->dynamic_symtab_section = section;
          break;
 
@@ -7170,7 +7165,7 @@ get_symtab (Filedata *filedata, Elf_Internal_Shdr *symsec,
 {
   *strtab = NULL;
   *strtablen = 0;
-  *symtab = GET_ELF_SYMBOLS (filedata, symsec, nsyms);
+  *symtab = get_elf_symbols (filedata, symsec, nsyms);
 
   if (*symtab == NULL)
     return false;
@@ -7341,7 +7336,7 @@ process_section_groups (Filedata * filedata)
            {
              symtab_sec = sec;
              free (symtab);
-             symtab = GET_ELF_SYMBOLS (filedata, symtab_sec, & num_syms);
+             symtab = get_elf_symbols (filedata, symtab_sec, & num_syms);
            }
 
          if (symtab == NULL)
@@ -10278,6 +10273,18 @@ get_64bit_dynamic_section (Filedata * filedata)
   return true;
 }
 
+static bool
+get_dynamic_section (Filedata *filedata)
+{
+  if (filedata->dynamic_section)
+    return true;
+
+  if (is_32bit_elf)
+    return get_32bit_dynamic_section (filedata);
+  else
+    return get_64bit_dynamic_section (filedata);
+}
+
 static void
 print_dynamic_flags (bfd_vma flags)
 {
@@ -10621,16 +10628,8 @@ process_dynamic_section (Filedata * filedata)
       return true;
     }
 
-  if (is_32bit_elf)
-    {
-      if (! get_32bit_dynamic_section (filedata))
-       return false;
-    }
-  else
-    {
-      if (! get_64bit_dynamic_section (filedata))
-       return false;
-    }
+  if (!get_dynamic_section (filedata))
+    return false;
 
   /* Find the appropriate symbol table.  */
   if (filedata->dynamic_symbols == NULL || do_histogram)
@@ -10714,7 +10713,7 @@ the .dynsym section doesn't match the DT_SYMTAB and DT_SYMENT tags\n"));
 
                  section.sh_name = filedata->string_table_length;
                  filedata->dynamic_symbols
-                   = GET_ELF_SYMBOLS (filedata, &section,
+                   = get_elf_symbols (filedata, &section,
                                       &filedata->num_dynamic_syms);
                  if (filedata->dynamic_symbols == NULL
                      || filedata->num_dynamic_syms != num_of_syms)
@@ -11744,7 +11743,7 @@ process_version_sections (Filedata * filedata)
 
            found = true;
 
-           symbols = GET_ELF_SYMBOLS (filedata, link_section, & num_syms);
+           symbols = get_elf_symbols (filedata, link_section, & num_syms);
            if (symbols == NULL)
              break;
 
@@ -12946,7 +12945,7 @@ process_symbol_table (Filedata * filedata)
          else
            printf (_("   Num:    Value          Size Type    Bind   Vis      Ndx Name\n"));
 
-         symtab = GET_ELF_SYMBOLS (filedata, section, & num_syms);
+         symtab = get_elf_symbols (filedata, section, & num_syms);
          if (symtab == NULL)
            continue;
 
@@ -14312,7 +14311,7 @@ apply_relocations (Filedata *                 filedata,
       if (filedata->file_header.e_machine == EM_SH)
        is_rela = false;
 
-      symtab = GET_ELF_SYMBOLS (filedata, symsec, & num_syms);
+      symtab = get_elf_symbols (filedata, symsec, & num_syms);
 
       for (rp = relocs; rp < relocs + num_relocs; ++rp)
        {
@@ -21185,16 +21184,6 @@ get_file_header (Filedata * filedata)
       filedata->file_header.e_shstrndx  = BYTE_GET (ehdr64.e_shstrndx);
     }
 
-  if (filedata->file_header.e_shoff)
-    {
-      /* There may be some extensions in the first section header.  Don't
-        bomb if we can't read it.  */
-      if (is_32bit_elf)
-       get_32bit_section_headers (filedata, true);
-      else
-       get_64bit_section_headers (filedata, true);
-    }
-
   return true;
 }
 
@@ -21305,19 +21294,8 @@ open_file (const char * pathname, bool is_separate)
   if (! get_file_header (filedata))
     goto fail;
 
-  if (filedata->file_header.e_shoff)
-    {
-      bool res;
-
-      /* Read the section headers again, this time for real.  */
-      if (is_32bit_elf)
-       res = get_32bit_section_headers (filedata, false);
-      else
-       res = get_64bit_section_headers (filedata, false);
-
-      if (!res)
-       goto fail;
-    }
+  if (!get_section_headers (filedata, false))
+    goto fail;
 
   return filedata;
 
@@ -21393,8 +21371,15 @@ process_object (Filedata * filedata)
 
   initialise_dump_sects (filedata);
 
+  /* There may be some extensions in the first section header.  Don't
+     bomb if we can't read it.  */
+  get_section_headers (filedata, true);
+
   if (! process_file_header (filedata))
-    return false;
+    {
+      res = false;
+      goto out;
+    }
 
   if (! process_section_headers (filedata))
     {
@@ -21490,6 +21475,7 @@ process_object (Filedata * filedata)
   if (! process_arch_specific (filedata))
     res = false;
 
+ out:
   free_filedata (filedata);
 
   free_debug_memory ();