[PATCH 0/6] Fix some Coverity low-hanging bugs

[PATCH 0/6] Fix some Coverity low-hanging bugs

[Darren Kenny]

Coverity has flagged a number of small issues that should be fixed to help in
cleaning up the code - these here are primarily memory leaks or uninitialized
variables.

In theory leaked memory is significant, but for short-lived processes it is
minor. 

Similarly for unitinialized variables - some compilers will do the right thing
and zero out the value allocated on the stack, but some won't. So it is better
to be sure of the content that leave it open for possible misuse.

Darren Kenny (6):
  grub-install-common: Fix memory leak in copy_all()
  grub-mkrescue: Fix memory leak in write_part()
  grub-fstest: Fix resource leaks in cmd_cmp()
  grub-mkfont: Fix memory leak in write_font_pf2()
  zfs: Fix possible insecure use of chunk size in zap_leaf_array_get()
  gzio: Fix possible use of uninitialized variable in huft_build()

 grub-core/fs/zfs/zfs.c     | 3 ++-
 grub-core/io/gzio.c        | 2 +-
 util/grub-fstest.c         | 7 ++++++-
 util/grub-install-common.c | 4 +++-
 util/grub-mkfont.c         | 1 +
 util/grub-mkrescue.c       | 1 +
 6 files changed, 14 insertions(+), 4 deletions(-)

-- 
2.27.0

[PATCH 1/6] grub-install-common: Fix memory leak in copy_all()

[Darren Kenny]

The copy_all() function skips a section of code using continue, but
fails to free the memory in srcf first, leaking it.

Fixes: CID 314026

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

diff --git a/util/grub-install-common.c b/util/grub-install-common.c
index 4e212e690c52..0995fa741834 100644
--- a/util/grub-install-common.c
+++ b/util/grub-install-common.c
@@ -753,8 +753,10 @@ copy_all (const char *srcd,
 	continue;
       srcf = grub_util_path_concat (2, srcd, de->d_name);
       if (grub_util_is_special_file (srcf)
-	  || grub_util_is_directory (srcf))
+	  || grub_util_is_directory (srcf)) {
+	free(srcf);
 	continue;
+      }  
       dstf = grub_util_path_concat (2, dstd, de->d_name);
       grub_install_compress_file (srcf, dstf, 1);
       free (srcf);
-- 
2.27.0

[PATCH 2/6] grub-mkrescue: Fix memory leak in write_part()

[Darren Kenny]

In the function write_part(), the value of 'inname' is not used beyond
the grub_util_fopen() call, so is should be freed to avoid leakage.

Fixes: CID 314028

Signed-off-by: Darren Kenny <darren.kenny@oracle.com>
---
 util/grub-mkrescue.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/util/grub-mkrescue.c b/util/grub-mkrescue.c
index fb4dcc6d58f7..d5e4c92fd74d 100644
--- a/util/grub-mkrescue.c
+++ b/util/grub-mkrescue.c
@@ -229,6 +229,7 @@ write_part (FILE *f, const char *srcdir)
   char *inname = grub_util_path_concat (2, srcdir, "partmap.lst");
   char buf[260];
   in = grub_util_fopen (inname, "rb");
+  free(inname);
   if (!in)
     return;
   while (fgets (buf, 256, in))
-- 
2.27.0

[PATCH 3/6] grub-fstest: Fix resource leaks in cmd_cmp()

[Darren Kenny]

In the function cmd_cmp() within the while loop, srcnew and destnew are
being allocated but are never freed either before leaving scope or in
the recursive calls being made to cmd_cmp().

Fixes: CID 314032
Fixes: CID 314045

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

diff --git a/util/grub-fstest.c b/util/grub-fstest.c
index 838656420098..da0751222f88 100644
--- a/util/grub-fstest.c
+++ b/util/grub-fstest.c
@@ -299,10 +299,15 @@ cmd_cmp (char *src, char *dest)
 	  *ptr++ = '/';
 	  strcpy (ptr, entry->d_name);
 
-	  if (grub_util_is_special_file (destnew))
+	  if (grub_util_is_special_file (destnew)) {
+	    free(srcnew);
+	    free(destnew);
 	    continue;
+	  }
 
 	  cmd_cmp (srcnew, destnew);
+	  free(srcnew);
+	  free(destnew);
 	}
       grub_util_fd_closedir (dir);
       return;
-- 
2.27.0

[PATCH 4/6] grub-mkfont: Fix memory leak in write_font_pf2()

[Darren Kenny]

In the function write_font_pf2() memory is allocated for 'font_name' to
construct a new name, but it is not released before returning from the
function, leaking the allocated memory.

Fixes: CID 314015

