diff mbox series

elf: Fix memory leaks for dl_scope_free_list (bug 26641)

Message ID 20230906104939.718623-1-peadar@arista.com
State New
Headers
Series elf: Fix memory leaks for dl_scope_free_list (bug 26641) |

Checks

Context Check Description
redhat-pt-bot/TryBot-apply_patch success Patch applied to master at the time it was sent
linaro-tcwg-bot/tcwg_glibc_build--master-aarch64 success Testing passed
redhat-pt-bot/TryBot-32bit success Build for i686
linaro-tcwg-bot/tcwg_glibc_check--master-aarch64 success Testing passed
linaro-tcwg-bot/tcwg_glibc_build--master-arm success Testing passed
linaro-tcwg-bot/tcwg_glibc_check--master-arm success Testing passed

Commit Message

Peter Edwards Sept. 6, 2023, 10:49 a.m. UTC 
  _dl_scope_free is used to maintain a cache of scope lists, so a number
of them can be free'd all at once (thus avoiding contention on a lock)

Once 50 scopes have been accumulated, the next call to _dl_scope_free
will free the accumulated 50 entries. However, it does *not* free the
just-passed entry.

Secondly, there's a __libc_freeres function for releasing the
_dl_scope_free list when tearing down libc's innards. This just free's
the actual list, but not its content. Update it to also free the content.

This fixes valgrind observed leaks from dl_open_worker where these lists
are allocated.

Signed-off-by: Peter Edwards <peadar@arista.com>
---
 elf/dl-libc.c  | 9 +++++++--
 elf/dl-scope.c | 1 +
 2 files changed, 8 insertions(+), 2 deletions(-)

Comments

Florian Weimer Sept. 7, 2023, 8:45 a.m. UTC | #1 
* Peter Edwards via Libc-alpha:

> _dl_scope_free is used to maintain a cache of scope lists, so a number
> of them can be free'd all at once (thus avoiding contention on a lock)
>
> Once 50 scopes have been accumulated, the next call to _dl_scope_free
> will free the accumulated 50 entries. However, it does *not* free the
> just-passed entry.
>
> Secondly, there's a __libc_freeres function for releasing the
> _dl_scope_free list when tearing down libc's innards. This just free's
> the actual list, but not its content. Update it to also free the content.
>
> This fixes valgrind observed leaks from dl_open_worker where these lists
> are allocated.

These two fixes are very different, maybe put them into separate
patches?  And __libc_freeres changes are much less risky than other
changes.  The _dl_scope_free fix should also reference a bug in Bugzilla
because it's a repeatable memory leak with potential application impact.

How difficult is it to trigger the missed free in _dl_scope_free?  Maybe
it's possible to write an mtrace-based test case?

Thanks,
Florian
diff mbox series

Patch

diff --git a/elf/dl-libc.c b/elf/dl-libc.c
index c12e52f330..0b2e30ea13 100644
--- a/elf/dl-libc.c
+++ b/elf/dl-libc.c
@@ -329,7 +329,12 @@  __dl_libc_freemem (void)
        malloc), and in the static library it's in .bss space.  */
     free_slotinfo (&GL(dl_tls_dtv_slotinfo_list)->next);
 
-  void *scope_free_list = GL(dl_scope_free_list);
+  struct dl_scope_free_list *scope_free_list = GL(dl_scope_free_list);
   GL(dl_scope_free_list) = NULL;
-  free (scope_free_list);
+  if (scope_free_list != NULL)
+    {
+      while (scope_free_list->count > 0)
+	free (scope_free_list->list[--scope_free_list->count]);
+      free (scope_free_list);
+    }
 }
diff --git a/elf/dl-scope.c b/elf/dl-scope.c
index 2d4653c6e6..dba77068bd 100644
--- a/elf/dl-scope.c
+++ b/elf/dl-scope.c
@@ -51,6 +51,7 @@  _dl_scope_free (void *old)
       THREAD_GSCOPE_WAIT ();
       while (fsl->count > 0)
 	free (fsl->list[--fsl->count]);
+      free (old);
       return 1;
     }
   return 0;