Re: [PATCH v2 06/19] elf: Do not parse ill-formatted strings

[Siddhesh Poyarekar <siddhesh@sourceware.org>]

On 2023-10-17 09:05, Adhemerval Zanella wrote:
> Instead of ignoring ill-formatted tunable strings, first, check all the
> tunable definitions are correct and then set each tunable value. It
> means that partially invalid strings, like "key1=value1:key2=key2=value'
> or 'key1=value':key2=value2=value2' do not enable 'key1=value1'. It
> avoids possible user-defined errors in tunable definitions.
> 
> Checked on x86_64-linux-gnu.
> ---

Harsher than 5/19, but fair I guess.  Please send v3 with a tiny nit 
fixup I've mentioned below.

Thanks,
Sid

>   elf/dl-tunables.c  | 50 +++++++++++++++++++++++++++++++++++-----------
>   elf/tst-tunables.c | 13 ++++++++----
>   2 files changed, 47 insertions(+), 16 deletions(-)
> 
> diff --git a/elf/dl-tunables.c b/elf/dl-tunables.c
> index 59bee61124..5d4b8c5bc0 100644
> --- a/elf/dl-tunables.c
> +++ b/elf/dl-tunables.c
> @@ -154,17 +154,29 @@ __tunable_set_val (tunable_id_t id, tunable_val_t *valp, tunable_num_t *minp,
>     do_tunable_update_val (cur, valp, minp, maxp);
>   }
>   
> -/* Parse the tunable string VALSTRING.  VALSTRING is a duplicated values,
> -   where delimiters ':' are replaced with '\0', so string tunables are null
> -   terminated.  */
> -static void
> -parse_tunables (char *valstring)
> +struct tunable_toset_t
> +{
> +  tunable_t *t;
> +  const char *value;
> +};
> +
> +enum { tunables_list_size = array_length (tunable_list) };
> +
> +/* Parse the tunable string VALSTRING and set TUNABLES with the found tunables
> +   and their respectibles values.  VALSTRING is a duplicated values,  where
> +   delimiters ':' are replaced with '\0', so string tunables are null
> +   terminated.
> +   Return the number of tunables found (including 0 if the string is empty)
> +   or -1 if for a ill-formatted definition.  */
> +static int
> +parse_tunables_string (char *valstring, struct tunable_toset_t *tunables)
>   {
>     if (valstring == NULL || *valstring == '\0')
> -    return;
> +    return 0;
>   
>     char *p = valstring;
>     bool done = false;
> +  int ntunables = 0;
>   
>     while (!done)
>       {
> @@ -177,7 +189,7 @@ parse_tunables (char *valstring)
>         /* If we reach the end of the string before getting a valid name-value
>   	 pair, bail out.  */
>         if (*p == '\0')
> -	break;
> +	return -1;
>   
>         /* We did not find a valid name-value pair before encountering the
>   	 colon.  */
> @@ -190,30 +202,44 @@ parse_tunables (char *valstring)
>         /* Skip the ':' or '='.  */
>         p++;
>   
> -      const char *value = p;
> +      char *value = p;
>   
>         while (*p != '=' && *p != ':' && *p != '\0')
>   	p++;
>   
>         if (*p == '=')
> -	break;
> +	return -1;
>         else if (*p == '\0')
>   	done = true;
>         else
>   	*p++ = '\0';
>   
>         /* Add the tunable if it exists.  */
> -      for (size_t i = 0; i < sizeof (tunable_list) / sizeof (tunable_t); i++)
> +      for (size_t i = 0; i < tunables_list_size; i++)
>   	{
>   	  tunable_t *cur = &tunable_list[i];
>   
>   	  if (tunable_is_name (cur->name, name))
>   	    {
> -	      tunable_initialize (cur, value);
> +	      tunables[ntunables++] = (struct tunable_toset_t) { cur, value };
>   	      break;
>   	    }
>   	}
>       }
> +
> +  return ntunables;
> +}
> +
> +static void
> +parse_tunables (char *valstring)
> +{
> +  struct tunable_toset_t tunables[tunables_list_size];
> +  int ntunables = parse_tunables_string (valstring, tunables);
> +  if (ntunables == -1)
> +    return;

You don't actually need this; the for loop below will return without 
doing anything if ntunables == -1.

> +
> +  for (int i = 0; i < ntunables; i++)
> +    tunable_initialize (tunables[i].t, tunables[i].value);
>   }
>   
>   /* Initialize the tunables list from the environment.  For now we only use the
> @@ -240,7 +266,7 @@ __tunables_init (char **envp)
>   	  continue;
>   	}
>   
> -      for (int i = 0; i < sizeof (tunable_list) / sizeof (tunable_t); i++)
> +      for (int i = 0; i < tunables_list_size; i++)
>   	{
>   	  tunable_t *cur = &tunable_list[i];
>   
> diff --git a/elf/tst-tunables.c b/elf/tst-tunables.c
> index 03039b5260..e124fa4c6d 100644
> --- a/elf/tst-tunables.c
> +++ b/elf/tst-tunables.c
> @@ -161,7 +161,7 @@ static const struct test_t
>       0,
>       0,
>     },
> -  /* If there is a ill-formatted key=value, everything after is also ignored.  */
> +  /* Ill-formatted tunables string is not parsed.  */
>     {
>       "glibc.malloc.mmap_threshold=glibc.malloc.mmap_threshold=4096:glibc.malloc.check=2",
>       0,
> @@ -186,13 +186,18 @@ static const struct test_t
>       0,
>       0,
>     },
> -  /* Valid tunables set before ill-formatted ones are set.  */
>     {
>       "glibc.malloc.check=2:glibc.malloc.mmap_threshold=4096=4096",
> -    2,
>       0,
>       0,
> -  }
> +    0,
> +  },
> +  {
> +    "glibc.malloc.check=2:glibc.malloc.mmap_threshold=4096=4096",
> +    0,
> +    0,
> +    0,
> +  },
>   };
>   
>   static int