[PATCH 0/9] fix memory leaks in fs module

[PATCH 0/9] fix memory leaks in fs module

[t.feng]

Hi all,
I am doing code review in grub-core/fs module and I found some
memory leaks.

*************************

t.feng (9):
  fs/affs:Fix memory leaks in grub_affs_create_node
  fs/btrfs: Fix memory leak in find_path
  fs/minix: Fix memory leak in grub_minix_lookup_symlink
  fs/ntfs: Fix memory leak in  grub_ntfs_read_symlink
  fs/bfs: Fix memory leak in read_bfs_file
  fs/hfsplus: Fix memory leak in grub_hfsplus_btree_search
  fs/iso9660: Fix memory leak in grub_iso9660_susp_iterate
  fs/squash4: Fix memeory leak in grub_squash_iterate_dir
  fs/xfs: Fix memory leaks in xfs

 grub-core/fs/affs.c    | 11 +++--------
 grub-core/fs/bfs.c     |  2 ++
 grub-core/fs/btrfs.c   |  7 ++++++-
 grub-core/fs/hfsplus.c |  5 ++++-
 grub-core/fs/iso9660.c | 10 ++++++++--
 grub-core/fs/minix.c   | 11 +++++++++--
 grub-core/fs/ntfs.c    | 26 ++++++++++++++++++++++----
 grub-core/fs/squash4.c | 20 ++++++++++++++++----
 grub-core/fs/xfs.c     | 11 +++++++++--
 9 files changed, 79 insertions(+), 24 deletions(-)

-- 
2.27.0

[PATCH 1/9] fs/affs:Fix memory leaks in grub_affs_create_node

[t.feng]

hashtable is unfreed in case GRUB_AFFS_FILETYPE_HARDLINK if
grub_disk_read failed. Because, if grub_affs_create_node
return not zero, the hashtable should be freed.
By the way hashtable is unused in grub_affs_create_node, so we can
remove the parameter and take care of it in grub_affs_iterate_dir.
(which one allocate the memory and it should be responsible for releasing)

This is why commit ebf32bc4e9(fs/affs: Fix resource leaks), missing
this memory leak.

Fixs: ebf32bc4e9(fs/affs: Fix resource leaks)

Signed-off-by: "t.feng" <fengtao40@huawei.com>
---
 grub-core/fs/affs.c | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/grub-core/fs/affs.c b/grub-core/fs/affs.c
