| Message ID | 1557422299-25391-1-git-send-email-mark@klomp.org |
|---|---|
| State | New |
| Headers | show |
| Series | dlfcn: Guard __dlerror_main_freeres with __libc_once_get (once) [BZ# 24476] | expand |
On 5/9/19 1:18 PM, Mark Wielaard wrote: > 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). Thank you for looking into this and fixing it! > 2019-05-09 Mark Wielaard <mark@klomp.org> > > [BZ# 24476] > * dlfcn/dlerror.c (__dlerror_main_freeres): Guard using > __libc_once_get (once). Reviewed-by: Carlos O'Donell <carlos@redhat.com> > 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 > (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); OK. > + if (__libc_once_get (once)) Not OK. If creating the key fails, then key remains 0, but once will have run init, and so this is true, but the key might be in use by someone else. I think this condition needs to be: if (__libc_once_get (once) && static_buf == NULL) This way you know: * The init() function ran and... * The key is assigned a valid entry via __libc_key_create(). This code suffers the same problem: __dlerror: 71 /* If we have not yet initialized the buffer do it now. */ 72 __libc_once (once, init); 73 74 /* Get error string. */ 75 result = (struct dl_action_result *) __libc_getspecific (key); 76 if (result == NULL) 77 result = &last_result; At this point __libc_key_create() may have failed, so checking for result == NULL is not valid, since key=0 may be in use by someone else. If should also be: if (static_buf != NULL) result = static_buf; Just like here: _dlerror_run: 131 /* Get error string and number. */ 132 if (static_buf != NULL) 133 result = static_buf; 134 else 135 { > + { > + void *mem; > + /* Free the global memory if used. */ > + check_free (&last_result); OK. > + /* Free the TSD memory if used. */ > + mem = __libc_getspecific (key); OK, except as noted above. > + if (mem != NULL) > + free_key_mem (mem); OK. > + } > } > > struct dlfcn_hook *_dlfcn_hook __attribute__((nocommon)); > In summary: - Check in __dlerror_main_freeres should be: if (__libc_once_get (once) && static_buf == NULL) - Fixup check in __dlerror: if (static_buf != NULL) result = static_buf; What you're fixing here is all the conditionals that appear wrong in the case that dlerror is never called or the other similar case (which can cause valgrind to fail) which is if __libc_key_create fails. No good deed goes unpunished :-)
On Thu, 2019-05-09 at 14:30 -0400, Carlos O'Donell wrote: > In summary: > - Check in __dlerror_main_freeres should be: > if (__libc_once_get (once) && static_buf == NULL) > - Fixup check in __dlerror: > if (static_buf != NULL) > result = static_buf; > > What you're fixing here is all the conditionals that appear > wrong in the case that dlerror is never called or the other > similar case (which can cause valgrind to fail) which is > if __libc_key_create fails. Makes sense. Done as follows: 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 <carlos@redhat.com> diff --git a/dlfcn/dlerror.c b/dlfcn/dlerror.c index 2737658..ca42c12 100644 --- a/dlfcn/dlerror.c +++ b/dlfcn/dlerror.c @@ -72,9 +72,16 @@ __dlerror (void) __libc_once (once, init); /* Get error string. */ - result = (struct dl_action_result *) __libc_getspecific (key); - if (result == NULL) - result = &last_result; + if (static_buf != NULL) + result = static_buf; + else + { + /* init () has been run and we don't use the static buffer. + So we have a valid key. */ + result = (struct dl_action_result *) __libc_getspecific (key); + if (result == NULL) + result = &last_result; + } /* Test whether we already returned the string. */ if (result->returned != 0) @@ -230,13 +237,19 @@ 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) && static_buf == NULL) + { + /* init () has been run and we don't use the static buffer. + So we have a valid key. */ + void *mem; + /* Free the TSD memory if used. */ + mem = __libc_getspecific (key); + if (mem != NULL) + free_key_mem (mem); + } } struct dlfcn_hook *_dlfcn_hook __attribute__((nocommon));
On 5/15/19 3:50 PM, Mark Wielaard wrote: > On Thu, 2019-05-09 at 14:30 -0400, Carlos O'Donell wrote: >> In summary: >> - Check in __dlerror_main_freeres should be: >> if (__libc_once_get (once) && static_buf == NULL) >> - Fixup check in __dlerror: >> if (static_buf != NULL) >> result = static_buf; >> >> What you're fixing here is all the conditionals that appear >> wrong in the case that dlerror is never called or the other >> similar case (which can cause valgrind to fail) which is >> if __libc_key_create fails. > > Makes sense. Done as follows: Perfect. Looks good to me. Please commit to master. > > 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 <carlos@redhat.com> > > diff --git a/dlfcn/dlerror.c b/dlfcn/dlerror.c > index 2737658..ca42c12 100644 > --- a/dlfcn/dlerror.c > +++ b/dlfcn/dlerror.c > @@ -72,9 +72,16 @@ __dlerror (void) > __libc_once (once, init); > > /* Get error string. */ > - result = (struct dl_action_result *) __libc_getspecific (key); > - if (result == NULL) > - result = &last_result; > + if (static_buf != NULL) > + result = static_buf; > + else > + { > + /* init () has been run and we don't use the static buffer. > + So we have a valid key. */ > + result = (struct dl_action_result *) __libc_getspecific (key); > + if (result == NULL) > + result = &last_result; > + } > > /* Test whether we already returned the string. */ > if (result->returned != 0) > @@ -230,13 +237,19 @@ 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) && static_buf == NULL) > + { > + /* init () has been run and we don't use the static buffer. > + So we have a valid key. */ > + void *mem; > + /* Free the TSD memory if used. */ > + mem = __libc_getspecific (key); > + if (mem != NULL) > + free_key_mem (mem); > + } > } > > struct dlfcn_hook *_dlfcn_hook __attribute__((nocommon)); >
On Wed, 2019-05-15 at 16:38 -0400, Carlos O'Donell wrote: > On 5/15/19 3:50 PM, Mark Wielaard wrote: > > Makes sense. Done as follows: > > Perfect. Looks good to me. > > Please commit to master. Thanks. Done. I also cherry-picked the commit to release/2.28/master and release/2.29/master. It applies cleanly and testing shows it fixes the bug. I am not entirely clear on the procedure to get them officially included in the release branches. Can I just commit the cherry-picked commits and email libc-stable about it? Or should the 2.28 and 2.29 release managers first ack the patch? Do I need to update the NEWS bugs sections too (master doesn't have a NEWS bugs section). Thanks, Mark
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));