From patchwork Tue Nov 1 13:00:51 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Matthew Fortune X-Patchwork-Id: 17067 Received: (qmail 33952 invoked by alias); 1 Nov 2016 13:01:06 -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 33930 invoked by uid 89); 1 Nov 2016 13:01:05 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-4.2 required=5.0 tests=AWL, BAYES_00, RCVD_IN_DNSWL_NONE, RP_MATCHES_RCVD, SPF_PASS, URIBL_RED autolearn=ham version=3.3.2 spammy=recommendation, 1560, partly, sandracodesourcerycom X-HELO: mailapp01.imgtec.com From: Matthew Fortune To: Sandra Loosemore , Joseph Myers CC: "libc-alpha@sourceware.org" , Petar Jovanovic Subject: [PATCH,resend] fix uninitialized variable in dynamic linker Date: Tue, 1 Nov 2016 13:00:51 +0000 Message-ID: <6D39441BF12EF246A7ABCE6654B0235380AB33A8@HHMAIL01.hh.imgtec.org> MIME-Version: 1.0 Sandra Loosemore writes: > On 10/26/2016 03:01 PM, Matthew Fortune wrote: > > Joseph Myers writes: > >> On Wed, 26 Oct 2016, Matthew Fortune wrote: > >> > >>> +# ifdef ELF_MACHINE_INIT_MAP > >>> + ELF_MACHINE_INIT_MAP (bootstrap_map); # endif > >> > >> We don't encourage use of #ifdef like that. It's better to have an > >> inline function defined everywhere and used unconditionally, for > >> which most systems have a dummy definition (see > >> dl-machine-reject-phdr.h and elf_machine_reject_phdr_p for an example > >> - if you have a header for a single function, you don't need to > >> update lots of dl-machine.h headers, just add a generic version - > >> which has the comments detailing the semantics of the function and > when it's needed - and a MIPS version). > > > > Thanks Joseph. It's been a while since I did a glibc patch and > > couldn't remember the recommended approach. > > > > Do you think I should add a whole new header for this? Or, since this > > is directly related to the reject_phdr feature for MIPS and only MIPS > > is affected then I could just add it to dl-machine-reject-phdr.h? > > Wouldn't it be easier and more maintainable just to unconditionally > zero-initialize the structure, as I did in the original patch? (updated subject to follow Sandra's original thread) OK, given no further comments and your recommendation to stick with the original. I'm reposting your original patch updated to remove minor fuzz on applying. Roland originally suggested leaving the compiler to initialise the stack allocated link_map structure but doing so would lead to an implicit memset function call. Since calls are not possible until the end of the _dl_start function then we can't do that. The data that gets zero'd increases from 1040 bytes to 1560 bytes for MIPS n64 but the benefit of knowing the link_map is zero initialised wherever it is created. I expect similar increases for the other architectures that do not define PI_STATIC_AND_HIDDEN. I'm re-running tests but Sandra's original testing should still be valid. Thanks, Matthew diff --git a/elf/rtld.c b/elf/rtld.c index 647661c..a1b3a39 100644 --- a/elf/rtld.c +++ b/elf/rtld.c @@ -357,7 +357,7 @@ _dl_start (void *arg) HP_TIMING_NOW (info.start_time); #endif - /* Partly clean the `bootstrap_map' structure up. Don't use + /* Zero-initialize the `bootstrap_map' structure. Don't use `memset' since it might not be built in or inlined and we cannot make function calls at this point. Use '__builtin_memset' if we know it is available. We do not have to clear the memory if we @@ -365,12 +365,14 @@ _dl_start (void *arg) are initialized to zero by default. */ #ifndef DONT_USE_BOOTSTRAP_MAP # ifdef HAVE_BUILTIN_MEMSET - __builtin_memset (bootstrap_map.l_info, '\0', sizeof (bootstrap_map.l_info)); + __builtin_memset (&bootstrap_map, '\0', sizeof (struct link_map)); # else - for (size_t cnt = 0; - cnt < sizeof (bootstrap_map.l_info) / sizeof (bootstrap_map.l_info[0]); - ++cnt) - bootstrap_map.l_info[cnt] = 0; + { + char *p = (char *) &bootstrap_map; + char *pend = p + sizeof (struct link_map); + while (p < pend) + *(p++) = '\0'; + } # endif #endif