[PATCH 0/2] util: Resolve issues with use of grub_util_mkdir

[PATCH 0/2] util: Resolve issues with use of grub_util_mkdir

[Darren Kenny]

Coverity flagged some issues where grub_util_mkdir() was being called but the
return value was not being checked.

After examining the implementation, on UNIX and ACROS, grub_util_mkdir() is a
macro that maps to libc's mkdir(), which returns an int.

But on MS Windows, grub_util_mkdir() is a function with a void return, and
which in turn eventually maps to the MS API CreateDirectory() call.

After discussing with Daniel, it was felt that fixing this by reimplementing
the Windows code for grub_util_mkdir() to return something like the
libc equivalents. This and then testing the return values on each call to
grub_util_mkdir() in the util code is of little benefit when the main usage is
to populate a temporary directory that was created earlier in the code of
grub-mkrescue, which should never fail.

The solution presented in patch 1, to resolve Coverity's concerns is to cast
the return value of the mkdir() command to void in the macro for
grub_util_mkdir().

The other use-case is in the grub_util_mkdir_p() function that creates
a directory tree, as the UNIX command mkdir -p does. This code should at least
try confirm that it is successfully creating the parent directories before it
continues to the sub-directories. This is done by using
grub_util_is_directory() to ensure that the directory exists, and if it
doesn't then to call grub_util_error() with some information and exit.

In fixing this, it was discovered that grub_util_mkdir() was incorrectly being
called with an empty directory name (""), which was due to an error in the
tokenisation being performed.

Darren Kenny (2):
  util: Ignore return value for grub_util_mkdir() on all platforms
  util: confirm directory creation in grub_install_mkdir_p

 include/grub/osdep/hostfile_aros.h | 2 +-
 include/grub/osdep/hostfile_unix.h | 2 +-
 util/grub-install-common.c         | 7 ++++++-
 3 files changed, 8 insertions(+), 3 deletions(-)

-- 
2.31.1

[PATCH 1/2] util: Ignore return value for grub_util_mkdir() on all platforms

[Darren Kenny]

Coverity signaled 2 issues where the return value of grub_util_mkdir()
was not being tested.

The Windows variant of this code defines the function as having no
return value (void), but the UNIX variants all are mapped using a macro
to the libc mkdir() function, which returns an int value.

To be consistent, the mapping should cast to void to for these too.

Fixes: CID 73583
Fixes: CID 73617

Signed-off-by: Darren Kenny <darren.kenny@oracle.com>
---
 include/grub/osdep/hostfile_aros.h | 2 +-
 include/grub/osdep/hostfile_unix.h | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/grub/osdep/hostfile_aros.h b/include/grub/osdep/hostfile_aros.h
index 161fbb7bdfd6..bd2c62c9a716 100644
--- a/include/grub/osdep/hostfile_aros.h
+++ b/include/grub/osdep/hostfile_aros.h
@@ -74,7 +74,7 @@ grub_util_readlink (const char *name, char *buf, size_t bufsize)
   return readlink(name, buf, bufsize);
 }
 
-#define grub_util_mkdir(a) mkdir ((a), 0755)
+#define grub_util_mkdir(a) (void)mkdir ((a), 0755)
 
 struct grub_util_fd
 {
diff --git a/include/grub/osdep/hostfile_unix.h b/include/grub/osdep/hostfile_unix.h
index 17cd3aa8b304..e6f082f259cb 100644
--- a/include/grub/osdep/hostfile_unix.h
+++ b/include/grub/osdep/hostfile_unix.h
@@ -77,7 +77,7 @@ grub_util_readlink (const char *name, char *buf, size_t bufsize)
   return readlink(name, buf, bufsize);
 }
 
-#define grub_util_mkdir(a) mkdir ((a), 0755)
+#define grub_util_mkdir(a) (void)mkdir ((a), 0755)
 
 #if defined (__NetBSD__)
 /* NetBSD uses /boot for its boot block.  */
-- 
2.31.1

[PATCH 2/2] util: confirm directory creation in grub_install_mkdir_p

