[PATCH 1/2] discover: Allow for empty paths

[Klaus Heinrich Kiwi]

Hi Jeremy, thanks for the review

On 1/20/2021 2:03 AM, Jeremy Kerr wrote:
> Hi Klaus,
> 


> We will need to change the cases that assume ->path is non-null -
> currently, there are cases where this will result in a NULL 'b'
> argument to join_paths, which tries to strlen(b), and crashes.

I honestly did thought of that, but the (first) reference I found
on google was saying that strlen(null) would return zero, but
now looking again, I couldn't find that in a proper standard,
so I'll add the necessary checks.

It's funny, however, that it works (i.e., didn't crash for me on
the zero-length path scenario)


>                  return -1;
>   
>          *devp = dev;
> -       *pathp = talloc_strdup(script, file->path);
> +       *pathp = !file->path ? NULL : talloc_strdup(script, file-
>> path);> talloc_strdup() does the exactly this check, you don't need this
> change.

I guess I was being paranoid and trying to make it solar clear what
was happening, but will remove this (and other similar stuff)


> 
>   
>          } else {
>                  /* device plus path - split into components */
> @@ -137,17 +137,18 @@ struct grub2_file *grub2_parse_file(struct
> grub2_script *script,
>   
>                  /* no closing bracket, or zero-length path? */
>                  if (!pos || *(pos+1) == '\0') {
> -                       talloc_free(file);
> -                       return NULL;
> +                       file->path = NULL;
> +               }
> +               else {
> +                       file->path = talloc_strdup(file, pos + 1);
> +                       file->dev = talloc_strndup(file, str + 1,
> (size_t) (pos - str - 1));
>                  }
> 
> You probably want to separate the missing-closing-bracket case from the
> zero-length-path case here, since you're making the latter a valid
> pathspec.
> 
> Regardless, in the zero-length path case, -> path is set to NULL, ->dev
> is not set either, so this function still returns NULL. Are you sure
> that this fixes the issue in the way that you expect?

I'm not sure if '()/something' is legal, even less '()'. For consistency,
can we settle for both dev as well as path can EITHER be zero-length (but
not both at the same time)?

  
> Also, super minor, but: curly brackets and else on the same line, and
> the last addition there is > 80 chars.
>

oops! (guess Linus allowing up to 100 columns hasn't hit us yet here ;-)

> -
> -               file->path = talloc_strdup(file, pos + 1);
> -
> -               dev_len = pos - str - 1;
> -               if (dev_len)
> -                       file->dev = talloc_strndup(file, str + 1,
> dev_len);
>          }
>   
> +       if (!file->dev && !file->path) {
> +               talloc_free(file);
> +               return NULL;
> +       }
> 
> You've dropped the case where dev_len is zero here - ie., brackets are
> present but empty. Then, since file->dev is a valid pointer to an empty
> string, the pathspec "()" won't hit that last conditional.
> 
> That's not necessarily an issue, but we'd need to be aware of the
> change.

As commented above, I *think* we want to disallow that as it may mean
the script is probably broken, and we'd be masking it making it
difficult to know why.

> 
> Cheers,
> 
> 
> Jeremy
> 
> 

Thanks! I'll send a v3 shortly.

-- 
Klaus Heinrich Kiwi <klaus at linux.vnet.ibm.com>