fix uninitialized variable in dynamic linker

  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
  

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