Signed-off-by: Darren Kenny <darren.kenny@oracle.com>
---
 util/grub-mkfont.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/util/grub-mkfont.c b/util/grub-mkfont.c
index 0fe45a6103dd..97e8d27e91d6 100644
--- a/util/grub-mkfont.c
+++ b/util/grub-mkfont.c
@@ -928,6 +928,7 @@ write_font_pf2 (struct grub_font_info *font_info, char *output_file)
 			     file, output_file);
     }
 
+  free(font_name);
   fclose (file);
 }
 
-- 
2.27.0

[PATCH 5/6] zfs: Fix possible insecure use of chunk size in zap_leaf_array_get()

[Darren Kenny]

In zap_leaf_array_get() the chunk size passed in is considered tainted
by Coverity, and is being used before it is tested for validity.

To fix this the assignment of 'la' is moved until after the test of the
value of 'chunk'.

Fixes: CID 314014

Signed-off-by: Darren Kenny <darren.kenny@oracle.com>
---
 grub-core/fs/zfs/zfs.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/grub-core/fs/zfs/zfs.c b/grub-core/fs/zfs/zfs.c
index 44e4e18147af..e9d7a7d0e4f6 100644
--- a/grub-core/fs/zfs/zfs.c
+++ b/grub-core/fs/zfs/zfs.c
@@ -2229,7 +2229,7 @@ zap_leaf_array_get (zap_leaf_phys_t * l, grub_zfs_endian_t endian, int blksft,
 
   while (bseen < array_len)
     {
-      struct zap_leaf_array *la = &ZAP_LEAF_CHUNK (l, blksft, chunk)->l_array;
+      struct zap_leaf_array *la;
       grub_size_t toread = array_len - bseen;
 
       if (toread > ZAP_LEAF_ARRAY_BYTES)
@@ -2239,6 +2239,7 @@ zap_leaf_array_get (zap_leaf_phys_t * l, grub_zfs_endian_t endian, int blksft,
 	/* Don't use grub_error because this error is to be ignored.  */
 	return GRUB_ERR_BAD_FS;
 
+      la = &ZAP_LEAF_CHUNK (l, blksft, chunk)->l_array;
       grub_memcpy (buf + bseen,la->la_array,  toread);
       chunk = grub_zfs_to_cpu16 (la->la_next, endian);
       bseen += toread;
-- 
2.27.0

[PATCH 6/6] gzio: Fix possible use of uninitialized variable in huft_build()

[Darren Kenny]

In huft_build() it is possible to reach the for loop where 'r' is being
assigned to 'q[j]' without 'r.v' ever being initialized.

Fixes: CID 314024

Signed-off-by: Darren Kenny <darren.kenny@oracle.com>
---
 grub-core/io/gzio.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/grub-core/io/gzio.c b/grub-core/io/gzio.c
index aea86a0a9a92..10156e569c85 100644
--- a/grub-core/io/gzio.c
+++ b/grub-core/io/gzio.c
@@ -447,7 +447,7 @@ huft_build (unsigned *b,	/* code lengths in bits (all assumed <= BMAX) */
   int l;			/* bits per table (returned in m) */
   register unsigned *p;		/* pointer into c[], b[], or v[] */
   register struct huft *q;	/* points to current table */
-  struct huft r;		/* table entry for structure assignment */
+  struct huft r = {0};		/* table entry for structure assignment */
   struct huft *u[BMAX];		/* table stack */
   unsigned v[N_MAX];		/* values in order of bit length */
   register int w;		/* bits before this table == (l * h) */
-- 
2.27.0

Re: [PATCH 0/6] Fix some Coverity low-hanging bugs

[Daniel Kiper]

On Tue, Oct 26, 2021 at 03:02:34PM +0000, Darren Kenny wrote:
> Coverity has flagged a number of small issues that should be fixed to help in
> cleaning up the code - these here are primarily memory leaks or uninitialized
> variables.
>
> In theory leaked memory is significant, but for short-lived processes it is
> minor.
>
> Similarly for unitinialized variables - some compilers will do the right thing
> and zero out the value allocated on the stack, but some won't. So it is better
> to be sure of the content that leave it open for possible misuse.
>
> Darren Kenny (6):
>   grub-install-common: Fix memory leak in copy_all()
>   grub-mkrescue: Fix memory leak in write_part()
>   grub-fstest: Fix resource leaks in cmd_cmp()
>   grub-mkfont: Fix memory leak in write_font_pf2()
>   zfs: Fix possible insecure use of chunk size in zap_leaf_array_get()
>   gzio: Fix possible use of uninitialized variable in huft_build()

For all the patches: Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>

Thank you for fixing these issues.

Daniel