[PATCH, binutils/ARM] Fix leak of local internal symbols in elf32_arm_size_stubs

[PATCH, binutils/ARM] Fix leak of local internal symbols in elf32_arm_size_stubs

[Thomas Preud'homme]

In elf32_arm_size_stubs, when encountering a relocation against a local symbol for the first time in a given input section, bfd_elf_get_elf_syms is called if symtab_hdr->contents is NULL. However, the allocation performed by this function is never freed, hence a potential leak if such a situation occurs. This patch adds a free before exiting the scope in which local_syms is valid.

ChangeLog entry is as follows:

*** bfd/ChangeLog ***

2015-09-25  Thomas Preud'homme  <thomas.preudhomme@arm.com>

        * elf32-arm.c (elf32_arm_size_stubs): Free local_syms before exiting
        the block where it's valid.


diff --git a/bfd/elf32-arm.c b/bfd/elf32-arm.c
index aa01a59..f3fe773 100644
--- a/bfd/elf32-arm.c
+++ b/bfd/elf32-arm.c
@@ -5126,6 +5126,8 @@ elf32_arm_size_stubs (bfd *output_bfd,
 		    error_ret_free_internal:
 		      if (elf_section_data (section)->relocs == NULL)
 			free (internal_relocs);
+		      if (!symtab_hdr->contents)
+			free (local_syms);
 		      goto error_ret_free_local;
 		    }
 
@@ -5420,6 +5422,12 @@ elf32_arm_size_stubs (bfd *output_bfd,
 		free (internal_relocs);
 	    }
 
+	  if (!symtab_hdr->contents)
+	    {
+	      free (local_syms);
+	      local_syms = NULL;
+	    }
+
 	  if (htab->fix_cortex_a8)
 	    {
 	      /* Sort relocs which might apply to Cortex-A8 erratum.  */
@@ -5433,7 +5441,11 @@ elf32_arm_size_stubs (bfd *output_bfd,
 					  a8_relocs, num_a8_relocs,
 					  prev_num_a8_fixes, &stub_changed)
 		  != 0)
-		goto error_ret_free_local;
+		{
+		  if (!symtab_hdr->contents)
+		    free (local_syms);
+		  goto error_ret_free_local;
+		}
 	    }
 	}


The testsuite shows no regression when run for arm-none-eabi target.

Is this ok for master branch?

Best regards,

Thomas

Re: [PATCH, binutils/ARM] Fix leak of local internal symbols in elf32_arm_size_stubs

[Nick Clifton]

Hi Thomas,

> In elf32_arm_size_stubs, when encountering a relocation against a local symbol for the first time in a given input section, bfd_elf_get_elf_syms is called if symtab_hdr->contents is NULL. However, the allocation performed by this function is never freed, hence a potential leak if such a situation occurs. This patch adds a free before exiting the scope in which local_syms is valid.

Hmm, something seems slightly wrong here...

>   		      if (elf_section_data (section)->relocs == NULL)
>   			free (internal_relocs);
> +		      if (!symtab_hdr->contents)
> +			free (local_syms);
>   		      goto error_ret_free_local;

Why doesn't the code at the error_ret_free_local label actually free the 
local symbols as the name implies ?  [Answer: because the label is 
outside of the scope of local_syms.  But why ?  If the label were inside 
the scope it could free the memory and then return, making the patch 
above unnecessary].

Also - why do you need to check symtab_hdr->contents ?  Wouldn't it make 
more sense to check "local_syms != NULL" ?


> +	  if (!symtab_hdr->contents)
> +	    {
> +	      free (local_syms);
> +	      local_syms = NULL;
> +	    }

Again it would appear to make more sense to check local_syms than 
symtab_hdr->contents.

Cheers
   Nick

Re: [PATCH, binutils/ARM] Fix leak of local internal symbols in elf32_arm_size_stubs

[Alan Modra]

On Wed, Oct 28, 2015 at 02:55:24PM +0000, Nick Clifton wrote:
> Hi Thomas,
> 
> >In elf32_arm_size_stubs, when encountering a relocation against a local symbol for the first time in a given input section, bfd_elf_get_elf_syms is called if symtab_hdr->contents is NULL. However, the allocation performed by this function is never freed, hence a potential leak if such a situation occurs. This patch adds a free before exiting the scope in which local_syms is valid.
> 
> Hmm, something seems slightly wrong here...
> 
> >  		      if (elf_section_data (section)->relocs == NULL)
> >  			free (internal_relocs);
> >+		      if (!symtab_hdr->contents)
> >+			free (local_syms);
> >  		      goto error_ret_free_local;
> 
> Why doesn't the code at the error_ret_free_local label actually free the
> local symbols as the name implies ?  [Answer: because the label is outside
> of the scope of local_syms.  But why ?  If the label were inside the scope
> it could free the memory and then return, making the patch above
> unnecessary].
> 
> Also - why do you need to check symtab_hdr->contents ?  Wouldn't it make
> more sense to check "local_syms != NULL" ?
> 
> 
> >+	  if (!symtab_hdr->contents)
> >+	    {
> >+	      free (local_syms);
> >+	      local_syms = NULL;
> >+	    }
> 
> Again it would appear to make more sense to check local_syms than
> symtab_hdr->contents.

