[PATCH v3] elf: dl-load: Get rid of alloca usage.

[Joe Simmons-Talbott]

Ping.

On Thu, Oct 19, 2023 at 01:24:48PM -0400, Joe Simmons-Talbott wrote:
> Replace alloca usage with a scratch_buffer.  Change the semantics of
> is_trusted_path_normalize to return 1, 0, or -1 on error.  Change
> _dl_dst_substitute to return NULL on error and update callers to handle
> the NULL return value.
> ---
> Changes to v2:
> * Remove 3 other alloca removals as separate patches.
> * Use "Memory allocation failure" message rather than a new one.
> * Simplify multiple returns into one.
> * Only call is_trusted_path_normalize if check_for_trusted is set.
> * In expand_dynamic_string_token handle a NULL return from
>   _dl_dst_substitute and free malloc'ed memory.
> * Print an error message in fillin_rpath when
>   expand_dynamic_string_token returns NULL.
> 
>  elf/dl-deps.c |  5 +++--
>  elf/dl-load.c | 50 ++++++++++++++++++++++++++++++++++++++------------
>  2 files changed, 41 insertions(+), 14 deletions(-)
> 
> diff --git a/elf/dl-deps.c b/elf/dl-deps.c
> index 0549b4a4ff..16077384b6 100644
> --- a/elf/dl-deps.c
> +++ b/elf/dl-deps.c
> @@ -100,8 +100,9 @@ DST not allowed in SUID/SGID programs"));				      \
>  						   __dst_cnt));		      \
>  									      \
>  	__result = _dl_dst_substitute (l, __str, __newp);		      \
> -									      \
> -	if (*__result == '\0')						      \
> +	if (__result == NULL)						      \
> +	  _dl_signal_error (0, __str, NULL, N_("Memory allocation failure")); \
> +	else if (*__result == '\0')					      \
>  	  {								      \
>  	    /* The replacement for the DST is not known.  We can't	      \
>  	       processed.  */						      \
> diff --git a/elf/dl-load.c b/elf/dl-load.c
> index 2923b1141d..1133421fd8 100644
> --- a/elf/dl-load.c
> +++ b/elf/dl-load.c
> @@ -21,6 +21,7 @@
>  #include <errno.h>
>  #include <fcntl.h>
>  #include <libintl.h>
> +#include <scratch_buffer.h>
>  #include <stdbool.h>
>  #include <stdlib.h>
>  #include <string.h>
> @@ -124,14 +125,21 @@ static const size_t system_dirs_len[] =
>  };
>  #define nsystem_dirs_len array_length (system_dirs_len)
>  
> -static bool
> +static int
>  is_trusted_path_normalize (const char *path, size_t len)
>  {
>    if (len == 0)
> -    return false;
> +    return 0;
> +
> +  struct scratch_buffer sbuf;
> +  scratch_buffer_init (&sbuf);
>  
> -  char *npath = (char *) alloca (len + 2);
> +  if (!scratch_buffer_set_array_size (&sbuf, 1, len + 2))
> +    return -1;
> +
> +  char *npath = sbuf.data;
>    char *wnp = npath;
> +
>    while (*path != '\0')
>      {
>        if (path[0] == '/')
> @@ -167,17 +175,22 @@ is_trusted_path_normalize (const char *path, size_t len)
>  
>    const char *trun = system_dirs;
>  
> +  int r = 0;
>    for (size_t idx = 0; idx < nsystem_dirs_len; ++idx)
>      {
>        if (wnp - npath >= system_dirs_len[idx]
>  	  && memcmp (trun, npath, system_dirs_len[idx]) == 0)
>  	/* Found it.  */
> -	return true;
> +	{
> +          r = 1;
> +	  break;
> +	}
>  
>        trun += system_dirs_len[idx] + 1;
>      }
>  
> -  return false;
> +  scratch_buffer_free (&sbuf);
> +  return r;
>  }
>  
>  /* Given a substring starting at INPUT, just after the DST '$' start
> @@ -270,7 +283,7 @@ _dl_dst_count (const char *input)
>     least equal to the value returned by DL_DST_REQUIRED.  Note that it
>     is possible for a DT_NEEDED, DT_AUXILIARY, and DT_FILTER entries to
>     have colons, but we treat those as literal colons here, not as path
> -   list delimiters.  */
> +   list delimiters.  Returns NULL on failure.  */
>  char *
>  _dl_dst_substitute (struct link_map *l, const char *input, char *result)
>  {
> @@ -362,11 +375,16 @@ _dl_dst_substitute (struct link_map *l, const char *input, char *result)
>       trusted to have designed this correctly.  Only $ORIGIN is tested in
>       this way because it may be manipulated in some ways with hard
>       links.  */
> -  if (__glibc_unlikely (check_for_trusted)
> -      && !is_trusted_path_normalize (result, wp - result))
> +  if (__glibc_unlikely (check_for_trusted))
>      {
> -      *result = '\0';
> -      return result;
> +      int r = is_trusted_path_normalize (result, wp - result);
> +      if (r == -1)
> +	return NULL;
> +      else if (r == 0)
> +	{
> +	  *result = '\0';
> +	  return result;
> +	}
>      }
>  
>    *wp = '\0';
> @@ -406,7 +424,11 @@ expand_dynamic_string_token (struct link_map *l, const char *input)
>    if (result == NULL)
>      return NULL;
>  
> -  return _dl_dst_substitute (l, input, result);
> +  char *r = _dl_dst_substitute (l, input, result);
> +  if (r == NULL)
> +    free (result);
> +
> +  return r;
>  }
>  
>  
> @@ -485,7 +507,11 @@ fillin_rpath (char *rpath, struct r_search_path_elem **result, const char *sep,
>  	  /* expand_dynamic_string_token can return NULL in case of empty
>  	     path or memory allocation failure.  */
>  	  if (cp == NULL)
> -	    continue;
> +	    {
> +	      _dl_signal_error (errno, NULL, NULL,
> +			        N_("empty path or memory allocation failure"));
> +	      continue;
> +	    }
>  
>  	  /* Compute the length after dynamic string token expansion and
>  	     ignore empty paths.  */
> -- 
> 2.39.2
>