Re: [PATCH] Revert "Detect ld.so and libc.so version inconsistency during startup"

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

On 25/08/22 03:03, Florian Weimer via Libc-alpha wrote:
> This reverts commit 6f85dbf102ad7982409ba0fe96886caeb6389fef.
> 
> Once this change hits the release branches, it will require relinking
> of all statically linked applications before static dlopen works
> again, for the majority of updates on release branches: The NEWS file
> is regularly updated with bug references, so the __libc_early_init
> suffix changes, and static dlopen cannot find the function anymore.
> 
> While this ABI check is still technically correct (we do require
> rebuilding & relinking after glibc updates to keep static dlopen
> working), it is too drastic for stable release branches.

Sounds reasonable, although this is a configure options not enabled by
default.  Maybe extend the notes on either documentation and release wiki
to describe the pitfalls of this option?

> 
> ---
>  INSTALL                                            | 10 ---
>  Makerules                                          | 14 ----
>  NEWS                                               |  7 +-
>  config.make.in                                     |  1 -
>  configure                                          | 11 ---
>  configure.ac                                       |  5 --
>  elf/Makefile                                       |  2 +-
>  elf/Versions                                       |  4 +-
>  ...libc_early_init.c => dl-call-libc-early-init.c} | 23 +++---
>  elf/dl-load.c                                      |  9 +++
>  elf/dl-open.c                                      |  4 +-
>  elf/dl-version.c                                   | 18 -----
>  elf/libc-early-init.h                              | 21 ++---
>  elf/rtld.c                                         | 12 ++-
>  manual/install.texi                                |  9 ---
>  scripts/libc_early_init_name.py                    | 89 ----------------------
>  sysdeps/generic/ldsodefs.h                         |  4 -
>  17 files changed, 45 insertions(+), 198 deletions(-)
> 
> diff --git a/INSTALL b/INSTALL
> index 6470cd3d25..659f75a97f 100644
> --- a/INSTALL
> +++ b/INSTALL
> @@ -120,16 +120,6 @@ if 'CFLAGS' is specified it must enable optimization.  For example:
>       compiler flags which target a later instruction set architecture
>       (ISA).
>  
> -'--with-extra-version-id=STRING'
> -     Use STRING as part of the fingerprint that is used by the dynamic
> -     linker to detect an incompatible version of 'libc.so'.  For
> -     example, STRING could be the full package version and release
> -     string used by a distribution build of the GNU C Library.  This
> -     way, concurrent process creation during a package update will fail
> -     with an error message, _error while loading shared libraries:
> -     /lib64/libc.so.6: ld.so/libc.so mismatch detected (upgrade in
> -     progress?)_, rather than crashing mysteriously.
> -
>  '--with-timeoutfactor=NUM'
>       Specify an integer NUM to scale the timeout of test programs.  This
>       factor can be changed at run time using 'TIMEOUTFACTOR' environment
> diff --git a/Makerules b/Makerules
> index 756c1f181c..d1e139d03c 100644
> --- a/Makerules
> +++ b/Makerules
> @@ -112,20 +112,6 @@ before-compile := $(common-objpfx)first-versions.h \
>  		  $(common-objpfx)ldbl-compat-choose.h $(before-compile)
>  $(common-objpfx)first-versions.h: $(common-objpfx)versions.stmp
>  $(common-objpfx)ldbl-compat-choose.h: $(common-objpfx)versions.stmp
> -
> -# libc_early_init_name.h provides the actual name of the
> -# __libc_early_init function.  It is used as a protocol version marker
> -# between ld.so and libc.so
> -before-compile := $(common-objpfx)libc_early_init_name.h $(before-compile)
> -libc_early_init_name-deps = \
> -  $(..)NEWS $(..)sysdeps/generic/ldsodefs.h $(..)include/link.h
> -$(common-objpfx)libc_early_init_name.h: $(..)scripts/libc_early_init_name.py \
> -  $(common-objpfx)config.make $(libc_early_init_name-deps)
> -	$(PYTHON) $(..)scripts/libc_early_init_name.py \
> -	  --output=$@T \
> -	  --extra-version-id="$(extra-version-id)" \
> -	  $(libc_early_init_name-deps)
> -	$(move-if-change) $@T $@
>  endif # avoid-generated
>  endif # $(build-shared) = yes
>  
> diff --git a/NEWS b/NEWS
> index 9d3c8c5ed8..f9bef48a8f 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -9,12 +9,7 @@ Version 2.37
>  
>  Major new features:
>  
> -* The dynamic loader now prints an error message, "ld.so/libc.so
> -  mismatch detected (upgrade in progress?)" if it detects that the
> -  version of libc.so it loaded comes from a different build of glibc.
> -  The new configure option --with-extra-version-id can be used to
> -  specify an arbitrary string that affects the computation of the
> -  version fingerprint.
> +  [Add new features here]
>  
>  Deprecated and removed features, and other changes affecting compatibility:
>  
> diff --git a/config.make.in b/config.make.in
> index ecaffbfd4b..d7c416cbea 100644
> --- a/config.make.in
> +++ b/config.make.in
> @@ -98,7 +98,6 @@ build-hardcoded-path-in-tests= @hardcoded_path_in_tests@
>  build-pt-chown = @build_pt_chown@
>  have-tunables = @have_tunables@
>  pthread-in-libc = @pthread_in_libc@
> -extra-version-id = @extra_version_id@
>  
>  # Build tools.
>  CC = @CC@
> diff --git a/configure b/configure
> index c576f9f133..ff2c406b3b 100755
> --- a/configure
> +++ b/configure
> @@ -760,7 +760,6 @@ with_headers
>  with_default_link
>  with_nonshared_cflags
>  with_rtld_early_cflags
> -with_extra_version_id
>  with_timeoutfactor
>  enable_sanity_checks
>  enable_shared
> @@ -1482,9 +1481,6 @@ Optional Packages:
>                            build nonshared libraries with additional CFLAGS
>    --with-rtld-early-cflags=CFLAGS
>                            build early initialization with additional CFLAGS
> -  --extra-version-id=STRING
> -                          specify an extra version string to use in internal
> -                          ABI checks
>    --with-timeoutfactor=NUM
>                            specify an integer to scale the timeout
>    --with-cpu=CPU          select code for CPU variant
> @@ -3401,13 +3397,6 @@ fi
>  
>  
>  
> -# Check whether --with-extra-version-id was given.
> -if test "${with_extra_version_id+set}" = set; then :
> -  withval=$with_extra_version_id; extra_version_id="$withval"
> -fi
> -
> -
> -
>  # Check whether --with-timeoutfactor was given.
>  if test "${with_timeoutfactor+set}" = set; then :
>    withval=$with_timeoutfactor; timeoutfactor=$withval
> diff --git a/configure.ac b/configure.ac
> index 68baeee4d7..eb5bc6a131 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -169,11 +169,6 @@ AC_ARG_WITH([rtld-early-cflags],
>  	    [rtld_early_cflags=])
>  AC_SUBST(rtld_early_cflags)
>  
> -AC_ARG_WITH([extra-version-id],
> -	    AS_HELP_STRING([--extra-version-id=STRING],
> -			   [specify an extra version string to use in internal ABI checks]),
> -	    [extra_version_id="$withval"])
> -
>  AC_ARG_WITH([timeoutfactor],
>  	    AS_HELP_STRING([--with-timeoutfactor=NUM],
>  			   [specify an integer to scale the timeout]),
> diff --git a/elf/Makefile b/elf/Makefile
> index bc68150a37..3928a08787 100644
> --- a/elf/Makefile
> +++ b/elf/Makefile
> @@ -52,6 +52,7 @@ routines = \
>  # The core dynamic linking functions are in libc for the static and
>  # profiled libraries.
>  dl-routines = \
> +  dl-call-libc-early-init \
>    dl-close \
>    dl-debug \
>    dl-debug-symbols \
> @@ -64,7 +65,6 @@ dl-routines = \
>    dl-load \
>    dl-lookup \
>    dl-lookup-direct \
> -  dl-lookup_libc_early_init \
>    dl-minimal-malloc \
>    dl-misc \
>    dl-object \
> diff --git a/elf/Versions b/elf/Versions
> index 6260c0fe03..a9ff278de7 100644
> --- a/elf/Versions
> +++ b/elf/Versions
> @@ -29,8 +29,8 @@ libc {
>      __placeholder_only_for_empty_version_map;
>    }
>    GLIBC_PRIVATE {
> -    # A pattern is needed here because the suffix is dynamically generated.
> -    __libc_early_init_*;
> +    # functions used in other libraries
> +    __libc_early_init;
>  
>      # Internal error handling support.  Interposes the functions in ld.so.
>      _dl_signal_exception; _dl_catch_exception;
> diff --git a/elf/dl-lookup_libc_early_init.c b/elf/dl-call-libc-early-init.c
> similarity index 66%
> rename from elf/dl-lookup_libc_early_init.c
> rename to elf/dl-call-libc-early-init.c
> index 64bc287a05..ee9860e3ab 100644
> --- a/elf/dl-lookup_libc_early_init.c
> +++ b/elf/dl-call-libc-early-init.c
> @@ -1,4 +1,4 @@
> -/* Find the address of the __libc_early_init function.
> +/* Invoke the early initialization function in libc.so.
>     Copyright (C) 2020-2022 Free Software Foundation, Inc.
>     This file is part of the GNU C Library.
>  
> @@ -16,21 +16,26 @@
>     License along with the GNU C Library; if not, see
>     <https://www.gnu.org/licenses/>.  */
>  
> +#include <assert.h>
>  #include <ldsodefs.h>
>  #include <libc-early-init.h>
>  #include <link.h>
>  #include <stddef.h>
>  
> -__typeof (__libc_early_init) *
> -_dl_lookup_libc_early_init (struct link_map *libc_map)
> +void
> +_dl_call_libc_early_init (struct link_map *libc_map, _Bool initial)
>  {
> +  /* There is nothing to do if we did not actually load libc.so.  */
> +  if (libc_map == NULL)
> +    return;
> +
>    const ElfW(Sym) *sym
> -    = _dl_lookup_direct (libc_map, LIBC_EARLY_INIT_NAME_STRING,
> -                         LIBC_EARLY_INIT_GNU_HASH,
> +    = _dl_lookup_direct (libc_map, "__libc_early_init",
> +                         0x069682ac, /* dl_new_hash output.  */
>                           "GLIBC_PRIVATE",
>                           0x0963cf85); /* _dl_elf_hash output.  */
> -  if (sym == NULL)
> -    _dl_signal_error (0, libc_map->l_name, NULL, "\
> -ld.so/libc.so mismatch detected (upgrade in progress?)");
> -  return DL_SYMBOL_ADDRESS (libc_map, sym);
> +  assert (sym != NULL);
> +  __typeof (__libc_early_init) *early_init
> +    = DL_SYMBOL_ADDRESS (libc_map, sym);
> +  early_init (initial);
>  }
> diff --git a/elf/dl-load.c b/elf/dl-load.c
> index 00e08b5500..1ad0868dad 100644
> --- a/elf/dl-load.c
> +++ b/elf/dl-load.c
> @@ -31,6 +31,7 @@
>  #include <sys/param.h>
>  #include <sys/stat.h>
>  #include <sys/types.h>
> +#include <gnu/lib-names.h>
>  
>  /* Type for the buffer we put the ELF header and hopefully the program
>     header.  This buffer does not really have to be too large.  In most
> @@ -1465,6 +1466,14 @@ cannot enable executable stack as shared object requires");
>      add_name_to_object (l, ((const char *) D_PTR (l, l_info[DT_STRTAB])
>  			    + l->l_info[DT_SONAME]->d_un.d_val));
>  
> +  /* If we have newly loaded libc.so, update the namespace
> +     description.  */
> +  if (GL(dl_ns)[nsid].libc_map == NULL
> +      && l->l_info[DT_SONAME] != NULL
> +      && strcmp (((const char *) D_PTR (l, l_info[DT_STRTAB])
> +		  + l->l_info[DT_SONAME]->d_un.d_val), LIBC_SO) == 0)
> +    GL(dl_ns)[nsid].libc_map = l;
> +
>    /* _dl_close can only eventually undo the module ID assignment (via
>       remove_slotinfo) if this function returns a pointer to a link
>       map.  Therefore, delay this step until all possibilities for
> diff --git a/elf/dl-open.c b/elf/dl-open.c
> index dcc24130fe..a23e65926b 100644
> --- a/elf/dl-open.c
> +++ b/elf/dl-open.c
> @@ -760,8 +760,8 @@ dl_open_worker_begin (void *a)
>    if (!args->libc_already_loaded)
>      {
>        /* dlopen cannot be used to load an initial libc by design.  */
> -      if (GL(dl_ns)[args->nsid].libc_map != NULL)
> -	GL(dl_ns)[args->nsid].libc_map_early_init (false);
> +      struct link_map *libc_map = GL(dl_ns)[args->nsid].libc_map;
> +      _dl_call_libc_early_init (libc_map, false);
>      }
>  
>    args->worker_continue = true;
> diff --git a/elf/dl-version.c b/elf/dl-version.c
> index d9ec44eed6..cda0889209 100644
> --- a/elf/dl-version.c
> +++ b/elf/dl-version.c
> @@ -23,8 +23,6 @@
>  #include <string.h>
>  #include <ldsodefs.h>
>  #include <_itoa.h>
> -#include <gnu/lib-names.h>
> -#include <libc-early-init.h>
>  
>  #include <assert.h>
>  
> @@ -361,22 +359,6 @@ _dl_check_map_versions (struct link_map *map, int verbose, int trace_mode)
>  	}
>      }
>  
> -  /* Detect a libc.so loaded into this namespace.  The
> -     __libc_early_init lookup below means that we have to do this
> -     after parsing the version data.  */
> -  if (GL(dl_ns)[map->l_ns].libc_map == NULL
> -      && map->l_info[DT_SONAME] != NULL
> -      && strcmp (((const char *) D_PTR (map, l_info[DT_STRTAB])
> -		  + map->l_info[DT_SONAME]->d_un.d_val), LIBC_SO) == 0)
> -    {
> -      /* Look up this symbol early to trigger a mismatch error before
> -	 relocation (which may call IFUNC resolvers, and those can
> -	 have an internal ABI dependency).  */
> -      GL(dl_ns)[map->l_ns].libc_map_early_init
> -	= _dl_lookup_libc_early_init (map);
> -      GL(dl_ns)[map->l_ns].libc_map = map;
> -    }
> -
>    /* When there is a DT_VERNEED entry with libc.so on DT_NEEDED, issue
>       an error if there is a DT_RELR entry without GLIBC_ABI_DT_RELR
>       dependency.  */
> diff --git a/elf/libc-early-init.h b/elf/libc-early-init.h
> index ac8c204bc7..a8edfadfb0 100644
> --- a/elf/libc-early-init.h
> +++ b/elf/libc-early-init.h
> @@ -19,10 +19,13 @@
>  #ifndef _LIBC_EARLY_INIT_H
>  #define _LIBC_EARLY_INIT_H
>  
> -#include <libc_early_init_name.h>
> -
>  struct link_map;
>  
> +/* If LIBC_MAP is not NULL, look up the __libc_early_init symbol in it
> +   and call this function, with INITIAL as the argument.  */
> +void _dl_call_libc_early_init (struct link_map *libc_map, _Bool initial)
> +  attribute_hidden;
> +
>  /* In the shared case, this function is defined in libc.so and invoked
>     from ld.so (or on the fist static dlopen) after complete relocation
>     of a new loaded libc.so, but before user-defined ELF constructors
> @@ -30,18 +33,6 @@ struct link_map;
>     startup code.  If INITIAL is true, the libc being initialized is
>     the libc for the main program.  INITIAL is false for libcs loaded
>     for audit modules, dlmopen, and static dlopen.  */
> -void __libc_early_init (_Bool initial)
> -#ifdef SHARED
> -/* Redirect to the actual implementation name.  */
> -  __asm__ (LIBC_EARLY_INIT_NAME_STRING)
> -#endif
> -  ;
> -
> -/* Attempts to find the appropriately named __libc_early_init function
> -   in LIBC_MAP.  On lookup failure, an exception is signaled,
> -   indicating an ld.so/libc.so mismatch.  */
> -__typeof (__libc_early_init) *_dl_lookup_libc_early_init (struct link_map *
> -                                                          libc_map)
> -  attribute_hidden;
> +void __libc_early_init (_Bool initial);
>  
>  #endif /* _LIBC_EARLY_INIT_H */
> diff --git a/elf/rtld.c b/elf/rtld.c
> index 910075c37f..cbbaf4a331 100644
> --- a/elf/rtld.c
> +++ b/elf/rtld.c
> @@ -1707,6 +1707,15 @@ dl_main (const ElfW(Phdr) *phdr,
>        /* Extract the contents of the dynamic section for easy access.  */
>        elf_get_dynamic_info (main_map, false, false);
>  
> +      /* If the main map is libc.so, update the base namespace to
> +	 refer to this map.  If libc.so is loaded later, this happens
> +	 in _dl_map_object_from_fd.  */
> +      if (main_map->l_info[DT_SONAME] != NULL
> +	  && (strcmp (((const char *) D_PTR (main_map, l_info[DT_STRTAB])
> +		      + main_map->l_info[DT_SONAME]->d_un.d_val), LIBC_SO)
> +	      == 0))
> +	GL(dl_ns)[LM_ID_BASE].libc_map = main_map;
> +
>        /* Set up our cache of pointers into the hash table.  */
>        _dl_setup_hash (main_map);
>      }
> @@ -2377,8 +2386,7 @@ dl_main (const ElfW(Phdr) *phdr,
>    /* Relocation is complete.  Perform early libc initialization.  This
>       is the initial libc, even if audit modules have been loaded with
>       other libcs.  */
> -  if (GL(dl_ns)[LM_ID_BASE].libc_map != NULL)
> -    GL(dl_ns)[LM_ID_BASE].libc_map_early_init (true);
> +  _dl_call_libc_early_init (GL(dl_ns)[LM_ID_BASE].libc_map, true);
>  
>    /* Do any necessary cleanups for the startup OS interface code.
>       We do these now so that no calls are made after rtld re-relocation
> diff --git a/manual/install.texi b/manual/install.texi
> index 6d43599a47..c775005581 100644
> --- a/manual/install.texi
> +++ b/manual/install.texi
> @@ -144,15 +144,6 @@ dynamic linker diagnostics to run on CPUs which are not compatible with
>  the rest of @theglibc{}, for example, due to compiler flags which target
>  a later instruction set architecture (ISA).
>  
> -@item --with-extra-version-id=@var{string}
> -Use @var{string} as part of the fingerprint that is used by the dynamic
> -linker to detect an incompatible version of @file{libc.so}.  For
> -example, @var{string} could be the full package version and release
> -string used by a distribution build of @theglibc{}.  This way,
> -concurrent process creation during a package update will fail with an
> -error message, @emph{ld.so/libc.so mismatch detected (upgrade in
> -progress?)}, rather than crashing mysteriously.
> -
>  @item --with-timeoutfactor=@var{NUM}
>  Specify an integer @var{NUM} to scale the timeout of test programs.
>  This factor can be changed at run time using @env{TIMEOUTFACTOR}
> diff --git a/scripts/libc_early_init_name.py b/scripts/libc_early_init_name.py
> deleted file mode 100644
> index a56c2008f3..0000000000
> --- a/scripts/libc_early_init_name.py
> +++ /dev/null
> @@ -1,89 +0,0 @@
> -#!/usr/bin/python3
> -# Compute the hash-based name of the __libc_early_init function.
> -# Copyright (C) 2022 Free Software Foundation, Inc.
> -# This file is part of the GNU C Library.
> -#
> -# The GNU C Library is free software; you can redistribute it and/or
> -# modify it under the terms of the GNU Lesser General Public
> -# License as published by the Free Software Foundation; either
> -# version 2.1 of the License, or (at your option) any later version.
> -#
> -# The GNU C Library is distributed in the hope that it will be useful,
> -# but WITHOUT ANY WARRANTY; without even the implied warranty of
> -# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> -# Lesser General Public License for more details.
> -#
> -# You should have received a copy of the GNU Lesser General Public
> -# License along with the GNU C Library; if not, see
> -# <https://www.gnu.org/licenses/>.
> -
> -"""Compute the name of the __libc_early_init function, which is used
> -as a protocol version marker between ld.so and libc.so.
> -
> -The name contains a hash suffix, and the hash changes if certain key
> -files in the source tree change.  Distributions can also configure
> -with --with-extra-version-id, to make the computed hash dependent on
> -the package version.
> -
> -"""
> -
> -import argparse
> -import hashlib
> -import os
> -import string
> -import sys
> -
> -def gnu_hash(s):
> -    """Computes the GNU hash of the string."""
> -    h = 5381
> -    for ch in s:
> -        if type(ch) is not int:
> -            ch = ord(ch)
> -        h = (h * 33 + ch) & 0xffffffff
> -    return h
> -
> -# Parse the command line.
> -parser = argparse.ArgumentParser(description=__doc__)
> -parser.add_argument('--output', metavar='PATH',
> -                    help='path to header file this tool generates')
> -parser.add_argument('--extra-version-id', metavar='ID',
> -                    help='extra string to influence hash computation')
> -parser.add_argument('inputs', metavar='PATH', nargs='*',
> -                    help='files whose contents influences the generated hash')
> -opts = parser.parse_args()
> -
> -# Obtain the blobs that affect the generated hash.
> -blobs = [(opts.extra_version_id or '').encode('UTF-8')]
> -for path in opts.inputs:
> -    with open(path, 'rb') as inp:
> -        blobs.append(inp.read())
> -
> -# Hash the file boundaries.
> -md = hashlib.sha256()
> -md.update(repr([len(blob) for blob in blobs]).encode('UTF-8'))
> -
> -# And then hash the file contents.  Do not hash the paths, to avoid
> -# impacting reproducibility.
> -for blob in blobs:
> -    md.update(blob)
> -
> -# These are the bits used to compute the suffix.
> -derived_bits = int.from_bytes(md.digest(), byteorder='big', signed=False)
> -
> -# These digits are used in the suffix (should result in base-62 encoding).
> -# They must be valid in C identifiers.
> -digits = string.digits + string.ascii_letters
> -
> -# Generate eight digits as a suffix.  They should provide enough
> -# uniqueness (47.6 bits).
> -name = '__libc_early_init_'
> -for n in range(8):
> -    name += digits[derived_bits % len(digits)]
> -    derived_bits //= len(digits)
> -
> -# Write the output file.
> -with open(opts.output, 'w') if opts.output else sys.stdout as out:
> -    out.write('#define LIBC_EARLY_INIT_NAME {}\n'.format(name))
> -    out.write('#define LIBC_EARLY_INIT_NAME_STRING "{}"\n'.format(name))
> -    out.write('#define LIBC_EARLY_INIT_GNU_HASH {}\n'.format(
> -        gnu_hash(name)))
> diff --git a/sysdeps/generic/ldsodefs.h b/sysdeps/generic/ldsodefs.h
> index 275dbc95ce..050a3032de 100644
> --- a/sysdeps/generic/ldsodefs.h
> +++ b/sysdeps/generic/ldsodefs.h
> @@ -333,10 +333,6 @@ struct rtld_global
>         its link map.  */
>      struct link_map *libc_map;
>  
> -    /* __libc_early_init function in libc_map.  Initialized at the
> -       same time as libc_map.  */
> -    void (*libc_map_early_init) (_Bool);
> -
>      /* Search table for unique objects.  */
>      struct unique_sym_table
>      {
> 
> base-commit: 025a8cce63a1d9b3ea9e84d0e844f14ec872e184
>