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

Jeremy Kerr jk at ozlabs.org
Wed Jan 20 16:03:30 AEDT 2021


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




More information about the Petitboot mailing list