[PATCH,resend] fix uninitialized variable in dynamic linker

[PATCH,resend] fix uninitialized variable in dynamic linker

[Matthew Fortune]

Sandra Loosemore <sandra@codesourcery.com> writes:
> On 10/26/2016 03:01 PM, Matthew Fortune wrote:
> > Joseph Myers <joseph@codesourcery.com> 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
 
-- 
2.7.4

RE: [PATCH,resend] fix uninitialized variable in dynamic linker

[Matthew Fortune]

Ping^2

> 
> Matthew
> > -----Original Message-----
> > From: libc-alpha-owner@sourceware.org [mailto:libc-alpha-owner@sourceware.org] On Behalf
> > Of Matthew Fortune
> > Sent: 01 November 2016 13:01
> > To: Sandra Loosemore; Joseph Myers
> > Cc: libc-alpha@sourceware.org; Petar Jovanovic
> > Subject: [PATCH,resend] fix uninitialized variable in dynamic linker
> >
> > Sandra Loosemore <sandra@codesourcery.com> writes:
> > > On 10/26/2016 03:01 PM, Matthew Fortune wrote:
> > > > Joseph Myers <joseph@codesourcery.com> 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
> >
> > --
> > 2.7.4

RE: [PATCH,resend] fix uninitialized variable in dynamic linker

[Matthew Fortune]

Ping

Matthew
> -----Original Message-----
> From: libc-alpha-owner@sourceware.org [mailto:libc-alpha-owner@sourceware.org] On Behalf
> Of Matthew Fortune
> Sent: 01 November 2016 13:01
> To: Sandra Loosemore; Joseph Myers
> Cc: libc-alpha@sourceware.org; Petar Jovanovic
> Subject: [PATCH,resend] fix uninitialized variable in dynamic linker
> 
> Sandra Loosemore <sandra@codesourcery.com> writes:
> > On 10/26/2016 03:01 PM, Matthew Fortune wrote:
> > > Joseph Myers <joseph@codesourcery.com> 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
> 
> --
> 2.7.4