Re: [PATCH v2] misc: Allow selective disabling of debug facility names

[Michael Schierl]

Hello Glenn,

Comments below, note that I did not test the patch so maybe I am missing
something.

Am 06.12.2021 um 18:03 schrieb Glenn Washburn:

>   grub_debug_enabled (const char * condition)
>   {
> -  const char *debug;
> +  const char *debug, *found;
> +  grub_size_t clen;
> +  int ret = 0;
>
>     debug = grub_env_get ("debug");
>     if (!debug)
>       return 0;
>
> -  if (grub_strword (debug, "all") || grub_strword (debug, condition))
> -    return 1;
> +  if (grub_strword (debug, "all"))
> +    {
> +      ret = 1;
> +      if (debug[3] == '\0')
> +       return 1;

maybe move the conditional before the assignment of ret?

> +    }
>
> -  return 0;
> +  clen = grub_strlen (condition);
> +  found = debug;
> +  while(1)
> +    {
> +      found = grub_strstr (found+1, condition);

Off by one error: in case the condition is the first one in debug, it
won't be found. And after fixing this...

> +
> +      if (found == NULL)
> +       break;
> +
> +      /* Found condition is not a whole word, so ignore it */
> +      if (*(found + clen) != '\0' && *(found + clen) != ','
> +        && !grub_isspace (*(found + clen)))
> +       continue;
> +
> +      /*
> +       * If found condition is prefixed with '-' and the start is on a word
> +       * boundary, then disable debug. Otherwise, if the start is on a word
> +       * boundary, enable debug. If neither, ignore.
> +       */
> +      if (*(found-1) == '-' && ((found == debug + 1) || (*(found-2) == ','
> +                              || grub_isspace (*(found-2)))))

you will read beyond the start of the string here.

> +       ret = 0;
> +      else if (*(found-1) == ',' || grub_isspace (*(found-1)))

What about the other separators in grub_iswordseparator besides comma?



Regards,


Michael