[Darren Kenny]

Because grub_util_mkdir() is implemented to not return a value on any
platform, grub_instal_mkdir_p can test for success by confirming that
the directory requested exists after attempting to create it, otherwise
it should fail with an error and exit.

While fixing this, a flaw in the logic was shown, where the first match
of the path separator, which almost always was the first character in
the path (e.g. /boot/grub2) would result in creating a directory with an
empty name (i.e. ""). To avoid that, it should skip the handling of the
path separator where p is pointing to the first character.

Signed-off-by: Darren Kenny <darren.kenny@oracle.com>
---
 util/grub-install-common.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/util/grub-install-common.c b/util/grub-install-common.c
index 347558bf5412..035293c2357e 100644
--- a/util/grub-install-common.c
+++ b/util/grub-install-common.c
@@ -173,15 +173,20 @@ grub_install_mkdir_p (const char *dst)
   char *p;
   for (p = t; *p; p++)
     {
-      if (is_path_separator (*p))
+      if (is_path_separator (*p) && p != t)
 	{
 	  char s = *p;
 	  *p = '\0';
 	  grub_util_mkdir (t);
+	  if (!grub_util_is_directory(t))
+	    grub_util_error (_("failed to make directory: '%s'"), t);
+
 	  *p = s;
 	}
     }
   grub_util_mkdir (t);
+  if (!grub_util_is_directory(t))
+    grub_util_error (_("failed to make directory: '%s'"), t);
   free (t);
 }
 
-- 
2.31.1

Re: [PATCH 2/2] util: confirm directory creation in grub_install_mkdir_p

[Vladimir 'phcoder' Serbinenko]

[-- Attachment #1: Type: text/plain, Size: 1972 bytes --]

Did you test the case when some of components exist and are symlinks? E.g.
/temp being a symlinkto /var/tmp

Le mar. 9 août 2022, 15:30, Darren Kenny <darren.kenny@oracle.com> a écrit :

> Because grub_util_mkdir() is implemented to not return a value on any
> platform, grub_instal_mkdir_p can test for success by confirming that
> the directory requested exists after attempting to create it, otherwise
> it should fail with an error and exit.
>
> While fixing this, a flaw in the logic was shown, where the first match
> of the path separator, which almost always was the first character in
> the path (e.g. /boot/grub2) would result in creating a directory with an
> empty name (i.e. ""). To avoid that, it should skip the handling of the
> path separator where p is pointing to the first character.
>
> Signed-off-by: Darren Kenny <darren.kenny@oracle.com>
> ---
>  util/grub-install-common.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/util/grub-install-common.c b/util/grub-install-common.c
> index 347558bf5412..035293c2357e 100644
> --- a/util/grub-install-common.c
> +++ b/util/grub-install-common.c
> @@ -173,15 +173,20 @@ grub_install_mkdir_p (const char *dst)
>    char *p;
>    for (p = t; *p; p++)
>      {
> -      if (is_path_separator (*p))
> +      if (is_path_separator (*p) && p != t)
>         {
>           char s = *p;
>           *p = '\0';
>           grub_util_mkdir (t);
> +         if (!grub_util_is_directory(t))
> +           grub_util_error (_("failed to make directory: '%s'"), t);
> +
>           *p = s;
>         }
>      }
>    grub_util_mkdir (t);
> +  if (!grub_util_is_directory(t))
> +    grub_util_error (_("failed to make directory: '%s'"), t);
>    free (t);
>  }
>
> --
> 2.31.1
>
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel
>

