[01/28] elf: Do not search HWCAP subdirectories in statically linked binaries

  This functionality does not seem to be useful since static dlopen
is mostly used for iconv/character set conversion and NSS support.
gconv modules are loaded with full paths anyway, so that the
HWCAP subdirectory logic does not apply.
---
 NEWS          |  4 ++++
 elf/Makefile  |  4 ++--
 elf/dl-load.c | 14 ++++++++++++++
 3 files changed, 20 insertions(+), 2 deletions(-)
  

On 01/10/2020 13:31, Florian Weimer via Libc-alpha wrote:
> This functionality does not seem to be useful since static dlopen
> is mostly used for iconv/character set conversion and NSS support.
> gconv modules are loaded with full paths anyway, so that the
> HWCAP subdirectory logic does not apply.

This change looks reasonable, although it makes the semantic of 
statically linked programs slight different than dynamic one regarding
dlopen.

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

> ---
>  NEWS          |  4 ++++
>  elf/Makefile  |  4 ++--
>  elf/dl-load.c | 14 ++++++++++++++
>  3 files changed, 20 insertions(+), 2 deletions(-)
> 
> diff --git a/NEWS b/NEWS
> index ce05d05b16..902fa3a7f8 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -33,6 +33,10 @@ Deprecated and removed features, and other changes affecting compatibility:
>  * The mallinfo function is marked deprecated.  Callers should call
>    mallinfo2 instead.
>  
> +* When dlopen is used in statically linked programs, alternative library
> +  implementations from HWCAP subdirectories are no longer loaded.
> +  Instead, the default implementation is used.
> +
>  Changes to build and runtime requirements:
>  
>    [Add changes to build and runtime requirements here]

Ok.