index 631d3d58a..ed606b3f1 100644
--- a/grub-core/fs/affs.c
+++ b/grub-core/fs/affs.c
@@ -321,7 +321,6 @@ static int
 grub_affs_create_node (grub_fshelp_node_t dir,
 		       grub_fshelp_iterate_dir_hook_t hook, void *hook_data,
 		       struct grub_fshelp_node **node,
-		       grub_uint32_t **hashtable,
 		       grub_uint32_t block, const struct grub_affs_file *fil)
 {
   struct grub_affs_data *data = dir->data;
@@ -332,10 +331,7 @@ grub_affs_create_node (grub_fshelp_node_t dir,
 
   *node = grub_zalloc (sizeof (**node));
   if (!*node)
-    {
-      grub_free (*hashtable);
-      return 1;
-    }
+    return 1;
 
   (*node)->data = data;
   (*node)->block = block;
@@ -395,7 +391,6 @@ grub_affs_create_node (grub_fshelp_node_t dir,
 
   if (hook ((char *) name_u8, type, *node, hook_data))
     {
-      grub_free (*hashtable);
       *node = 0;
       return 1;
     }
@@ -460,11 +455,11 @@ grub_affs_iterate_dir (grub_fshelp_node_t dir,
 	  if (grub_errno)
 	    goto fail;
 
-	  if (grub_affs_create_node (dir, hook, hook_data, &node, &hashtable,
-				     next, &file))
+	  if (grub_affs_create_node (dir, hook, hook_data, &node, next, &file))
 	    {
 	      /* Node has been replaced in function. */
 	      grub_free (orig_node);
+	      grub_free (hashtable);
 	      return 1;
 	    }
 
-- 
2.27.0

[PATCH 2/9] fs/btrfs: Fix memory leak in find_path

[t.feng]

Fix memory leak in find_path.

Fixs: 82591fa6e(Make / in btrfe refer to real root)

Signed-off-by: "t.feng" <fengtao40@huawei.com>
---
 grub-core/fs/btrfs.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/grub-core/fs/btrfs.c b/grub-core/fs/btrfs.c
index ec72f7be3..19bff4610 100644
--- a/grub-core/fs/btrfs.c
+++ b/grub-core/fs/btrfs.c
@@ -1982,7 +1982,12 @@ find_path (struct grub_btrfs_data *data,
 	    {
 	      err = get_root (data, key, tree, type);
 	      if (err)
-		return err;
+		{
+		  grub_free (direl);
+		  grub_free (path_alloc);
+		  grub_free (origpath);
+		  return err;
+		}
 	    }
 	  continue;
 	}
-- 
2.27.0

[PATCH 3/9] fs/minix: Fix memory leak in grub_minix_lookup_symlink

[t.feng]

Fix memory leaks in grub_minix_lookup_symlink.

Fixes: a07e6ad01(Remove variable length arrays)

Signed-off-by: "t.feng" <fengtao40@huawei.com>
---
 grub-core/fs/minix.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/grub-core/fs/minix.c b/grub-core/fs/minix.c
index 953df1191..2176b6e93 100644
--- a/grub-core/fs/minix.c
+++ b/grub-core/fs/minix.c
@@ -374,7 +374,10 @@ grub_minix_lookup_symlink (struct grub_minix_data *data, grub_minix_ino_t ino)
   if (!symlink)
     return grub_errno;
   if (grub_minix_read_file (data, 0, 0, 0, sz, symlink) < 0)
-    return grub_errno;
+    {
+      grub_free (symlink);
+      return grub_errno;
+    }
 
   symlink[sz] = '\0';
 
@@ -384,10 +387,14 @@ grub_minix_lookup_symlink (struct grub_minix_data *data, grub_minix_ino_t ino)
 
   /* Now load in the old inode.  */
   if (grub_minix_read_inode (data, ino))
-    return grub_errno;
+    {
+      grub_free (symlink);
+      return grub_errno;
+    }
 
   grub_minix_find_file (data, symlink);
 
+  grub_free (symlink);
   return grub_errno;
 }
 
-- 
2.27.0

Re: [PATCH 3/9] fs/minix: Fix memory leak in grub_minix_lookup_symlink

[Daniel Kiper]

On Sat, Nov 19, 2022 at 06:39:40PM +0800, t.feng via Grub-devel wrote:
> Fix memory leaks in grub_minix_lookup_symlink.
>
> Fixes: a07e6ad01(Remove variable length arrays)
>
> Signed-off-by: "t.feng" <fengtao40@huawei.com>
> ---
>  grub-core/fs/minix.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/grub-core/fs/minix.c b/grub-core/fs/minix.c
> index 953df1191..2176b6e93 100644
> --- a/grub-core/fs/minix.c
> +++ b/grub-core/fs/minix.c
> @@ -374,7 +374,10 @@ grub_minix_lookup_symlink (struct grub_minix_data *data, grub_minix_ino_t ino)
>    if (!symlink)
>      return grub_errno;
>    if (grub_minix_read_file (data, 0, 0, 0, sz, symlink) < 0)
> -    return grub_errno;
> +    {
> +      grub_free (symlink);
> +      return grub_errno;
> +    }
>
>    symlink[sz] = '\0';
>
> @@ -384,10 +387,14 @@ grub_minix_lookup_symlink (struct grub_minix_data *data, grub_minix_ino_t ino)
>
>    /* Now load in the old inode.  */
>    if (grub_minix_read_inode (data, ino))
> -    return grub_errno;
> +    {
> +      grub_free (symlink);
> +      return grub_errno;
> +    }
>
>    grub_minix_find_file (data, symlink);
>

This could be made simpler. Just add a label "fail" here and grub_free(symlink)
underneath. Then add "goto fail" above.

> +  grub_free (symlink);
>    return grub_errno;
>  }

Daniel

[PATCH 4/9] fs/ntfs: Fix memory leak in grub_ntfs_read_symlink

[t.feng]

Fix memory leaks in grub_ntfs_read_symlink.

Fixes: 5773fb641(Support NTFS reparse points.)

Signed-off-by: "t.feng" <fengtao40@huawei.com>
---
 grub-core/fs/ntfs.c | 26 ++++++++++++++++++++++----
 1 file changed, 22 insertions(+), 4 deletions(-)

diff --git a/grub-core/fs/ntfs.c b/grub-core/fs/ntfs.c
index 3511e4e2c..eb5170ab7 100644
--- a/grub-core/fs/ntfs.c
+++ b/grub-core/fs/ntfs.c
@@ -667,20 +667,27 @@ grub_ntfs_read_symlink (grub_fshelp_node_t node)
     return NULL;
 
   if (read_mft (mft->data, mft->buf, mft->ino))
-    return NULL;
+    {
+      grub_free (mft->buf);
+      return NULL;
+    }
 
   pa = locate_attr (&mft->attr, mft, GRUB_NTFS_AT_SYMLINK);
   if (pa == NULL)
     {
       grub_error (GRUB_ERR_BAD_FS, "no $SYMLINK in MFT 0x%llx",
 		  (unsigned long long) mft->ino);
+      grub_free (mft->buf);
       return NULL;
     }
 
   err = read_attr (&mft->attr, (grub_uint8_t *) &symdesc, 0,
 		   sizeof (struct symlink_descriptor), 1, 0, 0);
   if (err)
-    return NULL;
+    {
+      grub_free (mft->buf);
+      return NULL;
+    }
 
   switch (grub_cpu_to_le32 (symdesc.type))
     {
@@ -697,23 +704,34 @@ grub_ntfs_read_symlink (grub_fshelp_node_t node)
     default:
       grub_error (GRUB_ERR_BAD_FS, "symlink type invalid (%x)",
 		  grub_cpu_to_le32 (symdesc.type));
+      grub_free (mft->buf);
       return NULL;
     }
 
   buf16 = grub_malloc (len);
   if (!buf16)
-    return NULL;
+    {
+      grub_free (mft->buf);
+      return NULL;
+    }
 
   err = read_attr (&mft->attr, buf16, off, len, 1, 0, 0);
   if (err)
-    return NULL;
+    {
+      grub_free (mft->buf);
+      grub_free (buf16);
+      return NULL;
+    }
 
   buf = get_utf8 (buf16, len / 2);
   if (!buf)
     {
+      grub_free (mft->buf);
       grub_free (buf16);
       return NULL;
     }
+
+  grub_free (mft->buf);
   grub_free (buf16);
 
   for (end = buf; *end; end++)
-- 
2.27.0

Re: [PATCH 4/9] fs/ntfs: Fix memory leak in grub_ntfs_read_symlink

[Daniel Kiper]

On Sat, Nov 19, 2022 at 06:39:41PM +0800, t.feng via Grub-devel wrote:
> Fix memory leaks in grub_ntfs_read_symlink.
>
> Fixes: 5773fb641(Support NTFS reparse points.)
>
> Signed-off-by: "t.feng" <fengtao40@huawei.com>
> ---
>  grub-core/fs/ntfs.c | 26 ++++++++++++++++++++++----
>  1 file changed, 22 insertions(+), 4 deletions(-)
>
> diff --git a/grub-core/fs/ntfs.c b/grub-core/fs/ntfs.c
> index 3511e4e2c..eb5170ab7 100644
> --- a/grub-core/fs/ntfs.c
> +++ b/grub-core/fs/ntfs.c
> @@ -667,20 +667,27 @@ grub_ntfs_read_symlink (grub_fshelp_node_t node)
>      return NULL;
>
>    if (read_mft (mft->data, mft->buf, mft->ino))
> -    return NULL;
> +    {
> +      grub_free (mft->buf);
> +      return NULL;
> +    }

I would use "goto fail" construct in this patch too...

Daniel

[PATCH 5/9] fs/bfs: Fix memory leak in read_bfs_file

[t.feng]

Fix memory leaks in read_bfs_file.
l1_entries and l2_entries are forgotten to be freed if
end of reading file.

Fixes: 5825b3794(BFS implementation based on the specification.)

Signed-off-by: "t.feng" <fengtao40@huawei.com>
---
 grub-core/fs/bfs.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/grub-core/fs/bfs.c b/grub-core/fs/bfs.c
index a75876010..07cb3e3ac 100644
--- a/grub-core/fs/bfs.c
+++ b/grub-core/fs/bfs.c
@@ -416,6 +416,8 @@ read_bfs_file (grub_disk_t disk,
 	len -= read_size;
 	buf = (char *) buf + read_size;
       }
+    grub_free (l1_entries);
+    grub_free (l2_entries);
     return GRUB_ERR_NONE;
   }
 }