[-- Attachment #2: Type: text/html, Size: 2729 bytes --]

Re: [PATCH 2/2] util: confirm directory creation in grub_install_mkdir_p

[Darren Kenny]

Hi Vladimir,

I admit, that I did not previously.

I just tried it out where one of the components in a deeper path was a
symlink, and it works as I would expect. It will see it as a
directory in grub_util_is_directory() using stat(), since that is
looking at what is pointed to, in comparison to lstat(), which only
looks at the symlink itself.

Thanks,

Darren.

On Tuesday, 2022-08-09 at 16:04:32 +02, Vladimir Serbinenko wrote:
> Did you test the case when some of components exist and are symlinks? E.g.
> /temp being a symlinkto /var/tmp
>
> Le mar. 9 août 2022, 15:30, Darren Kenny <darren.kenny@oracle.com> a écrit :
>
>> Because grub_util_mkdir() is implemented to not return a value on any
>> platform, grub_instal_mkdir_p can test for success by confirming that
>> the directory requested exists after attempting to create it, otherwise
>> it should fail with an error and exit.
>>
>> While fixing this, a flaw in the logic was shown, where the first match
>> of the path separator, which almost always was the first character in
>> the path (e.g. /boot/grub2) would result in creating a directory with an
>> empty name (i.e. ""). To avoid that, it should skip the handling of the
>> path separator where p is pointing to the first character.
>>
>> Signed-off-by: Darren Kenny <darren.kenny@oracle.com>
>> ---
>>  util/grub-install-common.c | 7 ++++++-
>>  1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/util/grub-install-common.c b/util/grub-install-common.c
>> index 347558bf5412..035293c2357e 100644
>> --- a/util/grub-install-common.c
>> +++ b/util/grub-install-common.c
>> @@ -173,15 +173,20 @@ grub_install_mkdir_p (const char *dst)
>>    char *p;
>>    for (p = t; *p; p++)
>>      {
>> -      if (is_path_separator (*p))
>> +      if (is_path_separator (*p) && p != t)
>>         {
>>           char s = *p;
>>           *p = '\0';
>>           grub_util_mkdir (t);
>> +         if (!grub_util_is_directory(t))
>> +           grub_util_error (_("failed to make directory: '%s'"), t);
>> +
>>           *p = s;
>>         }
>>      }
>>    grub_util_mkdir (t);
>> +  if (!grub_util_is_directory(t))
>> +    grub_util_error (_("failed to make directory: '%s'"), t);
>>    free (t);
>>  }
>>
>> --
>> 2.31.1
>>
>>
>> _______________________________________________
>> Grub-devel mailing list
>> Grub-devel@gnu.org
>> https://lists.gnu.org/mailman/listinfo/grub-devel
>>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH 0/2] util: Resolve issues with use of grub_util_mkdir

[Daniel Kiper]

On Tue, Aug 09, 2022 at 01:29:03PM +0000, Darren Kenny wrote:
> Coverity flagged some issues where grub_util_mkdir() was being called but the
> return value was not being checked.
>
> After examining the implementation, on UNIX and ACROS, grub_util_mkdir() is a
> macro that maps to libc's mkdir(), which returns an int.
>
> But on MS Windows, grub_util_mkdir() is a function with a void return, and
> which in turn eventually maps to the MS API CreateDirectory() call.
>
> After discussing with Daniel, it was felt that fixing this by reimplementing
> the Windows code for grub_util_mkdir() to return something like the
> libc equivalents. This and then testing the return values on each call to
> grub_util_mkdir() in the util code is of little benefit when the main usage is
> to populate a temporary directory that was created earlier in the code of
> grub-mkrescue, which should never fail.
>
> The solution presented in patch 1, to resolve Coverity's concerns is to cast
> the return value of the mkdir() command to void in the macro for
> grub_util_mkdir().
>
> The other use-case is in the grub_util_mkdir_p() function that creates
> a directory tree, as the UNIX command mkdir -p does. This code should at least
> try confirm that it is successfully creating the parent directories before it
> continues to the sub-directories. This is done by using
> grub_util_is_directory() to ensure that the directory exists, and if it
> doesn't then to call grub_util_error() with some information and exit.
>
> In fixing this, it was discovered that grub_util_mkdir() was incorrectly being
> called with an empty directory name (""), which was due to an error in the
> tokenisation being performed.
>
> Darren Kenny (2):
>   util: Ignore return value for grub_util_mkdir() on all platforms
>   util: confirm directory creation in grub_install_mkdir_p

Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>

Daniel