Thomas, it might pay to take a look at how elf64-ppc.c
ppc64_elf_size_stubs handles this.

-- 
Alan Modra
Australia Development Lab, IBM

Re: [PATCH, binutils/ARM] Fix leak of local internal symbols in elf32_arm_size_stubs

[Thomas Preudhomme]

On Thursday 29 October 2015 08:04:45 Alan Modra wrote:
> On Wed, Oct 28, 2015 at 02:55:24PM +0000, Nick Clifton wrote:
> > Hi Thomas,
> > 
> > >In elf32_arm_size_stubs, when encountering a relocation against a local
> > >symbol for the first time in a given input section, bfd_elf_get_elf_syms
> > >is called if symtab_hdr->contents is NULL. However, the allocation
> > >performed by this function is never freed, hence a potential leak if
> > >such a situation occurs. This patch adds a free before exiting the scope
> > >in which local_syms is valid.> 
> > Hmm, something seems slightly wrong here...
> > 
> > >        if (elf_section_data (section)->relocs == NULL)
> > >  
> > >  free (internal_relocs);
> > >
> > >+      if (!symtab_hdr->contents)
> > >+free (local_syms);
> > >
> > >        goto error_ret_free_local;
> > 
> > Why doesn't the code at the error_ret_free_local label actually free the
> > local symbols as the name implies ?  [Answer: because the label is outside
> > of the scope of local_syms.  But why ?  If the label were inside the scope
> > it could free the memory and then return, making the patch above
> > unnecessary].
> > 
> > Also - why do you need to check symtab_hdr->contents ?  Wouldn't it make
> > more sense to check "local_syms != NULL" ?
> > 
> > >+  if (!symtab_hdr->contents)
> > >+    {
> > >+      free (local_syms);
> > >+      local_syms = NULL;
> > >+    }
> > 
> > Again it would appear to make more sense to check local_syms than
> > symtab_hdr->contents.
> 
> Thomas, it might pay to take a look at how elf64-ppc.c
> ppc64_elf_size_stubs handles this.

Sorry for the long delay. Better late than never as we say. Please find below 
an updated patch following Yours and Nick's comment.

In elf32_arm_size_stubs, when encountering a relocation against a local symbol 
for the first time in a given input section, bfd_elf_get_elf_syms is called if 
symtab_hdr->contents is NULL. However, the allocation performed by this 
function is never freed, hence a potential leak if such a situation occurs. 
This patch moves the error_ret_free_local error goto label to be a fall 
through from error_ret_free_internal. This allows it to be in a scope where 
local_syms is available and to free it if it was allocated by 
bfd_elf_get_elf_syms (and not from symtab_hdr->contents).

ChangeLog entry is as follows:

*** bfd/ChangeLog ***

2016-03-08  Thomas Preud'homme  <thomas.preudhomme@arm.com>

        * elf32-arm.c (elf32_arm_size_stubs): Move error_ret_free_local to be
        a fall through from error_ret_free_internal.  Free local_syms in
        error_ret_free_local if allocated from bfd_elf_get_elf_syms ().


diff --git a/bfd/elf32-arm.c b/bfd/elf32-arm.c
index 
aa01a59e9a2d787ce4ba40243fdadf752ecca97a..3ed0742200dfecda282046a1ecbced08f231a3b9 
100644
--- a/bfd/elf32-arm.c
+++ b/bfd/elf32-arm.c
@@ -5126,7 +5126,13 @@ elf32_arm_size_stubs (bfd *output_bfd,
 		    error_ret_free_internal:
 		      if (elf_section_data (section)->relocs == NULL)
 			free (internal_relocs);
-		      goto error_ret_free_local;
+		    /* Fall through.  */
+		    error_ret_free_local:
+		      if (local_syms != NULL
+			  && (symtab_hdr->contents
+			      != (unsigned char *) local_syms))
+			free (local_syms);
+		      return FALSE;
 		    }
 
 		  hash = NULL;
@@ -5532,9 +5538,6 @@ elf32_arm_size_stubs (bfd *output_bfd,
       htab->num_a8_erratum_fixes = 0;
     }
   return TRUE;
-
- error_ret_free_local:
-  return FALSE;
 }
 
 /* Build all the stubs associated with the current output file.  The



The testsuite shows no regression when run for arm-none-eabi target.

Is this ok for master branch?

Best regards,

Thomas

Re: [PATCH, binutils/ARM] Fix leak of local internal symbols in elf32_arm_size_stubs

[Alan Modra]

On Tue, Apr 05, 2016 at 06:00:30PM +0100, Thomas Preudhomme wrote:
>         * elf32-arm.c (elf32_arm_size_stubs): Move error_ret_free_local to be
>         a fall through from error_ret_free_internal.  Free local_syms in
>         error_ret_free_local if allocated from bfd_elf_get_elf_syms ().

Looks good.

-- 
Alan Modra
Australia Development Lab, IBM