From patchwork Wed May 15 19:50:24 2019 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Mark Wielaard X-Patchwork-Id: 32701 Received: (qmail 128820 invoked by alias); 15 May 2019 19:50:29 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: libc-alpha-owner@sourceware.org Delivered-To: mailing list libc-alpha@sourceware.org Received: (qmail 128811 invoked by uid 89); 15 May 2019 19:50:29 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-19.1 required=5.0 tests=AWL, BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, SPF_PASS autolearn=ham version=3.3.1 spammy=*mem X-HELO: gnu.wildebeest.org Message-ID: Subject: Re: [PATCH] dlfcn: Guard __dlerror_main_freeres with __libc_once_get (once) [BZ# 24476] From: Mark Wielaard To: Carlos O'Donell , libc-alpha@sourceware.org Cc: davidben@google.com, carlos@redhat.com Date: Wed, 15 May 2019 21:50:24 +0200 In-Reply-To: <6af615b8-814e-a724-3f09-4db7da9420ba@redhat.com> References: <1557422299-25391-1-git-send-email-mark@klomp.org> <6af615b8-814e-a724-3f09-4db7da9420ba@redhat.com> Mime-Version: 1.0 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 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));