This is the mail archive of the
binutils@sourceware.org
mailing list for the binutils project.
Re: [PATCH, binutils/ARM] Fix leak of local internal symbols in elf32_arm_size_stubs
- From: Thomas Preudhomme <thomas dot preudhomme at foss dot arm dot com>
- To: Alan Modra <amodra at gmail dot com>, "binutils at sourceware dot org" <binutils at sourceware dot org>
- Cc: "nickc at redhat dot com" <nickc at redhat dot com>
- Date: Tue, 05 Apr 2016 18:00:30 +0100
- Subject: Re: [PATCH, binutils/ARM] Fix leak of local internal symbols in elf32_arm_size_stubs
- Authentication-results: sourceware.org; auth=none
- References: <002501d10adf$89816b40$9c8441c0$ at arm dot com> <5630E1DC dot 9080306 at redhat dot com> <20151029000444 dot GG13961 at bubble dot grove dot modra dot org>
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