diff mbox series

[v3,07/32] elf: Disambiguate some failures in _dl_load_cache_lookup

Message ID 37bdcf0c7d559672bb913bb55cf681d60a6c30d8.1701944612.git.fweimer@redhat.com
State New
Headers
Series RELRO linkmaps |

Checks

Context Check Description
redhat-pt-bot/TryBot-apply_patch success Patch applied to master at the time it was sent

Commit Message

Florian Weimer Dec. 7, 2023, 10:31 a.m. UTC 
  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.
---
 elf/dl-cache.c             | 22 ++++++++++++++++------
 elf/dl-load.c              |  5 ++++-
 sysdeps/generic/ldsodefs.h | 10 +++++++---
 3 files changed, 27 insertions(+), 10 deletions(-)

Comments

Joseph Myers Feb. 19, 2024, 11:07 p.m. UTC | #1 
On Thu, 7 Dec 2023, Florian Weimer wrote:

> diff --git a/elf/dl-cache.c b/elf/dl-cache.c
> index 804bf23222..a3eb960dac 100644
> --- a/elf/dl-cache.c
> +++ b/elf/dl-cache.c
> @@ -398,8 +398,8 @@ _dl_cache_libcmp (const char *p1, const char *p2)
>     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)
> +bool
> +_dl_load_cache_lookup (const char *name, char **realname)

The comment above this function definition needs updating to describe the 
new semantics for return value and result stored in *REALNAME.

> -/* 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.  Return false on memory allocation
> +   failure and do not change *REALNAME.  If lookup fails, return true
> +   and write a null pointer to *REALNAME.  If lookup suceeds, write a

"succeeds"

> +   copy of the full name to *REALNAME (which has to be freed by the
> +   caller).  */
> +bool _dl_load_cache_lookup (const char *name, char **realname)
> +  attribute_hidden __nonnull ((1, 2)) __attribute__ ((warn_unused_result));

Something like the comment here - though the two comments give different 
information, so don't lose the extra information on the definition unless 
you're confident it's no longer relevant.  However, the comment here 
doesn't actually say it returns true when lookup and allocation both 
succeed.
diff mbox series

Patch

diff --git a/elf/dl-cache.c b/elf/dl-cache.c
index 804bf23222..a3eb960dac 100644
--- a/elf/dl-cache.c
+++ b/elf/dl-cache.c
@@ -398,8 +398,8 @@  _dl_cache_libcmp (const char *p1, const char *p2)
    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)
+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))
@@ -475,8 +475,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)
@@ -502,7 +505,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
@@ -512,7 +518,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 65f910f0e5..2084366663 100644
--- a/elf/dl-load.c
+++ b/elf/dl-load.c
@@ -2082,7 +2082,10 @@  _dl_map_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 9b50ddd09f..80f078b65f 100644
--- a/sysdeps/generic/ldsodefs.h
+++ b/sysdeps/generic/ldsodefs.h
@@ -1111,9 +1111,13 @@  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.  Return false on memory allocation
+   failure and do not change *REALNAME.  If lookup fails, return true
+   and write a null pointer to *REALNAME.  If lookup suceeds, write a
+   copy of the full name to *REALNAME (which has to be freed by the
+   caller).  */
+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.