[PATCH v2] elf: Fix tunable comma iterator

[Adhemerval Zanella Netto]

On 22/09/26 12:54, Daniel Fellows wrote:
> Empty comma-separated entries leave disable uninitialized.  Initialize
> it to false and only set it for entries beginning with a minus sign.
> 
> Check the input length before accessing the string, and only advance
> past a comma.  This avoids reading beyond the supplied string length.

The patch looks ok, but I think it would be good to add a tests-internal regression 
since we only test this indirectly.  Something like:

--

#include <dl-tunables.h>
#include <dl-tunables-parse.h>

#include <array_length.h>
#include <stdbool.h>
#include <stdio.h>
#include <string.h>
#include <support/check.h>
#include <support/next_to_fault.h>

struct token
{
  const char *str;
  bool disable;
};

struct test_case
{
  /* The suboptions string.  It is copied to a guarded buffer without the null
     terminator, so the iterator must honor the supplied length instead of
    G relying on the terminator.  */
  const char *input;
  /* The expected suboptions.  */
  const struct token *tokens;
  size_t ntokens;
};

#define TOKENS(...) \
  (const struct token[]) { __VA_ARGS__ }, \
  array_length (((const struct token[]) { __VA_ARGS__ }))

static const struct test_case tests[] =
{
  { "", NULL, 0 },
  { "abc", TOKENS ({ "abc", false }) },
  { "a,b", TOKENS ({ "a", false }, { "b", false }) },
  { "a,-b,c", TOKENS ({ "a", false }, { "b", true }, { "c", false }) },
  { "-", TOKENS ({ "", true }) },
  { "--a", TOKENS ({ "-a", true }) },
  { "-a,-", TOKENS ({ "a", true }, { "", true }) },
  /* Empty suboptions are returned as empty strings, not disabled.  */
  { ",", TOKENS ({ "", false }) },
  { ",,", TOKENS ({ "", false }, { "", false }) },
  { ",a", TOKENS ({ "", false }, { "a", false }) },
  { "a,", TOKENS ({ "a", false }) },
  { "a,,b", TOKENS ({ "a", false }, { "", false }, { "b", false }) },
  { "-avx2,,-,avx512f,",
    TOKENS ({ "avx2", true }, { "", false }, { "", true },
            { "avx512f", false }) },
};

static void
check_tokens (const char *input, tunable_val_t *valp,
              const struct token *tokens, size_t ntokens)
{
  struct tunable_str_comma_state_t state;
  tunable_str_comma_init (&state, valp);

  size_t i = 0;
  while (true)
    {
      /* Poison the result so an uninitialized field does not pass by
         accident.  */
      struct tunable_str_comma_t t;
      memset (&t, 0xff, sizeof (t));

      if (!tunable_str_comma_next (&state, &t))
        break;

      if (i >= ntokens)
        FAIL_EXIT1 ("\"%s\": unexpected suboption %zu \"%.*s\"",
                    input, i, (int) t.len, t.str);

      printf ("info: \"%s\": suboption %zu \"%.*s\" (disable=%d)\n",
              input, i, (int) t.len, t.str, (int) t.disable);
      TEST_COMPARE_BLOB (t.str, t.len, tokens[i].str,
                         strlen (tokens[i].str));
      TEST_COMPARE (t.disable, tokens[i].disable);
      i++;
    }

  TEST_COMPARE (i, ntokens);
}

static void
check_guarded (const char *input, const struct token *tokens, size_t ntokens)
{
  size_t len = strlen (input);
  struct support_next_to_fault ntf = support_next_to_fault_allocate (len);
  memcpy (ntf.buffer, input, len);

  tunable_val_t val = { .strval = { ntf.buffer, len } };
  check_tokens (input, &val, tokens, ntokens);

  support_next_to_fault_free (&ntf);
}

static void
check_length (const char *input, size_t len, const struct token *tokens,
              size_t ntokens)
{
  tunable_val_t val = { .strval = { input, len } };
  check_tokens (input, &val, tokens, ntokens);
}

static int
do_test (void)
{
  for (size_t i = 0; i < array_length (tests); i++)
    check_guarded (tests[i].input, tests[i].tokens, tests[i].ntokens);

  /* The suboptions after the supplied length are not visible.  */
  check_length ("abcdef,gh", 4, TOKENS ({ "abcd", false }));
  check_length ("abc,def", 3, TOKENS ({ "abc", false }));
  check_length ("abc,def", 4, TOKENS ({ "abc", false }));
  check_length ("-abc,def", 4, TOKENS ({ "abc", true }));

  /* The null terminator stops the iteration before the supplied  length.  */
  check_length ("a,b", 10, TOKENS ({ "a", false }, { "b", false }));
  check_length ("a,", 10, TOKENS ({ "a", false }));
  check_length (",", 10, TOKENS ({ "", false }));

  return 0;
}

#include <support/test-driver.c>

> ---
> Changes in v2:
> - Check the supplied length before reading from the string.
> - Stop at the end of the string and only advance past commas.
> - Add a comment explaining why decrementing the entry length is safe.
> 
>  sysdeps/generic/dl-tunables-parse.h | 29 +++++++++++++++++------------
>  1 file changed, 17 insertions(+), 12 deletions(-)
> 
> diff --git a/sysdeps/generic/dl-tunables-parse.h b/sysdeps/generic/dl-tunables-parse.h
> index 8ac49bff0b..f69f6d9a3d 100644
> --- a/sysdeps/generic/dl-tunables-parse.h
> +++ b/sysdeps/generic/dl-tunables-parse.h
> @@ -93,29 +93,34 @@ static inline bool
>  tunable_str_comma_next (struct tunable_str_comma_state_t *state,
>  			struct tunable_str_comma_t *str)
>  {
> -  if (*state->p == '\0' || state->plen >= state->maxplen)
> +  if (state->plen >= state->maxplen || *state->p == '\0')
>      return false;
>  
>    const char *c;
> -  for (c = state->p; *c != ','; c++, state->plen++)
> -    if (*c == '\0' || state->plen == state->maxplen)
> +  for (c = state->p; state->plen < state->maxplen;
> +       c++, state->plen++)
> +    if (*c == '\0' || *c == ',')
>        break;
>  
>    str->str = state->p;
>    str->len = c - state->p;
> +  str->disable = false;
>  
> -  if (str->len > 0)
> +  /* For an empty suboption, *str->str is ',' or '\0', so a leading
> +     '-' implies str->len > 0.  */
> +  if (*str->str == '-')
>      {
> -      str->disable = *str->str == '-';
> -      if (str->disable)
> -	{
> -	  str->str = str->str + 1;
> -	  str->len = str->len - 1;
> -	}
> +      str->disable = true;
> +      str->str = str->str + 1;
> +      str->len = str->len - 1;
>      }
>  
> -  state->p = c + 1;
> -  state->plen++;
> +  state->p = c;
> +  if (state->plen < state->maxplen && *c == ',')
> +    {
> +      state->p++;
> +      state->plen++;
> +    }
>  
>    return true;
>  }