dlfcn: Guard __dlerror_main_freeres with __libc_once_get (once) [BZ# 24476]
Commit Message
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>
Comments
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
* Mark Wielaard:
> 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?
Yes, that's fine. Please use cherry-pick -x and edit the NEWS file
manually to include a line for the bug. Maybe tweak the commit message
to get the bug notification, too.
Thanks,
Florian
On 5/16/19 9:06 AM, Florian Weimer wrote:
> * Mark Wielaard:
>
>> 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?
>
> Yes, that's fine. Please use cherry-pick -x and edit the NEWS file
> manually to include a line for the bug. Maybe tweak the commit message
> to get the bug notification, too.
Mark,
For the record the policy is documented here in our Release policy:
https://sourceware.org/glibc/wiki/Release/#General_policy
~~~
Patch backports to stable branches are discussed on libc-stable,
and any patch on master that doesn't change ABI or API is
immediately suitable for backporting to a stable branch. It
is polite to post your backport to libc-stable (in parallel
with your commit) and explain the reason for the backport;
this helps distro maintainers decide if they want to sync to
get the fix.
~~~
I don't blame anyone for not finding it or knowing it, please always
feel free to ask such questions.
@@ -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));