[PATCH 0/6] rebased patch set from Wu Guanghao

[PATCH 0/6] rebased patch set from Wu Guanghao

[Coly Li]

Hi Jes,

This is the rebased patch set from Wu Guanghao, all the patches can
be applied on top of commit 0a07dea8d3b78 ("Mdmonitor: Refactor
check_one_sharer() for better error handling") from the mdadm tree.

The patch from me is to solve conflict for the first patch of Guanghao.
In order to make the commit logs to be more understandable, I recompose
some patch subjects or commit logs.

Thank you in advance for taking them.

Coly Li
---

Coly Li (1):
  util.c: code cleanup for parse_layout_faulty()

Wu Guanghao (5):
  util.c: fix memleak in parse_layout_faulty()
  Detail.c: fix memleak in Detail()
  isuper-intel.c: fix double free in load_imsm_mpb()
  super-intel.c: fix memleak in find_disk_attached_hba()
  super-ddf.c: fix memleak in get_vd_num_of_subarray()

 Detail.c      |  1 +
 super-ddf.c   |  9 +++++++--
 super-intel.c |  5 +++--
 util.c        | 11 ++++++++---
 4 files changed, 19 insertions(+), 7 deletions(-)

-- 
2.39.2

[PATCH 1/6] util.c: code cleanup for parse_layout_faulty()

[Coly Li]

Resort the code lines in parse_layout_faulty() to make it more
comfortable, no logic change.

Signed-off-by: Coly Li <colyli@suse.de>
---
 util.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/util.c b/util.c
index 7fc881bf..b0b7aec4 100644
--- a/util.c
+++ b/util.c
@@ -421,12 +421,15 @@ int parse_layout_10(char *layout)
 
 int parse_layout_faulty(char *layout)
 {
+	int ln, mode;
+	char *m;
+
 	if (!layout)
 		return -1;
+
 	/* Parse the layout string for 'faulty' */
-	int ln = strcspn(layout, "0123456789");
-	char *m = xstrdup(layout);
-	int mode;
+	ln = strcspn(layout, "0123456789");
+	m = xstrdup(layout);
 	m[ln] = 0;
 	mode = map_name(faultylayout, m);
 	if (mode == UnSet)
-- 
2.39.2

Re: [PATCH 1/6] util.c: code cleanup for parse_layout_faulty()

[Paul Menzel]

Dear Coly,


Am 03.03.23 um 09:33 schrieb Coly Li:
> Resort the code lines in parse_layout_faulty() to make it more
> comfortable, no logic change.

Maybe use a more specific subject line and make it a statement by using 
a verb (imperative mood):

util.c: Reorder code lines in `parse_layout_faulty()`

> Signed-off-by: Coly Li <colyli@suse.de>
> ---
>   util.c | 9 ++++++---
>   1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/util.c b/util.c
> index 7fc881bf..b0b7aec4 100644
> --- a/util.c
> +++ b/util.c
> @@ -421,12 +421,15 @@ int parse_layout_10(char *layout)
>   
>   int parse_layout_faulty(char *layout)
>   {
> +	int ln, mode;
> +	char *m;
> +
>   	if (!layout)
>   		return -1;
> +
>   	/* Parse the layout string for 'faulty' */
> -	int ln = strcspn(layout, "0123456789");
> -	char *m = xstrdup(layout);
> -	int mode;
> +	ln = strcspn(layout, "0123456789");
> +	m = xstrdup(layout);
>   	m[ln] = 0;
>   	mode = map_name(faultylayout, m);
>   	if (mode == UnSet)

Reviewed-by: Paul Menzel <pmenzel@molgen.mpg.de>


Kind regards,

Paul

[PATCH 2/6] util.c: fix memleak in parse_layout_faulty()

[Coly Li]

From: Wu Guanghao <wuguanghao3@huawei.com>

char *m is allocated by xstrdup but not free() before return, will cause
a memory leak

Signed-off-by: Wu Guanghao <wuguanghao3@huawei.com>
Acked-by: Mariusz Tkaczyk <mariusz.tkaczyk@linux.intel.com>
Acked-by: Coly Li <colyli@suse.de>
---
 util.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/util.c b/util.c
index b0b7aec4..9f1e1f7c 100644
--- a/util.c
+++ b/util.c
@@ -432,6 +432,8 @@ int parse_layout_faulty(char *layout)
 	m = xstrdup(layout);
 	m[ln] = 0;
 	mode = map_name(faultylayout, m);
+	free(m);
+
 	if (mode == UnSet)
 		return -1;
 
-- 
2.39.2

[PATCH 3/6] Detail.c: fix memleak in Detail()

[Coly Li]

From: Wu Guanghao <wuguanghao3@huawei.com>

char *sysdev = xstrdup() but not free() in for loop, will cause memory
leak

Signed-off-by: Wu Guanghao <wuguanghao3@huawei.com>
Acked-by: Mariusz Tkaczyk <mariusz.tkaczyk@linux.intel.com>
Acked-by: Coly Li <colyli@suse.de>
---
 Detail.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Detail.c b/Detail.c
index ce7a8445..4ef26460 100644
--- a/Detail.c
+++ b/Detail.c
@@ -303,6 +303,7 @@ int Detail(char *dev, struct context *c)
 				if (path)
 					printf("MD_DEVICE_%s_DEV=%s\n",
 					       sysdev, path);
+				free(sysdev);
 			}
 		}
 		goto out;
-- 
2.39.2

[PATCH 4/6] isuper-intel.c: fix double free in load_imsm_mpb()

[Coly Li]

From: Wu Guanghao <wuguanghao3@huawei.com>

In load_imsm_mpb() there is potential double free issue on super->buf.

The first location to free super->buf is from get_super_block() <==
load_and_parse_mpb() <== load_imsm_mpb():
 4514         if (posix_memalign(&super->migr_rec_buf, MAX_SECTOR_SIZE,
 4515             MIGR_REC_BUF_SECTORS*MAX_SECTOR_SIZE) != 0) {
 4516                 pr_err("could not allocate migr_rec buffer\n");
 4517                 free(super->buf);
 4518                 return 2;
 4519         }

If the above error condition happens, super->buf is freed and value 2
is returned to get_super_block() eventually. Then in the following code
block inside load_imsm_mpb(),
 5289  error:
 5290         if (!err) {
 5291                 s->next = *super_list;
 5292                 *super_list = s;
 5293         } else {
 5294                 if (s)
 5295                         free_imsm(s);
 5296                 close_fd(&dfd);
 5297         }
at line 5295 when free_imsm() is called, super->buf is freed again from
the call chain free_imsm() <== __free_imsm(), in following code block,
 4651         if (super->buf) {
 4652                 free(super->buf);
 4653                 super->buf = NULL;
 4654         }

This patch sets super->buf as NULL after line 4517 in load_imsm_mpb()
to avoid the potential double free().

(Coly Li helps to re-compose the commit log)

Signed-off-by: Wu Guanghao <wuguanghao3@huawei.com>
Reviewed-by: Mariusz Tkaczyk <mariusz.tkaczyk@linux.intel.com>
Acked-by: Coly Li <colyli@suse.de>
---
 super-intel.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/super-intel.c b/super-intel.c
index 89fac626..4a3da847 100644
--- a/super-intel.c
+++ b/super-intel.c
@@ -4515,6 +4515,7 @@ static int load_imsm_mpb(int fd, struct intel_super *super, char *devname)
 	    MIGR_REC_BUF_SECTORS*MAX_SECTOR_SIZE) != 0) {
 		pr_err("could not allocate migr_rec buffer\n");
 		free(super->buf);
+		super->buf = NULL;
 		return 2;
 	}
 	super->clean_migration_record_by_mdmon = 0;
