[v2,1/2] Replace __libc_multiple_libcs with __libc_initial flag

  Change sbrk to fail for !__libc_initial (in the generic
implementation).  As a result, sbrk is (relatively) safe to use
for the __libc_initial case (from the main libc).  It is therefore
no longer necessary to avoid using it in that case (or updating the
brk cache), and the __libc_initial flag does not need to be updated
as part of dlmopen or static dlopen.

As before, direct brk system calls on Linux may lead to memory
corruption.
---
v2: Switch to a flag with just two states, simplifying the code.
  Add sbrk blocking for inner libcs.  brk can't be changed yet
  due to the pending refactoring.

 csu/init-first.c                    | 10 +++------
 csu/libc-start.c                    | 13 +++++------
 elf/dl-open.c                       |  6 -----
 elf/dl-sysdep.c                     |  2 --
 elf/libc_early_init.c               |  9 ++++++++
 include/libc-internal.h             |  7 +++++-
 misc/sbrk.c                         | 34 ++++++++++++++++++++---------
 sysdeps/mach/hurd/dl-sysdep.c       |  2 --
 sysdeps/mach/hurd/i386/init-first.c | 10 +++++----
 9 files changed, 53 insertions(+), 40 deletions(-)
  

On 02/12/2020 08:30, Florian Weimer via Libc-alpha wrote:
> Change sbrk to fail for !__libc_initial (in the generic
> implementation).  As a result, sbrk is (relatively) safe to use
> for the __libc_initial case (from the main libc).  It is therefore
> no longer necessary to avoid using it in that case (or updating the
> brk cache), and the __libc_initial flag does not need to be updated
> as part of dlmopen or static dlopen.
> 
> As before, direct brk system calls on Linux may lead to memory
> corruption.
> ---
> v2: Switch to a flag with just two states, simplifying the code.
>   Add sbrk blocking for inner libcs.  brk can't be changed yet
>   due to the pending refactoring.

LGTM, thanks.

Reviewed-by: Adhemerval Zanella  <adhemerval.zanella@linaro.org>

> 
>  csu/init-first.c                    | 10 +++------
>  csu/libc-start.c                    | 13 +++++------
>  elf/dl-open.c                       |  6 -----
>  elf/dl-sysdep.c                     |  2 --
>  elf/libc_early_init.c               |  9 ++++++++
>  include/libc-internal.h             |  7 +++++-
>  misc/sbrk.c                         | 34 ++++++++++++++++++++---------
>  sysdeps/mach/hurd/dl-sysdep.c       |  2 --
>  sysdeps/mach/hurd/i386/init-first.c | 10 +++++----
>  9 files changed, 53 insertions(+), 40 deletions(-)
> 
> diff --git a/csu/init-first.c b/csu/init-first.c
> index 47aaacdbd0..2115215668 100644
> --- a/csu/init-first.c
> +++ b/csu/init-first.c
> @@ -28,10 +28,6 @@
>  
>  #include <ldsodefs.h>
>  
> -/* Set nonzero if we have to be prepared for more than one libc being
> -   used in the process.  Safe assumption if initializer never runs.  */
> -int __libc_multiple_libcs attribute_hidden = 1;
> -
>  /* Remember the command line argument and enviroment contents for
>     later calls of initializers for dynamic libraries.  */
>  int __libc_argc attribute_hidden;

Ok.

