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

[Glenn Washburn]

Hi Michael,

Thanks for taking a look at this.

On Mon, 6 Dec 2021 22:01:22 +0100
Michael Schierl <schierlm@gmx.de> wrote:

> 
> 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?

I'm understanding you to be suggesting to move the assignment of ret to
after the if statement that follows it. The only point I see is saving
an assignment in the case that debug=all. Is there more to it?

> > +    }
> >
> > -  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...

Yep, this is an issue. It was intentional when I had the code only
running in the case "all" was present, in which case skipping the
condition if its the at the start of $debug is fine because the
condition is already enabled. After changing to run also without "all"
being present, this becomes an issue.

> > +
> > +      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.

Yep, that's why I had the +1 above to account for this.

> > +   ret = 0;
> > +      else if (*(found-1) == ',' || grub_isspace (*(found-1)))
> 
> What about the other separators in grub_iswordseparator besides comma?

I was using grub_iswordseparator in the original patch, but decided
against it this round because the documentation states that separators
are whitespace or comma.

Glenn