Re: [PATCH 1/2] Replace __libc_multiple_libcs with tri-state __libc_type

[Adhemerval Zanella <adhemerval.zanella@linaro.org>]

On 13/11/2020 13:14, Florian Weimer via Libc-alpha wrote:
> The three states are (1) initial libc without any other libcs loaded
> (yet), (2) initial with other libcs loaded, and (3) secondary libc
> (audit module, inner libc after dlmopen, inner libc as the result of
> static dlopen).
> 
> The three states are required because we want to use sbrk for the
> malloc in the primary libc, whether or not secondary libcs have been
> loaded.  ptmalloc_init currently uses _dl_addr to detect inner
> namespaces because __libc_multiple_libcs is not set up correctly for
> audit modules (because _dl_starting_up is not used on Linux).

Looks good in general, although I am not sure if '__libc_type' name
does show its intent.  Although thing that I am not sure is if we need
to access it through atomics (since dlmopen/static dlopen might be
called by multiple threads and it access does not seemed to be protected
by any lock).

> ---
>  csu/init-first.c                    |  8 +-------
>  csu/libc-start.c                    |  4 +---
>  dlfcn/dlmopen.c                     |  6 ++++++
>  elf/Versions                        |  3 +++
>  elf/dl-open.c                       |  2 +-
>  elf/dl-sysdep.c                     |  2 --
>  elf/libc_early_init.c               | 22 ++++++++++++++++++++++
>  include/libc-internal.h             | 18 +++++++++++++++++-
>  misc/sbrk.c                         | 11 +++++++++--
>  sysdeps/mach/hurd/dl-sysdep.c       |  2 --
>  sysdeps/mach/hurd/i386/init-first.c |  5 +----
>  11 files changed, 61 insertions(+), 22 deletions(-)
> 
> diff --git a/csu/init-first.c b/csu/init-first.c
> index 47aaacdbd0..28fa66ad54 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,10 +46,8 @@ _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)
> +  if (__libc_type != libc_type_secondary)
>      {
>        /* Set the FPU control word to the proper default value if the
>  	 kernel would use a different value.  */

Ok.

> diff --git a/csu/libc-start.c b/csu/libc-start.c
> index 2d4d2ed1f9..7e0c4ce610 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,7 +211,7 @@ LIBC_START_MAIN (int (*main) (int, char **, char ** MAIN_AUXVEC_DECL),
>  # endif
>  
>  # ifdef DL_SYSDEP_OSCHECK
> -  if (!__libc_multiple_libcs)
> +  if (__libc_type != libc_type_secondary)
>      {
>        /* This needs to run to initiliaze _dl_osversion before TLS
>  	 setup might check it.  */

Ok.

> diff --git a/dlfcn/dlmopen.c b/dlfcn/dlmopen.c
> index 1396c818ac..c880c13443 100644
> --- a/dlfcn/dlmopen.c
> +++ b/dlfcn/dlmopen.c
> @@ -22,6 +22,7 @@
>  #include <stddef.h>
>  #include <unistd.h>
>  #include <ldsodefs.h>
> +#include <libc-internal.h>
>  
>  #if !defined SHARED && IS_IN (libdl)
>  
> @@ -79,6 +80,11 @@ void *
>  __dlmopen (Lmid_t nsid, const char *file, int mode DL_CALLER_DECL)
>  {
>  # ifdef SHARED
> +  /* Advertise that we may have multiple libc from now on.  Do not
> +     overwrite a libc_type_secondary value.  */
> +  if (__libc_type == libc_type_initial_only)
> +    __libc_type = libc_type_initial;
> +
>    if (!rtld_active ())
>      return _dlfcn_hook->dlmopen (nsid, file, mode, RETURN_ADDRESS (0));
>  # endif

Ok.

> diff --git a/elf/Versions b/elf/Versions
> index be88c48e6d..39df728f0a 100644
> --- a/elf/Versions
> +++ b/elf/Versions
> @@ -28,6 +28,9 @@ libc {
>      __libc_dlclose; __libc_dlopen_mode; __libc_dlsym; __libc_dlvsym;
>      __libc_early_init;
>  
> +    # Updated from libdl.
> +    __libc_type;
> +
>      # Internal error handling support.  Interposes the functions in ld.so.
>      _dl_signal_exception; _dl_catch_exception;
>      _dl_signal_error; _dl_catch_error;

Ok.

> diff --git a/elf/dl-open.c b/elf/dl-open.c
> index 8769e47051..7b7a9214b8 100644
> --- a/elf/dl-open.c
> +++ b/elf/dl-open.c
> @@ -790,7 +790,7 @@ dl_open_worker (void *a)
>  #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;
> +  __libc_type = libc_type_initial;
>  #endif
>  
>    /* Let the user know about the opencount.  */

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..fec5d6035e 100644
> --- a/elf/libc_early_init.c
> +++ b/elf/libc_early_init.c
> @@ -17,9 +17,14 @@
>     <https://www.gnu.org/licenses/>.  */
>  
>  #include <ctype.h>
> +#include <ldsodefs.h>
>  #include <libc-early-init.h>
> +#include <libc-internal.h>
>  #include <sys/single_threaded.h>
>  
> +char __libc_type __attribute__ ((nocommon));
> +libc_hidden_def (__libc_type)
> +

I am not sure about the __libc_type, it seems a bit too vague and does not
indicate its real usage.  Maybe __libc_multiple_state ?

I also forgot why do we need a noncommon attribute here, is it to force
a linker error on multiple definitions?

>  void
>  __libc_early_init (_Bool initial)
>  {
> @@ -28,4 +33,21 @@ __libc_early_init (_Bool initial)
>  
>    /* Only the outer namespace is marked as single-threaded.  */
>    __libc_single_threaded = initial;
> +
> +#ifdef SHARED
> +  if (initial)
> +    {
> +      if (GLRO (dl_naudit) == 0)
> +        __libc_type = libc_type_initial_only;
> +      else
> +        /* If auditing is active, the initial libc is not the only
> +           libc.  */
> +        __libc_type = libc_type_initial;
> +    }
> +  else
> +    __libc_type = libc_type_secondary;
> +#else
> +  /* There is no auditing support for statically linked binaries.  */
> +  __libc_type = libc_type_initial_only;
> +#endif
>  }

Ok.

> diff --git a/include/libc-internal.h b/include/libc-internal.h
> index 915613c030..7331ff2c51 100644
> --- a/include/libc-internal.h
> +++ b/include/libc-internal.h
> @@ -47,6 +47,22 @@ 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;
> +enum
> +  {
> +    /* The running libc is the only libc in the process image.  It can
> +       transition to libc_type_initial via dlmopen or static
> +       dlopen.  */
> +    libc_type_initial_only,
> +
> +    /* The running libc is the libc for the main program, but other
> +       libcs may have been loaded.  */
> +    libc_type_initial,
> +
> +    /* The running libc is a secondary libc (loaded via dlmopen, to
> +       support auditing, or for static dlopen).  */
> +    libc_type_secondary,
> +  };
> +extern char __libc_type;
> +libc_hidden_proto (__libc_type)
>  
>  #endif /* _LIBC_INTERNAL  */

Ok.

> diff --git a/misc/sbrk.c b/misc/sbrk.c
> index ba3322fba6..88840b426b 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;
> @@ -37,7 +38,13 @@ __sbrk (intptr_t increment)
>       __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)
> +#if IS_IN (rtld)
> +  bool update_curbrk = __curbrk == NULL;
> +#else
> +  bool update_curbrk = (__curbrk == NULL
> +			|| __libc_type != libc_type_initial_only);
> +#endif
> +  if (update_curbrk)
>      if (__brk (0) < 0)		/* Initialize the break.  */
>        return (void *) -1;
>  

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..8497089f7a 100644
> --- a/sysdeps/mach/hurd/i386/init-first.c
> +++ b/sysdeps/mach/hurd/i386/init-first.c
> @@ -40,7 +40,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 +55,11 @@ 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);
>  
>    /* Make sure we don't initialize twice.  */
> -  if (!__libc_multiple_libcs)
> +  if (__libc_type != libc_type_secondary)
>      {
>        /* Set the FPU control word to the proper default value.  */
>        __setfpucw (__fpu_control);
> 

Ok.