readelf section reading

readelf section reading

[Alan Modra]

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/readelf.c b/binutils/readelf.c
index ebb93b7dc1a..52d5302d07b 100644
--- 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 ();

-- 
Alan Modra
Australia Development Lab, IBM

Re: readelf section reading

[Alan Modra]

On Fri, Jun 11, 2021 at 05:33:12PM +0930, Alan Modra wrote:
> 	* 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.

Well that was more than a little bit broken.

Fix commit 4de91c10cdd9, which cached the single section header read
to pick up file header extension fields.  Also, testing e_shoff in
get_section_headers opened a hole for fuzzers where we'd end up with
segfaults due to non-zero e_shnum but NULL section_headers.

	* readelf.c (get_section_headers): Don't test e_shoff here, leave
	that to get_32bit_section_headers or get_64bit_section_headers.
	(process_object): Throw away section header read to print file
	header extension.

diff --git a/binutils/readelf.c b/binutils/readelf.c
index 1456c03a073..4217ea3b5b0 100644
--- a/binutils/readelf.c
+++ b/binutils/readelf.c
@@ -5859,9 +5859,6 @@ 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
@@ -21361,6 +21358,11 @@ process_object (Filedata * filedata)
       goto out;
     }
 
+  /* Throw away the single section header read above, so that we
+     re-read the entire set.  */
+  free (filedata->section_headers);
+  filedata->section_headers = NULL;
+
   if (! process_section_headers (filedata))
     {
       /* Without loaded section headers we cannot process lots of things.  */

-- 
Alan Modra
Australia Development Lab, IBM

Re: readelf section reading

[Alan Modra]

On Sat, Jun 12, 2021 at 11:01:33AM +0930, Alan Modra wrote:
> On Fri, Jun 11, 2021 at 05:33:12PM +0930, Alan Modra wrote:
> Fix commit 4de91c10cdd9, which cached the single section header read
> to pick up file header extension fields.

Hmm, no it didn't.  We had this one tucked away in
process_file_header.  Some of the paths through readelf read all the
section headers, eg. open_file.  process_file_header shouldn't be
throwing away the section headers in those cases.

	* readelf.c (process_file_header): Don't clear section_headers.

diff --git a/binutils/readelf.c b/binutils/readelf.c
index 4217ea3b5b0..79724e05494 100644
--- a/binutils/readelf.c
+++ b/binutils/readelf.c
@@ -5237,8 +5237,6 @@ process_file_header (Filedata * filedata)
 	header->e_shstrndx = filedata->section_headers[0].sh_link;
       if (header->e_shstrndx >= header->e_shnum)
 	header->e_shstrndx = SHN_UNDEF;
-      free (filedata->section_headers);
-      filedata->section_headers = NULL;
     }
 
   return true;


-- 
Alan Modra
Australia Development Lab, IBM