> diff --git a/elf/Makefile b/elf/Makefile
> index c587e9f06e..e0a8bf2998 100644
> --- a/elf/Makefile
> +++ b/elf/Makefile
> @@ -29,7 +29,7 @@ routines	= $(all-dl-routines) dl-support dl-iteratephdr \
>  
>  # The core dynamic linking functions are in libc for the static and
>  # profiled libraries.
> -dl-routines	= $(addprefix dl-,load lookup object reloc deps hwcaps \
> +dl-routines	= $(addprefix dl-,load lookup object reloc deps \
>  				  runtime init fini debug misc \
>  				  version profile tls origin scope \
>  				  execstack open close trampoline \

Ok.

> @@ -59,7 +59,7 @@ elide-routines.os = $(all-dl-routines) dl-support enbl-secure dl-origin \
>  # ld.so uses those routines, plus some special stuff for being the program
>  # interpreter and operating independent of libc.
>  rtld-routines	= rtld $(all-dl-routines) dl-sysdep dl-environ dl-minimal \
> -  dl-error-minimal dl-conflict
> +  dl-error-minimal dl-conflict dl-hwcaps
>  all-rtld-routines = $(rtld-routines) $(sysdep-rtld-routines)
>  
>  CFLAGS-dl-runtime.c += -fexceptions -fasynchronous-unwind-tables

Ok.

> diff --git a/elf/dl-load.c b/elf/dl-load.c
> index 646c5dca40..5ba117d597 100644
> --- a/elf/dl-load.c
> +++ b/elf/dl-load.c
> @@ -101,9 +101,13 @@ int __stack_prot attribute_hidden attribute_relro
>  static struct r_search_path_struct env_path_list attribute_relro;
>  
>  /* List of the hardware capabilities we might end up using.  */
> +#ifdef SHARED
>  static const struct r_strlenpair *capstr attribute_relro;
>  static size_t ncapstr attribute_relro;
>  static size_t max_capstrlen attribute_relro;
> +#else
> +enum { ncapstr = 1, max_capstrlen = 0 };
> +#endif
>  
>  
>  /* Get the generated information about the trusted directories.  Use

Ok.

> @@ -691,9 +695,11 @@ _dl_init_paths (const char *llp)
>    /* Fill in the information about the application's RPATH and the
>       directories addressed by the LD_LIBRARY_PATH environment variable.  */
>  
> +#ifdef SHARED
>    /* Get the capabilities.  */
>    capstr = _dl_important_hwcaps (GLRO(dl_platform), GLRO(dl_platformlen),
>  				 &ncapstr, &max_capstrlen);
> +#endif
>  
>    /* First set up the rest of the default search directory entries.  */
>    aelem = rtld_search_dirs.dirs = (struct r_search_path_elem **)
> @@ -1521,11 +1527,15 @@ print_search_path (struct r_search_path_elem **list,
>        for (cnt = 0; cnt < ncapstr; ++cnt)
>  	if ((*list)->status[cnt] != nonexisting)
>  	  {
> +#ifdef SHARED
>  	    char *cp = __mempcpy (endp, capstr[cnt].str, capstr[cnt].len);
>  	    if (cp == buf || (cp == buf + 1 && buf[0] == '/'))
>  	      cp[0] = '\0';
>  	    else
>  	      cp[-1] = '\0';
> +#else
> +	    *endp = '\0';
> +#endif
>  
>  	    _dl_debug_printf_c (first ? "%s" : ":%s", buf);
>  	    first = 0;
> @@ -1886,11 +1896,15 @@ open_path (const char *name, size_t namelen, int mode,
>  	  if (this_dir->status[cnt] == nonexisting)
>  	    continue;
>  
> +#ifdef SHARED
>  	  buflen =
>  	    ((char *) __mempcpy (__mempcpy (edp, capstr[cnt].str,
>  					    capstr[cnt].len),
>  				 name, namelen)
>  	     - buf);
> +#else
> +	  buflen = (char *) __mempcpy (edp, name, namelen) - buf;
> +#endif
>  
>  	  /* Print name we try if this is wanted.  */
>  	  if (__glibc_unlikely (GLRO(dl_debug_mask) & DL_DEBUG_LIBS))
> 

Ok.
  

On 10/1/20 2:22 PM, Adhemerval Zanella via Libc-alpha wrote:
> 
> 
> On 01/10/2020 13:31, Florian Weimer via Libc-alpha wrote:
>> This functionality does not seem to be useful since static dlopen
>> is mostly used for iconv/character set conversion and NSS support.
>> gconv modules are loaded with full paths anyway, so that the
>> HWCAP subdirectory logic does not apply.
> 
> This change looks reasonable, although it makes the semantic of 
> statically linked programs slight different than dynamic one regarding
> dlopen.

I think this is OK.

dlopen from statically linked programs is deprecated, the only uses
should be internal to glibc in NSS And iconv.

We really really want:

* Just a statically linked program that has no external deps.

* Just a dynamically linked program can support dlopen.

This half-way inbetween support we have has serious consequences,
and is the reason I supported us deprecating dlopen from static
binaries. Users should not be allowed to shoot themselves in
the foot.

$0.02.
  

On 01/10/2020 15:24, Carlos O'Donell wrote:
> On 10/1/20 2:22 PM, Adhemerval Zanella via Libc-alpha wrote:
>>
>>
>> On 01/10/2020 13:31, Florian Weimer via Libc-alpha wrote:
>>> This functionality does not seem to be useful since static dlopen
>>> is mostly used for iconv/character set conversion and NSS support.
>>> gconv modules are loaded with full paths anyway, so that the
>>> HWCAP subdirectory logic does not apply.
>>
>> This change looks reasonable, although it makes the semantic of 
>> statically linked programs slight different than dynamic one regarding
>> dlopen.
> 
> I think this is OK.
> 
> dlopen from statically linked programs is deprecated, the only uses
> should be internal to glibc in NSS And iconv.
> 
> We really really want:
> 
> * Just a statically linked program that has no external deps.
> 
> * Just a dynamically linked program can support dlopen.
> 
> This half-way inbetween support we have has serious consequences,
> and is the reason I supported us deprecating dlopen from static
> binaries. Users should not be allowed to shoot themselves in
> the foot.
> 
> $0.02.
> 

I agree, usually on discussion about static linking with glibc users
get perplexed why shared libraries are being loaded under the hood
and usually this is a deal breaker on most usercases (another one
is the minimum kernel requirement).
  

On 10/1/20 2:29 PM, Adhemerval Zanella wrote:
> 
> 
> On 01/10/2020 15:24, Carlos O'Donell wrote:
>> On 10/1/20 2:22 PM, Adhemerval Zanella via Libc-alpha wrote:
>>>
>>>
>>> On 01/10/2020 13:31, Florian Weimer via Libc-alpha wrote:
>>>> This functionality does not seem to be useful since static dlopen
>>>> is mostly used for iconv/character set conversion and NSS support.
>>>> gconv modules are loaded with full paths anyway, so that the
>>>> HWCAP subdirectory logic does not apply.
>>>
>>> This change looks reasonable, although it makes the semantic of 
>>> statically linked programs slight different than dynamic one regarding
>>> dlopen.
>>
>> I think this is OK.
>>
>> dlopen from statically linked programs is deprecated, the only uses
>> should be internal to glibc in NSS And iconv.
>>
>> We really really want:
>>
>> * Just a statically linked program that has no external deps.
>>
>> * Just a dynamically linked program can support dlopen.
>>
>> This half-way inbetween support we have has serious consequences,
>> and is the reason I supported us deprecating dlopen from static
>> binaries. Users should not be allowed to shoot themselves in
>> the foot.
>>
>> $0.02.
>>
> 
> I agree, usually on discussion about static linking with glibc users
> get perplexed why shared libraries are being loaded under the hood
> and usually this is a deal breaker on most usercases (another one
> is the minimum kernel requirement).
 Fixing the dlopen case requires splitting the interface at the NSS
boundary (as Florian suggested?) and calling out from the static
binary to some kind of 'getent'-like binary, perhaps even getent
itself in compat cases.

The minimum kernel requirement is much easier to explain. We have
a minimum requirement. I find users get that when it's explained
clearly.