-- 
2.39.2

[PATCH 5/6] super-intel.c: fix memleak in find_disk_attached_hba()

[Coly Li]

From: Wu Guanghao <wuguanghao3@huawei.com>

If disk_path = diskfd_to_devpath(), we need free(disk_path) before
return, otherwise there will be a memory leak

Signed-off-by: Wu Guanghao <wuguanghao3@huawei.com>
Reviewed-by: Mariusz Tkaczyk <mariusz.tkaczyk@linux.intel.com>
Acked-by: Coly Li <colyli@suse.de>
---
 super-intel.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/super-intel.c b/super-intel.c
index 4a3da847..e155a8ae 100644
--- a/super-intel.c
+++ b/super-intel.c
@@ -713,12 +713,12 @@ static struct sys_dev* find_disk_attached_hba(int fd, const char *devname)
 
 	for (elem = list; elem; elem = elem->next)
 		if (path_attached_to_hba(disk_path, elem->path))
-			return elem;
+			break;
 
 	if (disk_path != devname)
 		free(disk_path);
 
-	return NULL;
+	return elem;
 }
 
 static int find_intel_hba_capability(int fd, struct intel_super *super,
-- 
2.39.2

[PATCH 6/6] super-ddf.c: fix memleak in get_vd_num_of_subarray()

[Coly Li]

From: Wu Guanghao <wuguanghao3@huawei.com>

sra = sysfs_read() should be free before return in
get_vd_num_of_subarray()

Signed-off-by: Wu Guanghao <wuguanghao3@huawei.com>
Acked-by: Mariusz Tkaczyk <mariusz.tkaczyk@linux.intel.com>
Acked-by: Coly Li <colyli@suse.de>
---
 super-ddf.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/super-ddf.c b/super-ddf.c
index 309812df..b86c6acd 100644
--- a/super-ddf.c
+++ b/super-ddf.c
@@ -1592,15 +1592,20 @@ static unsigned int get_vd_num_of_subarray(struct supertype *st)
 	sra = sysfs_read(-1, st->devnm, GET_VERSION);
 	if (!sra || sra->array.major_version != -1 ||
 	    sra->array.minor_version != -2 ||
-	    !is_subarray(sra->text_version))
+	    !is_subarray(sra->text_version)) {
+		if (sra)
+			sysfs_free(sra);
 		return DDF_NOTFOUND;
+	}
 
 	sub = strchr(sra->text_version + 1, '/');
 	if (sub != NULL)
 		vcnum = strtoul(sub + 1, &end, 10);
 	if (sub == NULL || *sub == '\0' || *end != '\0' ||
-	    vcnum >= be16_to_cpu(ddf->active->max_vd_entries))
+	    vcnum >= be16_to_cpu(ddf->active->max_vd_entries)) {
+		sysfs_free(sra);
 		return DDF_NOTFOUND;
+	}
 
 	return vcnum;
 }
-- 
2.39.2