[PATCH v4 06/14] elf: Disambiguate some failures in _dl_load_cache_lookup

[Adhemerval Zanella Netto]

On 02/02/25 18:13, Florian Weimer wrote:
> Failure to allocate a copy of the string is now distinct from
> a cache lookup failure.  Some infrastructure failures in
> _dl_sysdep_read_whole_file are still treated as cache lookup
> failures, though.


LGTM, thanks.

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

> ---
>  elf/dl-cache.c             | 42 ++++++++++++++++++++++++++------------
>  elf/dl-load.c              |  5 ++++-
>  sysdeps/generic/ldsodefs.h |  6 +++---
>  3 files changed, 36 insertions(+), 17 deletions(-)
> 
> diff --git a/elf/dl-cache.c b/elf/dl-cache.c
> index 300aa1b6dd..c9c5bf549a 100644
> --- a/elf/dl-cache.c
> +++ b/elf/dl-cache.c
> @@ -375,15 +375,21 @@ _dl_cache_libcmp (const char *p1, const char *p2)
>  }
>  
>  
> -/* Look up NAME in ld.so.cache and return the file name stored there, or null
> -   if none is found.  The cache is loaded if it was not already.  If loading
> -   the cache previously failed there will be no more attempts to load it.
> -   The caller is responsible for freeing the returned string.  The ld.so.cache
> -   may be unmapped at any time by a completing recursive dlopen and
> -   this function must take care that it does not return references to
> -   any data in the mapping.  */
> -char *
> -_dl_load_cache_lookup (const char *name)
> +/* Look up NAME in ld.so.cache and write the file name stored there to
> +  *REALNAME, or null if none is found, and return true.  In this case,
> +  the caller is responsible for freeing the string in *REALNAME.  If
> +  there is an error condition that causes the lookup to fail (such as
> +  a failure to allocate memory), the function returns false, and
> +  *REALNAME is unchanged.
> +
> +   The cache is loaded if it was not already.  If loading the cache
> +   previously failed there will be no more attempts to load it.
> +
> +   The ld.so.cache may be unmapped at any time by a completing
> +   recursive dlopen and this function must take care that it does not
> +   return references to any data in the mapping.  */
> +bool
> +_dl_load_cache_lookup (const char *name, char **realname)
>  {
>    /* Print a message if the loading of libs is traced.  */
>    if (__glibc_unlikely (GLRO(dl_debug_mask) & DL_DEBUG_LIBS))
> @@ -459,8 +465,11 @@ _dl_load_cache_lookup (const char *name)
>      }
>  
>    if (cache == (void *) -1)
> -    /* Previously looked for the cache file and didn't find it.  */
> -    return NULL;
> +    {
> +      /* Previously looked for the cache file and didn't find it.  */
> +      *realname = NULL;
> +      return true;
> +    }
>  
>    const char *best;
>    if (cache_new != (void *) -1)
> @@ -486,7 +495,10 @@ _dl_load_cache_lookup (const char *name)
>      _dl_debug_printf ("  trying file=%s\n", best);
>  
>    if (best == NULL)
> -    return NULL;
> +    {
> +      *realname = NULL;
> +      return true;
> +    }
>  
>    /* The double copy is *required* since malloc may be interposed
>       and call dlopen itself whose completion would unmap the data
> @@ -496,7 +508,11 @@ _dl_load_cache_lookup (const char *name)
>    size_t best_len = strlen (best) + 1;
>    temp = alloca (best_len);
>    memcpy (temp, best, best_len);
> -  return __strdup (temp);
> +  char *copy = __strdup (temp);
> +  if (copy == NULL)
> +    return false;
> +  *realname = copy;
> +  return true;
>  }
>  
>  #ifndef MAP_COPY
> diff --git a/elf/dl-load.c b/elf/dl-load.c
> index e328d678b4..b583714c96 100644
> --- a/elf/dl-load.c
> +++ b/elf/dl-load.c
> @@ -2046,7 +2046,10 @@ _dl_map_new_object (struct link_map *loader, const char *name,
>  	{
>  	  /* Check the list of libraries in the file /etc/ld.so.cache,
>  	     for compatibility with Linux's ldconfig program.  */
> -	  char *cached = _dl_load_cache_lookup (name);
> +	  char *cached;
> +	  if (!_dl_load_cache_lookup (name, &cached))
> +	    _dl_signal_error (ENOMEM, NULL, NULL,
> +			      N_("cannot allocate library name"));
>  
>  	  if (cached != NULL)
>  	    {
> diff --git a/sysdeps/generic/ldsodefs.h b/sysdeps/generic/ldsodefs.h
> index 8465cbaa9b..c0785cba04 100644
> --- a/sysdeps/generic/ldsodefs.h
> +++ b/sysdeps/generic/ldsodefs.h
> @@ -1117,9 +1117,9 @@ const struct r_strlenpair *_dl_important_hwcaps (const char *prepend,
>  						 size_t *max_capstrlen)
>    attribute_hidden;
>  
> -/* Look up NAME in ld.so.cache and return the file name stored there,
> -   or null if none is found.  Caller must free returned string.  */
> -extern char *_dl_load_cache_lookup (const char *name) attribute_hidden;
> +/* Look up NAME in ld.so.cache.  */
> +bool _dl_load_cache_lookup (const char *name, char **realname)
> +  attribute_hidden __nonnull ((1, 2)) __attribute__ ((warn_unused_result));
>  
>  /* If the system does not support MAP_COPY we cannot leave the file open
>     all the time since this would create problems when the file is replaced.