diff mbox series

Leak in i386_elf_section_change_hook

Message ID ZdZuDfDPOToYAuRk@squeak.grove.modra.org
State Unresolved
Headers
Series Leak in i386_elf_section_change_hook |

Checks

Context Check Description
snail/binutils-gdb-check warning Git am fail log

Commit Message

Alan Modra Feb. 21, 2024, 9:41 p.m. UTC 
  notes_alloc is perfect for assorted memory you can't free easily
and/or would rather leave freeing until just before exit.

	* config/tc-i386.c (i386_elf_section_change_hook): Use notes_alloc.

Comments

Jan Beulich Feb. 22, 2024, 9:20 a.m. UTC | #1 
On 21.02.2024 22:41, Alan Modra wrote:
> notes_alloc is perfect for assorted memory you can't free easily
> and/or would rather leave freeing until just before exit.
> 
> 	* config/tc-i386.c (i386_elf_section_change_hook): Use notes_alloc.
> 
> diff --git a/gas/config/tc-i386.c b/gas/config/tc-i386.c
> index ed7c4a5ea03..c56ca4a2b4b 100644
> --- a/gas/config/tc-i386.c
> +++ b/gas/config/tc-i386.c
> @@ -17627,7 +17627,7 @@ i386_elf_section_change_hook (void)
>        break;
>    if (!curr)
>      {
> -      curr = XNEW (struct i386_segment_info);
> +      curr = notes_alloc (sizeof (*curr));
>        curr->subseg = info->subseg;
>        curr->next = NULL;
>        prev->next = curr;
> 

Just for my understanding: Is it an unwritten(?) requirement then that
all XNEW()-ed (and alike) memory be freed before exiting? If so, why
would that be? I can certainly see that library code may not leak
memory, but something like gas, which is an isolated process, gives
up all resources anyway once finished with the (singular) task.

Jan
Alan Modra Feb. 22, 2024, 9:47 p.m. UTC | #2 
On Thu, Feb 22, 2024 at 10:20:50AM +0100, Jan Beulich wrote:
> On 21.02.2024 22:41, Alan Modra wrote:
> > notes_alloc is perfect for assorted memory you can't free easily
> > and/or would rather leave freeing until just before exit.
> > 
> > 	* config/tc-i386.c (i386_elf_section_change_hook): Use notes_alloc.
> > 
> > diff --git a/gas/config/tc-i386.c b/gas/config/tc-i386.c
> > index ed7c4a5ea03..c56ca4a2b4b 100644
> > --- a/gas/config/tc-i386.c
> > +++ b/gas/config/tc-i386.c
> > @@ -17627,7 +17627,7 @@ i386_elf_section_change_hook (void)
> >        break;
> >    if (!curr)
> >      {
> > -      curr = XNEW (struct i386_segment_info);
> > +      curr = notes_alloc (sizeof (*curr));
> >        curr->subseg = info->subseg;
> >        curr->next = NULL;
> >        prev->next = curr;
> > 
> 
> Just for my understanding: Is it an unwritten(?) requirement then that
> all XNEW()-ed (and alike) memory be freed before exiting? If so, why
> would that be? I can certainly see that library code may not leak
> memory, but something like gas, which is an isolated process, gives
> up all resources anyway once finished with the (singular) task.

As you say, there is very little reason to free memory that holds
persistent data.  In fact, freeing memory slows down gas, ld, and the
binutils.  The only reason we bother is that many people using shiny
new tools that reports memory leaks, or automated checkers like
oss-fuzz, don't care or aren't able to distinguish between leaks that
matter and those that don't.  This leak obviously doesn't matter, but
a leak that occurred on every symbol evaluation would matter.
Hopefully by tidying memory before exit we'll only see reports of
leaks that matter.
diff mbox series

Patch

diff --git a/gas/config/tc-i386.c b/gas/config/tc-i386.c
index ed7c4a5ea03..c56ca4a2b4b 100644
--- a/gas/config/tc-i386.c
+++ b/gas/config/tc-i386.c
@@ -17627,7 +17627,7 @@  i386_elf_section_change_hook (void)
       break;
   if (!curr)
     {
-      curr = XNEW (struct i386_segment_info);
+      curr = notes_alloc (sizeof (*curr));
       curr->subseg = info->subseg;
       curr->next = NULL;
       prev->next = curr;