[1/2] discover: Allow for empty paths

Some grub tests involve a (device)/path structure, where it is
actually legal to have an empty path. Adjust join_path() and
dependant functions to allow for empty pathnames.

Signed-off-by: Klaus Heinrich Kiwi <klaus@linux.vnet.ibm.com>
---
 discover/grub2/builtins.c |  8 ++++----
 discover/grub2/grub2.c    | 19 ++++++++++---------
 discover/paths.c          |  8 +++++---
 test/parser/utils.c       |  2 +-
 4 files changed, 20 insertions(+), 17 deletions(-)

Hi Klaus,

Some grub tests involve a (device)/path structure, where it is
actually legal to have an empty path. Adjust join_path() and
dependant functions to allow for empty pathnames.

Makes sense! Some comments on the implementation though.

In general, we do need to decide whether this situation gives a NULL
value for ->path, or an empty string for ->path. You've gone for the
former here (which I think is the correct choice), but there are are a
lot of assumptions that ->path will be non-null.

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.

--- a/discover/grub2/builtins.c
+++ b/discover/grub2/builtins.c
@@ -216,7 +216,7 @@ static int parse_to_device_path(struct grub2_script
*script,
                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.

diff --git a/discover/grub2/grub2.c b/discover/grub2/grub2.c
index b176ce2..52c75e9 100644
--- a/discover/grub2/grub2.c
+++ b/discover/grub2/grub2.c
@@ -118,7 +118,6 @@ struct grub2_file *grub2_parse_file(struct
grub2_script *script,
                const char *str)
 {
        struct grub2_file *file;
-       size_t dev_len;
        char *pos;
 
        if (!str)
@@ -129,6 +128,7 @@ struct grub2_file *grub2_parse_file(struct
grub2_script *script,
        if (*str != '(') {
                /* just a path - no device, return path as-is */
                file->path = talloc_strdup(file, str);
+               file->dev = NULL;

This is already covered by the talloc_zero.

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

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

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

Cheers,


Jeremy

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.

Hi Klaus,

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

Yep, from reading your v1 patch, it looked like there was no way for
grub2_parse_file to return a NULL ->path, so I don't think you were
hitting that case.

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

This is regarding an empty path specifier, not an empty device. It looks
like you've addressed that in v3 though.


> can we settle for both dev as well as path can EITHER be zero-length
> (but not both at the same time)?

Yep, I think that's a good approach.

Cheers,


Jeremy