[Bug dynamic-link/24476] New: __libc_freeres triggers bad free in libdl if dlerror was not used

classic Classic list List threaded Threaded
9 messages Options
Reply | Threaded
Open this post in threaded view
|

[Bug dynamic-link/24476] New: __libc_freeres triggers bad free in libdl if dlerror was not used

claude at 2xlibre dot net
https://sourceware.org/bugzilla/show_bug.cgi?id=24476

            Bug ID: 24476
           Summary: __libc_freeres triggers bad free in libdl if dlerror
                    was not used
           Product: glibc
           Version: 2.28
            Status: UNCONFIRMED
          Severity: normal
          Priority: P2
         Component: dynamic-link
          Assignee: unassigned at sourceware dot org
          Reporter: davidben at google dot com
  Target Milestone: ---

Created attachment 11750
  --> https://sourceware.org/bugzilla/attachment.cgi?id=11750&action=edit
sample reproducer

glibc 2.28 includes [0], which causes __libc_freeres to free some memory from
libdl. One of the functions it calls is __dlerror_main_freeres[1], which frees
the current thread's dlerror state. This does not work right if init[2] here
was never run. In that case, __dlerror_main_freeres ends up freeing whatever is
at thread local key zero, which may have been allocated by some other
application and may not be cleaned up by free().

I've attached a sample program which demonstrates the problem when run in
valgrind. See the comment at the top for instructions and notes.

[0]
https://sourceware.org/git/?p=glibc.git;a=commit;h=2827ab990aefbb0e53374199b875d98f116d6390
[1]
https://sourceware.org/git/?p=glibc.git;a=blob;f=dlfcn/dlerror.c;h=33574faab65628ff85c358677e567837390efe7b;hb=3c03baca37fdcb52c3881e653ca392bba7a99c2b#l226
[2]
https://sourceware.org/git/?p=glibc.git;a=blob;f=dlfcn/dlerror.c;h=33574faab65628ff85c358677e567837390efe7b;hb=3c03baca37fdcb52c3881e653ca392bba7a99c2b#l173

--
You are receiving this mail because:
You are on the CC list for the bug.
Reply | Threaded
Open this post in threaded view
|

[Bug dynamic-link/24476] __libc_freeres triggers bad free in libdl if dlerror was not used

claude at 2xlibre dot net
https://sourceware.org/bugzilla/show_bug.cgi?id=24476

Carlos O'Donell <carlos at redhat dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |carlos at redhat dot com
           Assignee|unassigned at sourceware dot org   |carlos at redhat dot com

--
You are receiving this mail because:
You are on the CC list for the bug.
Reply | Threaded
Open this post in threaded view
|

[Bug dynamic-link/24476] __libc_freeres triggers bad free in libdl if dlerror was not used

claude at 2xlibre dot net
In reply to this post by claude at 2xlibre dot net
https://sourceware.org/bugzilla/show_bug.cgi?id=24476

--- Comment #1 from Carlos O'Donell <carlos at redhat dot com> ---
I'm looking into this to see what went wrong and how I might fix it. Thanks for
the report.

--
You are receiving this mail because:
You are on the CC list for the bug.
Reply | Threaded
Open this post in threaded view
|

[Bug dynamic-link/24476] __libc_freeres triggers bad free in libdl if dlerror was not used

claude at 2xlibre dot net
In reply to this post by claude at 2xlibre dot net
https://sourceware.org/bugzilla/show_bug.cgi?id=24476

Mark Wielaard <mark at klomp dot org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |NEW
   Last reconfirmed|                            |2019-05-09
                 CC|                            |mark at klomp dot org
     Ever confirmed|0                           |1

--- Comment #2 from Mark Wielaard <mark at klomp dot org> ---
Proposed fix. Only free the memory if __libc_once (once, init) has been called.

diff --git a/dlfcn/dlerror.c b/dlfcn/dlerror.c
index 2737658..41a41ee 100644
--- a/dlfcn/dlerror.c
+++ b/dlfcn/dlerror.c
@@ -230,13 +230,16 @@ free_key_mem (void *mem)
 void
 __dlerror_main_freeres (void)
 {
-  void *mem;
-  /* Free the global memory if used.  */
-  check_free (&last_result);
-  /* Free the TSD memory if used.  */
-  mem = __libc_getspecific (key);
-  if (mem != NULL)
-    free_key_mem (mem);
+  if (__libc_once_get (once))
+    {
+      void *mem;
+      /* Free the global memory if used.  */
+      check_free (&last_result);
+      /* Free the TSD memory if used.  */
+      mem = __libc_getspecific (key);
+      if (mem != NULL)
+       free_key_mem (mem);
+    }
 }

 struct dlfcn_hook *_dlfcn_hook __attribute__((nocommon));

--
You are receiving this mail because:
You are on the CC list for the bug.
Reply | Threaded
Open this post in threaded view
|

[Bug dynamic-link/24476] __libc_freeres triggers bad free in libdl if dlerror was not used

