Message ID | 550DE6E7.5030900@codesourcery.com |
---|---|
State | New, archived |
Headers |
Received: (qmail 123727 invoked by alias); 21 Mar 2015 21:48:08 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: <libc-alpha.sourceware.org> List-Unsubscribe: <mailto:libc-alpha-unsubscribe-##L=##H@sourceware.org> List-Subscribe: <mailto:libc-alpha-subscribe@sourceware.org> List-Archive: <http://sourceware.org/ml/libc-alpha/> List-Post: <mailto:libc-alpha@sourceware.org> List-Help: <mailto:libc-alpha-help@sourceware.org>, <http://sourceware.org/ml/#faqs> Sender: libc-alpha-owner@sourceware.org Delivered-To: mailing list libc-alpha@sourceware.org Received: (qmail 123714 invoked by uid 89); 21 Mar 2015 21:48:07 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.1 required=5.0 tests=AWL, BAYES_00, RCVD_IN_DNSWL_LOW, SPF_PASS autolearn=ham version=3.3.2 X-HELO: relay1.mentorg.com Message-ID: <550DE6E7.5030900@codesourcery.com> Date: Sat, 21 Mar 2015 15:47:19 -0600 From: Sandra Loosemore <sandra@codesourcery.com> User-Agent: Mozilla/5.0 (X11; Linux i686; rv:17.0) Gecko/20130329 Thunderbird/17.0.5 MIME-Version: 1.0 To: <libc-alpha@sourceware.org> Subject: [patch] fix uninitialized variable in dynamic linker Content-Type: multipart/mixed; boundary="------------020209010609000903070607" |
Commit Message
Sandra Loosemore
March 21, 2015, 9:47 p.m. UTC
When we were testing glibc 2.21 here, we were getting some mysterious dynamic linking failures on MIPS due to the recent-ish changes for that target to ignore objects with incompatible FP ABIs. On further investigation, it turned out that the dynamic linker's own entry in the link map had an invalid l_mach.fpabi value. I tracked this down to failure to initialize that field in the stack-allocated bootstrap_map structure in _dl_start; _dl_start_final was then happily copying the uninitialized l_mach value into the real map data structure. The attached patch zero-initializes the entire bootstrap_map data structure. I thought this was better than selectively initializing specific fields since it future-proofs the code against similar errors involving other fields that might be added (or used) in the future. I've verified that this fixes the 2.21 MIPS problems we saw. Is this patch OK for mainline head, or is further testing required? -Sandra
Comments
Sandra Loosemore <sandra@codesourcery.com> writes: > When we were testing glibc 2.21 here, we were getting some mysterious > dynamic linking failures on MIPS due to the recent-ish changes for that > target to ignore objects with incompatible FP ABIs. On further Thanks for looking at this, sorry to not find it during my testing of the fpabi work. > investigation, it turned out that the dynamic linker's own entry in the > link map had an invalid l_mach.fpabi value. I tracked this down to > failure to initialize that field in the stack-allocated bootstrap_map > structure in _dl_start; _dl_start_final was then happily copying the > uninitialized l_mach value into the real map data structure. I'm quite confused as to why I never saw this during test, I had over 70000 tests run correctly and I'd have thought I would see this at least once. > The attached patch zero-initializes the entire bootstrap_map data > structure. I thought this was better than selectively initializing > specific fields since it future-proofs the code against similar errors > involving other fields that might be added (or used) in the future. This sounds good. > I've verified that this fixes the 2.21 MIPS problems we saw. Is this > patch OK for mainline head, or is further testing required? I can't say OK as I don't understand this area of the code well enough but can at least confirm that the code I added relies on the fpabi field being zero initialised which looked like it held true. Thanks, Matthew
On 04/13/2015 04:29 PM, Matthew Fortune wrote: > Sandra Loosemore <sandra@codesourcery.com> writes: >> When we were testing glibc 2.21 here, we were getting some mysterious >> dynamic linking failures on MIPS due to the recent-ish changes for that >> target to ignore objects with incompatible FP ABIs. On further > > Thanks for looking at this, sorry to not find it during my testing of > the fpabi work. > >> investigation, it turned out that the dynamic linker's own entry in the >> link map had an invalid l_mach.fpabi value. I tracked this down to >> failure to initialize that field in the stack-allocated bootstrap_map >> structure in _dl_start; _dl_start_final was then happily copying the >> uninitialized l_mach value into the real map data structure. > > I'm quite confused as to why I never saw this during test, I had over > 70000 tests run correctly and I'd have thought I would see this at least > once. If you run the dynamic linker in the usual way in a fresh process, it gets a fresh stack that is almost certainly still zero-initialized when it gets to this code. But.... we have a utility that wraps the dynamic linker startup with some magic stuff to override the DT_RPATH, etc in the executable. So in this case the dynamic linker isn't starting with a clean stack. >> The attached patch zero-initializes the entire bootstrap_map data >> structure. I thought this was better than selectively initializing >> specific fields since it future-proofs the code against similar errors >> involving other fields that might be added (or used) in the future. > > This sounds good. Even though we ran into this in an atypical scenario, explicitly initializing stack-allocated data structures seems like the best solution for robustness. >> I've verified that this fixes the 2.21 MIPS problems we saw. Is this >> patch OK for mainline head, or is further testing required? > > I can't say OK as I don't understand this area of the code well enough > but can at least confirm that the code I added relies on the fpabi > field being zero initialised which looked like it held true. > > Thanks, > Matthew > -Sandra
If it is indeed right to zero the entire structure, then the cleanest way to do that is just to change: struct dl_start_final_info info; to: struct dl_start_final_info info = {}; Then the compiler is responsible for figuring out the memset equivalent. Heretofore it never seemed necessary, suggesting that l_info was the only part of struct link_map that is not explicitly initialized and that is actually consulted during bootstrap relocation. If that was indeed true before, and recent MIPS additions to its l_mach (struct link_map_machine) were the only thing that caused any other part of bootstrap_map to need initialization, then there is a case to be made for initializing only those recently-added fields. The general logic trading off blind zeroing as "future-proofing" vs the performance cost has a different calculus here (and often in libc in general), because we are talking about adding some static cost to every program's startup across the entire system. Since most machines define PI_STATIC_AND_HIDDEN and so do not use this code path (bootstrap_map on the stack), I think we are less concerned than we otherwise would be about the blind zeroing doing excess dead stores. AFAICT the machines that fail to define PI_STATIC_AND_HIDDEN are aarch64, arm, hppa, m68k, mips*, powerpc*, tile*. For aarch64 I suspect it is just an oversight. For arm I suspect it could be enabled unconditionally now that we require more recent compilers; certainly it could be enabled by a configure test, though I don't know how to write one (since I don't know the circumstances in which it might not work). For the others I don't personally care about regressions in microoptimization. Thanks, Roland
Roland McGrath <roland@hack.frob.com> writes: > If it is indeed right to zero the entire structure, then the cleanest > way to do that is just to change: > > struct dl_start_final_info info; > > to: > > struct dl_start_final_info info = {}; > > Then the compiler is responsible for figuring out the memset equivalent. > > Heretofore it never seemed necessary, suggesting that l_info was the > only part of struct link_map that is not explicitly initialized and that > is actually consulted during bootstrap relocation. If that was indeed > true before, and recent MIPS additions to its l_mach (struct > link_map_machine) were the only thing that caused any other part of > bootstrap_map to need initialization, then there is a case to be made > for initializing only those recently-added fields. The general logic > trading off blind zeroing as "future-proofing" vs the performance cost > has a different calculus here (and often in libc in general), because we > are talking about adding some static cost to every program's startup > across the entire system. Granted this will cost a little more but does at least bring the dynamic loader link_map in line with a dynamically allocated one created by _dl_new_object which calloc's the memory. If opinion is swayed to having an init function or similar then I'm happy to add that to handle the recent MIPS additions. Perhaps an alternative though would be to unconditionally initialise the l_mach field which would make it a bit simpler for ports to understand how that structure can be used. I found it difficult to figure out if the structure would be zero initialised and only found the _dl_new_object way of creating it. Thanks, Matthew > Since most machines define PI_STATIC_AND_HIDDEN and so do not use this > code path (bootstrap_map on the stack), I think we are less concerned > than we otherwise would be about the blind zeroing doing excess dead > stores. > AFAICT the machines that fail to define PI_STATIC_AND_HIDDEN are > aarch64, arm, hppa, m68k, mips*, powerpc*, tile*. For aarch64 I suspect > it is just an oversight. For arm I suspect it could be enabled > unconditionally now that we require more recent compilers; certainly it > could be enabled by a configure test, though I don't know how to write > one (since I don't know the circumstances in which it might not work). > For the others I don't personally care about regressions in > microoptimization. > > > Thanks, > Roland
Index: elf/rtld.c =================================================================== --- elf/rtld.c (revision 447026) +++ elf/rtld.c (working copy) @@ -356,24 +356,26 @@ _dl_start (void *arg) HP_TIMING_NOW (start_time); #else 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 do not have to use the temporary bootstrap_map. Global variables 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 /* Figure out the run-time load address of the dynamic linker itself. */ bootstrap_map.l_addr = elf_machine_load_address ();