[PATCH v4 07/14] elf: Merge the three implementations of _dl_dst_substitute

[Adhemerval Zanella Netto]

On 02/02/25 18:13, Florian Weimer wrote:
> Use one implementation to perform the copying and the counting.
> Use l_origin of the main program as a fallback for objects
> that have not been dlopen'ed (such as ld.so itself, or the vDSO).
> These should not end up as dlopen callers normally, but they
> might one day if a dlopen variant with an explicit caller
> argument is supported for application use.

LGTM, thanks.

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


>  	  else if (len != 0)
> @@ -329,16 +310,20 @@ _dl_dst_substitute (struct link_map *l, const char *input, char *result)
>  	      /* We found a valid DST that we know about, but we could
>  	         not find a replacement value for it, therefore we
>  		 cannot use this path and discard it.  */
> -	      *result = '\0';
> -	      return result;
> +	      alloc_buffer_mark_failed (result);
> +	      return 0;
>  	    }
>  	  else
> -	    /* No DST we recognize.  */
> -	    *wp++ = '$';
> +	    {
> +	      /* No DST we recognize.  */
> +	      ++length;
> +	      alloc_buffer_add_byte (result, '$');

This will incur in alloc_buffer_mark_failed for each bytes, which 
is two word stores for each byte.  Not the most optimized interface,
but I think it should be ok.

> +	    }
>  	}
>        else
>  	{
> -	  *wp++ = *input++;
> +	  ++length;
> +	  alloc_buffer_add_byte (result, *input++);
>  	}
>      }
>    while (*input != '\0');
> @@ -353,15 +338,19 @@ _dl_dst_substitute (struct link_map *l, const char *input, char *result)
>       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))
> +      && !alloc_buffer_has_failed (result)
> +      && !is_trusted_path_normalize (result_start,
> +				     alloc_buffer_next (result, char)
> +				     - result_start))
>      {
> -      *result = '\0';
> -      return result;
> +      alloc_buffer_mark_failed (result);
> +      return 0;
>      }
>  
> -  *wp = '\0';
> +  ++length;
> +  alloc_buffer_add_byte (result, 0);
>  
> -  return result;
> +  return length;
>  }
>  
>  
> @@ -373,30 +362,18 @@ _dl_dst_substitute (struct link_map *l, const char *input, char *result)
>  static char *
>  expand_dynamic_string_token (struct link_map *l, const char *input)
>  {
> -  /* We make two runs over the string.  First we determine how large the
> -     resulting string is and then we copy it over.  Since this is no
> -     frequently executed operation we are looking here not for performance
> -     but rather for code size.  */
> -  size_t cnt;
> -  size_t total;
> -  char *result;
> -
> -  /* Determine the number of DSTs.  */
> -  cnt = _dl_dst_count (input);
> -
> -  /* If we do not have to replace anything simply copy the string.  */
> -  if (__glibc_likely (cnt == 0))
> -    return __strdup (input);
> -
> -  /* Determine the length of the substituted string.  */
> -  total = DL_DST_REQUIRED (l, input, strlen (input), cnt);
> -
> -  /* Allocate the necessary memory.  */
> -  result = (char *) malloc (total + 1);
> +  struct alloc_buffer buf = {};
> +  size_t size = _dl_dst_substitute (l, input, &buf);
> +  char *result = malloc (size);
>    if (result == NULL)
>      return NULL;
> -
> -  return _dl_dst_substitute (l, input, result);
> +  buf = alloc_buffer_create (result, size);
> +  if (_dl_dst_substitute (l, input, &buf) == 0)
> +    /* Mark the expanded string as to be ignored.  */
> +    *result = '\0';
> +  else
> +    assert (!alloc_buffer_has_failed (&buf));
> +  return result;
>  }
>  
>  
> diff --git a/elf/dl-open.c b/elf/dl-open.c
> index 60a1dce9de..4fb77e3ff7 100644
> --- a/elf/dl-open.c
> +++ b/elf/dl-open.c
> @@ -38,7 +38,6 @@
>  #include <gnu/lib-names.h>
>  #include <dl-find_object.h>
>  
> -#include <dl-dst.h>
>  #include <dl-prop.h>
>  
>  
> diff --git a/elf/dl-origin.c b/elf/dl-origin.c
> index 9f6b921b01..5d06f5bbe3 100644
> --- a/elf/dl-origin.c
> +++ b/elf/dl-origin.c
> @@ -21,9 +21,6 @@
>  #include <sys/param.h>
>  #include <ldsodefs.h>
>  
> -#include <dl-dst.h>
> -
> -
>  const char *
>  _dl_get_origin (void)
>  {
> diff --git a/sysdeps/generic/ldsodefs.h b/sysdeps/generic/ldsodefs.h
> index c0785cba04..e8418973ed 100644
> --- a/sysdeps/generic/ldsodefs.h
> +++ b/sysdeps/generic/ldsodefs.h
> @@ -1223,12 +1223,11 @@ extern struct link_map * _dl_get_dl_main_map (void) attribute_hidden;
>  /* Find origin of the executable.  */
>  extern const char *_dl_get_origin (void) attribute_hidden;
>  
> -/* Count DSTs.  */
> -extern size_t _dl_dst_count (const char *name) attribute_hidden;
> -
>  /* Substitute DST values.  */
> -extern char *_dl_dst_substitute (struct link_map *l, const char *name,
> -				 char *result) attribute_hidden;
> +struct alloc_buffer;
> +size_t _dl_dst_substitute (struct link_map *l, const char *name,
> +			   struct alloc_buffer *result)
> +     attribute_hidden __nonnull ((1, 2, 3));
>  
>  /* Open the shared object NAME, relocate it, and run its initializer if it
>     hasn't already been run.  MODE is as for `dlopen' (see <dlfcn.h>).  If
> diff --git a/sysdeps/unix/sysv/linux/dl-origin.c b/sysdeps/unix/sysv/linux/dl-origin.c
> index decdd8ae9e..9c87ca3208 100644
> --- a/sysdeps/unix/sysv/linux/dl-origin.c
> +++ b/sysdeps/unix/sysv/linux/dl-origin.c
> @@ -17,7 +17,6 @@
>     <https://www.gnu.org/licenses/>.  */
>  
>  #include <assert.h>
> -#include <dl-dst.h>
>  #include <fcntl.h>
>  #include <ldsodefs.h>
>  #include <sysdep.h>