-- 
2.27.0

[PATCH 6/9] fs/hfsplus: Fix memory leak in grub_hfsplus_btree_search

[t.feng]

Fix memory leak in grub_hfsplus_btree_search.

Fixes: 58ea11d5b(Don't fetch a key beyond the end of the node)

Signed-off-by: "t.feng" <fengtao40@huawei.com>
---
 grub-core/fs/hfsplus.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/grub-core/fs/hfsplus.c b/grub-core/fs/hfsplus.c
index 6337cbfcb..11393ca34 100644
--- a/grub-core/fs/hfsplus.c
+++ b/grub-core/fs/hfsplus.c
@@ -652,7 +652,10 @@ grub_hfsplus_btree_search (struct grub_hfsplus_btree *btree,
 			 + 2);
 
 	      if ((char *) pointer > node + btree->nodesize - 2)
-		return grub_error (GRUB_ERR_BAD_FS, "HFS+ key beyond end of node");
+	        {
+	          grub_free (node);
+	          return grub_error (GRUB_ERR_BAD_FS, "HFS+ key beyond end of node");
+	        }
 
 	      currnode = grub_be_to_cpu32 (grub_get_unaligned32 (pointer));
 	      match = 1;
-- 
2.27.0

[PATCH 7/9] fs/iso9660: Fix memory leak in grub_iso9660_susp_iterate

[t.feng]

Fix memory leak in grub_iso9660_susp_iterate.

Fixes: 99373ce47(iso9660.c: Remove nested functions.)

Signed-off-by: "t.feng" <fengtao40@huawei.com>
---
 grub-core/fs/iso9660.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/grub-core/fs/iso9660.c b/grub-core/fs/iso9660.c
index 91817ec1f..df9f7783b 100644
--- a/grub-core/fs/iso9660.c
+++ b/grub-core/fs/iso9660.c
@@ -279,7 +279,10 @@ grub_iso9660_susp_iterate (grub_fshelp_node_t node, grub_off_t off,
   /* Load a part of the System Usage Area.  */
   err = read_node (node, off, sua_size, sua);
   if (err)
-    return err;
+    {
+      grub_free (sua);
+      return err;
+    }
 
   for (entry = (struct grub_iso9660_susp_entry *) sua; (char *) entry < (char *) sua + sua_size - 1 && entry->len > 0;
        entry = (struct grub_iso9660_susp_entry *)
@@ -309,7 +312,10 @@ grub_iso9660_susp_iterate (grub_fshelp_node_t node, grub_off_t off,
 	  err = grub_disk_read (node->data->disk, ce_block, off,
 				sua_size, sua);
 	  if (err)
-	    return err;
+	    {
+	      grub_free (sua);
+	      return err;
+	    }
 
 	  entry = (struct grub_iso9660_susp_entry *) sua;
 	}
-- 
2.27.0

Re: [PATCH 7/9] fs/iso9660: Fix memory leak in grub_iso9660_susp_iterate

[Thomas Schmitt]

Hi,

on Sat, 19 Nov 2022 18:39:44 +0800, t.feng via Grub-devel wrote:
> Fix memory leak in grub_iso9660_susp_iterate.
>
> Fixes: 99373ce47(iso9660.c: Remove nested functions.)
>
> Signed-off-by: "t.feng" <fengtao40@huawei.com>

Reviewed-by: Thomas Schmitt <scdbackup@gmx.net>


Have a nice day :)

Thomas

[PATCH 8/9] fs/squash4: Fix memeory leak in grub_squash_iterate_dir

[t.feng]

Fix memory leaks in grub_squash_iterate_dir.

Fixes: 20dd511c8(Handle "." and ".." on squashfs.)

Signed-off-by: "t.feng" <fengtao40@huawei.com>
---
 grub-core/fs/squash4.c | 20 ++++++++++++++++----
 1 file changed, 16 insertions(+), 4 deletions(-)

diff --git a/grub-core/fs/squash4.c b/grub-core/fs/squash4.c
index 02b1f9b6d..a30e6ebe1 100644
--- a/grub-core/fs/squash4.c
+++ b/grub-core/fs/squash4.c
@@ -550,7 +550,10 @@ grub_squash_iterate_dir (grub_fshelp_node_t dir,
 			  + node->stack[node->stsize - 1].ino_chunk,
 			  node->stack[node->stsize - 1].ino_offset);
 	if (err)
-	  return 0;
+	  {
+	    grub_free (node);
+	    return 0;
+	  }
 
 	if (hook ("..", GRUB_FSHELP_DIR, node, hook_data))
 	  return 1;
@@ -600,7 +603,10 @@ grub_squash_iterate_dir (grub_fshelp_node_t dir,
 			    grub_le_to_cpu64 (dir->data->sb.diroffset)
 			    + chunk, off);
 	  if (err)
-	    return 0;
+	    {
+	      grub_free (buf);
+	      return 0;
+	    }
 
 	  off += grub_le_to_cpu16 (di.namelen) + 1;
 	  buf[grub_le_to_cpu16 (di.namelen) + 1] = 0;
@@ -612,11 +618,17 @@ grub_squash_iterate_dir (grub_fshelp_node_t dir,
 	  if (grub_add (dir->stsize, 1, &sz) ||
 	      grub_mul (sz, sizeof (dir->stack[0]), &sz) ||
 	      grub_add (sz, sizeof (*node), &sz))
-	    return 0;
+	    {
+	      grub_free (buf);
+	      return 0;
+	    }
 
 	  node = grub_malloc (sz);
 	  if (! node)
-	    return 0;
+	    {
+	      grub_free (buf);
+	      return 0;
+	    }
 
 	  grub_memcpy (node, dir, sz - sizeof(dir->stack[0]));
 
-- 
2.27.0

[PATCH 9/9] fs/xfs: Fix memory leaks in xfs

[t.feng]

Fix memory leaks in xfs.

Signed-off-by: "t.feng" <fengtao40@huawei.com>
---
 grub-core/fs/xfs.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/grub-core/fs/xfs.c b/grub-core/fs/xfs.c
index d6de7f1a2..b67407690 100644
--- a/grub-core/fs/xfs.c
+++ b/grub-core/fs/xfs.c
@@ -585,7 +585,10 @@ grub_xfs_read_block (grub_fshelp_node_t node, grub_disk_addr_t fileblock)
           if (grub_disk_read (node->data->disk,
                               GRUB_XFS_FSB_TO_BLOCK (node->data, get_fsb (keys, i - 1 + recoffset)) << (node->data->sblock.log2_bsize - GRUB_DISK_SECTOR_BITS),
                               0, node->data->bsize, leaf))
-            return 0;
+            {
+              grub_free (leaf);
+              return 0;
+            }
 
 	  if ((!node->data->hascrc &&
 	       grub_strncmp ((char *) leaf->magic, "BMAP", 4)) ||
@@ -751,6 +754,7 @@ static int iterate_dir_call_hook (grub_uint64_t ino, const char *filename,
   if (err)
     {
       grub_print_error ();
+      grub_free (fdiro);
       return 0;
     }
 
@@ -861,7 +865,10 @@ grub_xfs_iterate_dir (grub_fshelp_node_t dir,
 					  blk << dirblk_log2,
 					  dirblk_size, dirblock, 0);
 	    if (numread != dirblk_size)
-	      return 0;
+	      {
+	        grub_free (dirblock);
+	        return 0;
+	      }
 
 	    entries = (grub_be_to_cpu32 (tail->leaf_count)
 		       - grub_be_to_cpu32 (tail->leaf_stale));
-- 
2.27.0

Re: [PATCH 0/9] fix memory leaks in fs module

[Daniel Kiper]

On Sat, Nov 19, 2022 at 06:39:37PM +0800, t.feng via Grub-devel wrote:
> Hi all,
> I am doing code review in grub-core/fs module and I found some
> memory leaks.
>
> *************************
>
> t.feng (9):
>   fs/affs:Fix memory leaks in grub_affs_create_node
>   fs/btrfs: Fix memory leak in find_path
>   fs/minix: Fix memory leak in grub_minix_lookup_symlink
>   fs/ntfs: Fix memory leak in  grub_ntfs_read_symlink
>   fs/bfs: Fix memory leak in read_bfs_file
>   fs/hfsplus: Fix memory leak in grub_hfsplus_btree_search
>   fs/iso9660: Fix memory leak in grub_iso9660_susp_iterate
>   fs/squash4: Fix memeory leak in grub_squash_iterate_dir
>   fs/xfs: Fix memory leaks in xfs

You can add Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
to all patches for which I did not send a reply.

Daniel