claude at 2xlibre dot net
In reply to this post by claude at 2xlibre dot net
https://sourceware.org/bugzilla/show_bug.cgi?id=24476

--- Comment #3 from Carlos O'Donell <carlos at redhat dot com> ---
(In reply to Mark Wielaard from comment #2)

> Proposed fix. Only free the memory if __libc_once (once, init) has been
> called.
>
> diff --git a/dlfcn/dlerror.c b/dlfcn/dlerror.c
> index 2737658..41a41ee 100644
> --- a/dlfcn/dlerror.c
> +++ b/dlfcn/dlerror.c
> @@ -230,13 +230,16 @@ free_key_mem (void *mem)
>  void
>  __dlerror_main_freeres (void)
>  {
> -  void *mem;
> -  /* Free the global memory if used.  */
> -  check_free (&last_result);
> -  /* Free the TSD memory if used.  */
> -  mem = __libc_getspecific (key);
> -  if (mem != NULL)
> -    free_key_mem (mem);
> +  if (__libc_once_get (once))
> +    {
> +      void *mem;
> +      /* Free the global memory if used.  */
> +      check_free (&last_result);
> +      /* Free the TSD memory if used.  */
> +      mem = __libc_getspecific (key);
> +      if (mem != NULL)
> +       free_key_mem (mem);
> +    }
>  }
>  
>  struct dlfcn_hook *_dlfcn_hook __attribute__((nocommon));

I reviewed this on libc-alpha.

It isn't quite right, and I noticed a similar failure if the key fails to be
created.

I have suggested some changes for Mark to look at.

I think we're almost done though.

--
You are receiving this mail because:
You are on the CC list for the bug.
Reply | Threaded
Open this post in threaded view
|

[Bug dynamic-link/24476] __libc_freeres triggers bad free in libdl if dlerror was not used

claude at 2xlibre dot net
In reply to this post by claude at 2xlibre dot net
https://sourceware.org/bugzilla/show_bug.cgi?id=24476

Mark Wielaard <mark at klomp dot org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |ASSIGNED
           Assignee|carlos at redhat dot com           |mark at klomp dot org

--- Comment #4 from Mark Wielaard <mark at klomp dot org> ---
Updated patch: https://sourceware.org/ml/libc-alpha/2019-05/msg00320.html

--
You are receiving this mail because:
You are on the CC list for the bug.
Reply | Threaded
Open this post in threaded view
|

[Bug dynamic-link/24476] __libc_freeres triggers bad free in libdl if dlerror was not used

claude at 2xlibre dot net
In reply to this post by claude at 2xlibre dot net
https://sourceware.org/bugzilla/show_bug.cgi?id=24476

Mark Wielaard <mark at klomp dot org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|ASSIGNED                    |RESOLVED
         Resolution|---                         |FIXED

--- Comment #5 from Mark Wielaard <mark at klomp dot org> ---
commit 11b451c8868d8a2b0edc5dfd44fc58d9ee538be0
Author: Mark Wielaard <[hidden email]>
Date:   Wed May 15 17:14:01 2019 +0200

    dlfcn: Guard __dlerror_main_freeres with __libc_once_get (once) [BZ# 24476]

    dlerror.c (__dlerror_main_freeres) will try to free resources which only
    have been initialized when init () has been called. That function is
    called when resources are needed using __libc_once (once, init) where
    once is a __libc_once_define (static, once) in the dlerror.c file.
    Trying to free those resources if init () hasn't been called will
    produce errors under valgrind memcheck. So guard the freeing of those
    resources using __libc_once_get (once) and make sure we have a valid
    key. Also add a similar guard to __dlerror ().

        * dlfcn/dlerror.c (__dlerror_main_freeres): Guard using
        __libc_once_get (once) and static_bug == NULL.
        (__dlerror): Check we have a valid key, set result to static_buf
        otherwise.

    Reviewed-by: Carlos O'Donell <[hidden email]>

--
You are receiving this mail because:
You are on the CC list for the bug.
Reply | Threaded
Open this post in threaded view
|

[Bug dynamic-link/24476] __libc_freeres triggers bad free in libdl if dlerror was not used

claude at 2xlibre dot net
In reply to this post by claude at 2xlibre dot net
https://sourceware.org/bugzilla/show_bug.cgi?id=24476

Mark Wielaard <mark at klomp dot org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Keywords|                            |glibc_2.28, glibc_2.29

--
You are receiving this mail because:
You are on the CC list for the bug.
Reply | Threaded
Open this post in threaded view
|

[Bug dynamic-link/24476] __libc_freeres triggers bad free in libdl if dlerror was not used

claude at 2xlibre dot net
In reply to this post by claude at 2xlibre dot net
https://sourceware.org/bugzilla/show_bug.cgi?id=24476

--- Comment #6 from Mark Wielaard <mark at klomp dot org> ---
Also backported/cherry-picked to release/2.28/master and release/2.29/master.

--
You are receiving this mail because:
You are on the CC list for the bug.