> @@ -50,16 +46,16 @@ _init_first (int argc, char **argv, char **envp)
>  {
>  #endif
>  
> -  __libc_multiple_libcs = &_dl_starting_up && !_dl_starting_up;
> -
>    /* Make sure we don't initialize twice.  */
> -  if (!__libc_multiple_libcs)
> +#ifdef SHARED
> +  if (__libc_initial)
>      {
>        /* Set the FPU control word to the proper default value if the
>  	 kernel would use a different value.  */
>        if (__fpu_control != GLRO(dl_fpu_control))
>  	__setfpucw (__fpu_control);
>      }
> +#endif
>  
>    /* Save the command-line arguments.  */
>    __libc_argc = argc;

Ok.

> diff --git a/csu/libc-start.c b/csu/libc-start.c
> index 2d4d2ed1f9..d330812c2d 100644
> --- a/csu/libc-start.c
> +++ b/csu/libc-start.c
> @@ -141,8 +141,6 @@ LIBC_START_MAIN (int (*main) (int, char **, char ** MAIN_AUXVEC_DECL),
>    /* Result of the 'main' function.  */
>    int result;
>  
> -  __libc_multiple_libcs = &_dl_starting_up && !_dl_starting_up;
> -
>  #ifndef SHARED
>    _dl_relocate_static_pie ();
>  

Ok.

> @@ -213,12 +211,11 @@ LIBC_START_MAIN (int (*main) (int, char **, char ** MAIN_AUXVEC_DECL),
>  # endif
>  
>  # ifdef DL_SYSDEP_OSCHECK
> -  if (!__libc_multiple_libcs)
> -    {
> -      /* This needs to run to initiliaze _dl_osversion before TLS
> -	 setup might check it.  */
> -      DL_SYSDEP_OSCHECK (__libc_fatal);
> -    }
> +  {
> +    /* This needs to run to initiliaze _dl_osversion before TLS
> +       setup might check it.  */
> +    DL_SYSDEP_OSCHECK (__libc_fatal);
> +  }

Maybe remove the brackets here, DL_SYSDEP_OSCHECK already does it
and if we need to reimplement to another system a static inline
would be a better replacement.

>  # endif
>  
>    /* Initialize libpthread if linked in.  */

Ok.

> diff --git a/elf/dl-open.c b/elf/dl-open.c
> index 8769e47051..6710ea04cd 100644
> --- a/elf/dl-open.c
> +++ b/elf/dl-open.c
> @@ -787,12 +787,6 @@ dl_open_worker (void *a)
>    if (mode & RTLD_GLOBAL)
>      add_to_global_update (new);
>  
> -#ifndef SHARED
> -  /* We must be the static _dl_open in libc.a.  A static program that
> -     has loaded a dynamic object now has competition.  */
> -  __libc_multiple_libcs = 1;
> -#endif
> -
>    /* Let the user know about the opencount.  */
>    if (__glibc_unlikely (GLRO(dl_debug_mask) & DL_DEBUG_FILES))
>      _dl_debug_printf ("opening file=%s [%lu]; direct_opencount=%u\n\n",

Ok.

> diff --git a/elf/dl-sysdep.c b/elf/dl-sysdep.c
> index 854570821c..6cc4a76560 100644
> --- a/elf/dl-sysdep.c
> +++ b/elf/dl-sysdep.c
> @@ -58,8 +58,6 @@ ElfW(Addr) _dl_base_addr;
>  #endif
>  int __libc_enable_secure attribute_relro = 0;
>  rtld_hidden_data_def (__libc_enable_secure)
> -int __libc_multiple_libcs = 0;	/* Defining this here avoids the inclusion
> -				   of init-first.  */
>  /* This variable contains the lowest stack address ever used.  */
>  void *__libc_stack_end attribute_relro = NULL;
>  rtld_hidden_data_def(__libc_stack_end)

Ok.

> diff --git a/elf/libc_early_init.c b/elf/libc_early_init.c
> index 725ab2f811..28c6adca10 100644
> --- a/elf/libc_early_init.c
> +++ b/elf/libc_early_init.c
> @@ -18,8 +18,13 @@
>  
>  #include <ctype.h>
>  #include <libc-early-init.h>
> +#include <libc-internal.h>
>  #include <sys/single_threaded.h>
>  
> +#ifdef SHARED
> +_Bool __libc_initial;
> +#endif
> +
>  void
>  __libc_early_init (_Bool initial)
>  {
> @@ -28,4 +33,8 @@ __libc_early_init (_Bool initial)
>  
>    /* Only the outer namespace is marked as single-threaded.  */
>    __libc_single_threaded = initial;
> +
> +#ifdef SHARED
> +  __libc_initial = initial;
> +#endif
>  }

Ok.

> diff --git a/include/libc-internal.h b/include/libc-internal.h
> index 915613c030..c1e74056ac 100644
> --- a/include/libc-internal.h
> +++ b/include/libc-internal.h
> @@ -47,6 +47,11 @@ extern void __init_misc (int, char **, char **) attribute_hidden;
>  extern __typeof (__profile_frequency) __profile_frequency attribute_hidden;
>  # endif
>  
> -extern int __libc_multiple_libcs attribute_hidden;
> +#ifdef SHARED
> +/* True if this libc belongs to the initially loaded program (i.e., it
> +   is not for an audit module, not loaded via dlmopen, and not loaded
> +   via static dlopen either).  */
> +extern _Bool __libc_initial attribute_hidden;
> +#endif
>  
>  #endif /* _LIBC_INTERNAL  */

Ok.

> diff --git a/misc/sbrk.c b/misc/sbrk.c
> index ba3322fba6..a6929d736d 100644
> --- a/misc/sbrk.c
> +++ b/misc/sbrk.c
> @@ -16,9 +16,10 @@
>     <https://www.gnu.org/licenses/>.  */
>  
>  #include <errno.h>
> +#include <libc-internal.h>
> +#include <stdbool.h>
>  #include <stdint.h>
>  #include <unistd.h>
> -#include <libc-internal.h>
>  
>  /* Defined in brk.c.  */
>  extern void *__curbrk;

Ok.

> @@ -30,21 +31,34 @@ extern int __brk (void *addr);
>  void *
>  __sbrk (intptr_t increment)
>  {
> -  void *oldbrk;
> -
> -  /* If this is not part of the dynamic library or the library is used
> -     via dynamic loading in a statically linked program update
> -     __curbrk from the kernel's brk value.  That way two separate
> -     instances of __brk and __sbrk can share the heap, returning
> -     interleaved pieces of it.  */
> -  if (__curbrk == NULL || __libc_multiple_libcs)
> +  /* Controls whether __brk (0) is called to read the brk value from
> +     the kernel.  */
> +  bool update_brk = __curbrk == NULL;
> +
> +#if defined (SHARED) && ! IS_IN (rtld)
> +  if (!__libc_initial)
> +    {
> +      if (increment != 0)
> +	{
> +	  /* Do not allow changing the brk from an inner libc because
> +	     it cannot be synchronized with the outer libc's brk.  */
> +	  __set_errno (ENOMEM);
> +	  return (void *) -1;
> +	}
> +      /* Querying the kernel's brk value from an inner namespace is
> +	 fine.  */
> +      update_brk = true;
> +    }
> +#endif
> +
> +  if (update_brk)
>      if (__brk (0) < 0)		/* Initialize the break.  */
>        return (void *) -1;
>  
>    if (increment == 0)
>      return __curbrk;
>  
> -  oldbrk = __curbrk;
> +  void *oldbrk = __curbrk;
>    if (increment > 0
>        ? ((uintptr_t) oldbrk + (uintptr_t) increment < (uintptr_t) oldbrk)
>        : ((uintptr_t) oldbrk < (uintptr_t) -increment))

Ok.

> diff --git a/sysdeps/mach/hurd/dl-sysdep.c b/sysdeps/mach/hurd/dl-sysdep.c
> index 370495710e..a5169d85e7 100644
> --- a/sysdeps/mach/hurd/dl-sysdep.c
> +++ b/sysdeps/mach/hurd/dl-sysdep.c
> @@ -57,8 +57,6 @@ extern char **_environ;
>  
>  int __libc_enable_secure = 0;
>  rtld_hidden_data_def (__libc_enable_secure)
> -int __libc_multiple_libcs = 0;	/* Defining this here avoids the inclusion
> -				   of init-first.  */
>  /* This variable contains the lowest stack address ever used.  */
>  void *__libc_stack_end = NULL;
>  rtld_hidden_data_def(__libc_stack_end)

Ok.

> diff --git a/sysdeps/mach/hurd/i386/init-first.c b/sysdeps/mach/hurd/i386/init-first.c
> index 1827479f86..cbbc12204a 100644
> --- a/sysdeps/mach/hurd/i386/init-first.c
> +++ b/sysdeps/mach/hurd/i386/init-first.c
> @@ -30,6 +30,7 @@
>  #include <ldsodefs.h>
>  #include <fpu_control.h>
>  #include <libc-diag.h>
> +#include <libc-internal.h>
>  
>  extern void __mach_init (void);
>  extern void __init_misc (int, char **, char **);

Ok.

> @@ -40,7 +41,6 @@ unsigned long int __hurd_threadvar_stack_mask;
>  #ifndef SHARED
>  int __libc_enable_secure;
>  #endif
> -int __libc_multiple_libcs attribute_hidden = 1;
>  
>  extern int __libc_argc attribute_hidden;
>  extern char **__libc_argv attribute_hidden;

Ok.

> @@ -56,13 +56,12 @@ DEFINE_HOOK (_hurd_preinit_hook, (void));
>  static void
>  posixland_init (int argc, char **argv, char **envp)
>  {
> -  __libc_multiple_libcs = &_dl_starting_up && !_dl_starting_up;
> -
>    /* Now we have relocations etc. we can start signals etc.  */
>    _hurd_libc_proc_init (argv);
>  
> +#ifdef SHARED
>    /* Make sure we don't initialize twice.  */
> -  if (!__libc_multiple_libcs)
> +  if (__libc_initial)
>      {
>        /* Set the FPU control word to the proper default value.  */
>        __setfpucw (__fpu_control);
> @@ -72,6 +71,9 @@ posixland_init (int argc, char **argv, char **envp)
>        /* Initialize data structures so the additional libc can do RPCs.  */
>        __mach_init ();
>      }
> +#else /* !SHARED */
> +  __setfpucw (__fpu_control);
> +#endif
>  
>    /* Save the command-line arguments.  */
>    __libc_argc = argc;
> 

Ok.