[PATCH 0/4] Fix memory leak for Manage Assemble Kill mdadm

[PATCH 0/4] Fix memory leak for Manage Assemble Kill mdadm

[miaoguanqin]

When we test mdadm with asan,we found some memory leaks.
We fix these memory leaks based on code logic. 

miaoguanqin (4):
  Fix memory leak in file Assemble
  Fix memory leak in file Kill
  Fix memory leak in file Manage
  Fix memory leak in file mdadm

 Assemble.c | 15 +++++++++++++--
 Kill.c     |  9 ++++++++-
 Manage.c   | 16 +++++++++++++++-
 mdadm.c    |  4 ++++
 4 files changed, 40 insertions(+), 4 deletions(-)

-- 
2.33.0

[PATCH 1/4] Fix memory leak in file Assemble

[miaoguanqin]

When we test mdadm with asan,we found some memory leaks in Assemble.c
We fix these memory leaks based on code logic.

Signed-off-by: miaoguanqin <miaoguanqin@huawei.com>
Signed-off-by: lixiaokeng <lixiaokeng@huawei.com>
---
 Assemble.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/Assemble.c b/Assemble.c
index 49804941..574865eb 100644
--- a/Assemble.c
+++ b/Assemble.c
@@ -341,8 +341,10 @@ static int select_devices(struct mddev_dev *devlist,
 				st->ss->free_super(st);
 			dev_policy_free(pol);
 			domain_free(domains);
-			if (tst)
+			if (tst) {
 				tst->ss->free_super(tst);
+				free(tst);
+			}
 			return -1;
 		}
 
@@ -417,6 +419,7 @@ static int select_devices(struct mddev_dev *devlist,
 				st->ss->free_super(st);
 				dev_policy_free(pol);
 				domain_free(domains);
+				free(st);
 				return -1;
 			}
 			if (c->verbose > 0)
@@ -425,6 +428,8 @@ static int select_devices(struct mddev_dev *devlist,
 
 			/* make sure we finished the loop */
 			tmpdev = NULL;
+			if (st)
+				free(st);
 			goto loop;
 		} else {
 			content = *contentp;
@@ -533,6 +538,7 @@ static int select_devices(struct mddev_dev *devlist,
 				st->ss->free_super(st);
 				dev_policy_free(pol);
 				domain_free(domains);
+				free(tst);
 				return -1;
 			}
 			tmpdev->used = 1;
@@ -546,8 +552,10 @@ static int select_devices(struct mddev_dev *devlist,
 		}
 		dev_policy_free(pol);
 		pol = NULL;
-		if (tst)
+		if (tst) {
 			tst->ss->free_super(tst);
+			free(tst);
+		}
 	}
 
 	/* Check if we found some imsm spares but no members */
@@ -839,6 +847,7 @@ static int load_devices(struct devs *devices, char *devmap,
 				close(mdfd);
 				free(devices);
 				free(devmap);
+				free(best);
 				*stp = st;
 				return -1;
 			}
@@ -1950,6 +1959,8 @@ out:
 	} else if (mdfd >= 0)
 		close(mdfd);
 
+	if (best)
+		free(best);
 	/* '2' means 'OK, but not started yet' */
 	if (rv == -1) {
 		free(devices);
-- 
2.33.0

Re: [PATCH 1/4] Fix memory leak in file Assemble

[Mariusz Tkaczyk]

On Thu, 23 Mar 2023 09:30:50 +0800
miaoguanqin <miaoguanqin@huawei.com> wrote:

> When we test mdadm with asan,we found some memory leaks in Assemble.c
> We fix these memory leaks based on code logic.
> 
> Signed-off-by: miaoguanqin <miaoguanqin@huawei.com>
> Signed-off-by: lixiaokeng <lixiaokeng@huawei.com>
> ---

Acked-by: Mariusz Tkaczyk <mariusz.tkaczyk@linux.intel.com>

Re: [PATCH 1/4] Fix memory leak in file Assemble

[Paul Menzel]

Dear Miao,


Thank you for your patches.

Am 23.03.23 um 02:30 schrieb miaoguanqin:
> When we test mdadm with asan,we found some memory leaks in Assemble.c
> We fix these memory leaks based on code logic.
> 
> Signed-off-by: miaoguanqin <miaoguanqin@huawei.com>
> Signed-off-by: lixiaokeng <lixiaokeng@huawei.com>

It’d be great if you used the full names (also in your From: field) 
instead of usernames. For example, Li Xiao Keng.

     git config --global user.name "…"
     git commit --amend --author="… <miaoguanqin@huawei.com>"


Kind regards,

Paul

[PATCH 2/4] Fix memory leak in file Kill

[miaoguanqin]

When we test mdadm with asan,we found some memory leaks in Kill.c
We fix these memory leaks based on code logic.

Signed-off-by: miaoguanqin <miaoguanqin@huawei.com>
Signed-off-by: lixiaokeng <lixiaokeng@huawei.com>
---
 Kill.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/Kill.c b/Kill.c
index bfd0efdc..46a1a8a0 100644
--- a/Kill.c
+++ b/Kill.c
@@ -41,6 +41,7 @@ int Kill(char *dev, struct supertype *st, int force, int verbose, int noexcl)
 	 *  4 - failed to find a superblock.
 	 */
 
+	int flags = 0;
 	int fd, rv = 0;
 
 	if (force)
@@ -52,8 +53,10 @@ int Kill(char *dev, struct supertype *st, int force, int verbose, int noexcl)
 				dev);
 		return 2;
 	}
-	if (st == NULL)
+	if (st == NULL) {
 		st = guess_super(fd);
+		flags = 1;
+	}
 	if (st == NULL || st->ss->init_super == NULL) {
 		if (verbose >= 0)
 			pr_err("Unrecognised md component device - %s\n", dev);
@@ -77,6 +80,10 @@ int Kill(char *dev, struct supertype *st, int force, int verbose, int noexcl)
 			rv = 0;
 		}
 	}
+	if (flags == 1 && st) {
+		st->ss->free_super(st);
+		free(st);
+	}
 	close(fd);
 	return rv;
 }
-- 
2.33.0

Re: [PATCH 2/4] Fix memory leak in file Kill

[Mariusz Tkaczyk]

Hi,
see comments below.

On Thu, 23 Mar 2023 09:30:51 +0800
miaoguanqin <miaoguanqin@huawei.com> wrote:

> When we test mdadm with asan,we found some memory leaks in Kill.c
missing space after ","

> We fix these memory leaks based on code logic.
> 
> Signed-off-by: miaoguanqin <miaoguanqin@huawei.com>
> Signed-off-by: lixiaokeng <lixiaokeng@huawei.com>
> ---
>  Kill.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/Kill.c b/Kill.c
> index bfd0efdc..46a1a8a0 100644
> --- a/Kill.c
> +++ b/Kill.c
> @@ -41,6 +41,7 @@ int Kill(char *dev, struct supertype *st, int force, int
> verbose, int noexcl)
>  	 *  4 - failed to find a superblock.
>  	 */
>  
> +	int flags = 0;

could you name it "free_super" or something like that? could you use bool?

Thanks,
Mariusz

[PATCH 3/4] Fix memory leak in file Manage

[miaoguanqin]

When we test mdadm with asan,we found some memory leaks in Manage.c
We fix these memory leaks based on code logic.

Signed-off-by: miaoguanqin <miaoguanqin@huawei.com>
Signed-off-by: lixiaokeng <lixiaokeng@huawei.com>
---
 Manage.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/Manage.c b/Manage.c
index fde6aba3..75bed83b 100644
--- a/Manage.c
+++ b/Manage.c
@@ -222,6 +222,8 @@ int Manage_stop(char *devname, int fd, int verbose, int will_retry)
 		if (verbose >= 0)
 			pr_err("Cannot get exclusive access to %s:Perhaps a running process, mounted filesystem or active volume group?\n",
 			       devname);
+		if (mdi)
+			sysfs_free(mdi);
 		return 1;
 	}
 	/* If this is an mdmon managed array, just write 'inactive'
@@ -818,8 +820,16 @@ int Manage_add(int fd, int tfd, struct mddev_dev *dv,
 						    rdev, update, devname,
 						    verbose, array);
 				dev_st->ss->free_super(dev_st);
-				if (rv)
+				if (rv) {
+					if (dev_st)
+						free(dev_st);
 					return rv;
+				}
+			}
+			if (dev_st) {
+				if (dev_st->sb)
+					dev_st->ss->free_super(dev_st);
+				free(dev_st);
 			}
 		}
 		if (dv->disposition == 'M') {
@@ -1716,6 +1726,8 @@ int Manage_subdevs(char *devname, int fd,
 			break;
 		}
 	}
+	if (tst)
+		free(tst);
 	if (frozen > 0)
 		sysfs_set_str(&info, NULL, "sync_action","idle");
 	if (test && count == 0)
@@ -1723,6 +1735,8 @@ int Manage_subdevs(char *devname, int fd,
 	return 0;
 
 abort:
+	if (tst)
+		free(tst);
 	if (frozen > 0)
 		sysfs_set_str(&info, NULL, "sync_action","idle");
 	return !test && busy ? 2 : 1;
-- 
2.33.0

Re: [PATCH 3/4] Fix memory leak in file Manage

[Mariusz Tkaczyk]

On Thu, 23 Mar 2023 09:30:52 +0800
miaoguanqin <miaoguanqin@huawei.com> wrote:

> When we test mdadm with asan,we found some memory leaks in Manage.c
> We fix these memory leaks based on code logic.

Missing space after comma.
> 
> Signed-off-by: miaoguanqin <miaoguanqin@huawei.com>
> Signed-off-by: lixiaokeng <lixiaokeng@huawei.com>
> ---
>  Manage.c | 16 +++++++++++++++-
>  1 file changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/Manage.c b/Manage.c
> index fde6aba3..75bed83b 100644
> --- a/Manage.c
> +++ b/Manage.c
> @@ -222,6 +222,8 @@ int Manage_stop(char *devname, int fd, int verbose, int
> will_retry) if (verbose >= 0)
>  			pr_err("Cannot get exclusive access to %s:Perhaps a
> running process, mounted filesystem or active volume group?\n", devname);
> +		if (mdi)
> +			sysfs_free(mdi);
>  		return 1;
>  	}
>  	/* If this is an mdmon managed array, just write 'inactive'
> @@ -818,8 +820,16 @@ int Manage_add(int fd, int tfd, struct mddev_dev *dv,
>  						    rdev, update, devname,
>  						    verbose, array);
>  				dev_st->ss->free_super(dev_st);
> -				if (rv)
> +				if (rv) {
> +					if (dev_st)
> +						free(dev_st);

We are calling dev_st->ss->free_super(dev_st) in this code branch so it seems
that dev_st cannot be null, just free is needed.

[PATCH 4/4] Fix memory leak in file mdadm

[miaoguanqin]

When we test mdadm with asan,we found some memory leaks in mdadm.c
We fix these memory leaks based on code logic.

Signed-off-by: miaoguanqin <miaoguanqin@huawei.com>
Signed-off-by: lixiaokeng <lixiaokeng@huawei.com>
---
 mdadm.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/mdadm.c b/mdadm.c
index 4685ad6b..19d7916b 100644
--- a/mdadm.c
+++ b/mdadm.c
@@ -1709,6 +1709,10 @@ int main(int argc, char *argv[])
 		autodetect();
 		break;
 	}
+	if (ss) {
+		ss->ss->free_super(ss);
+		free(ss);
+	}
 	if (locked)
 		cluster_release_dlmlock();
 	close_fd(&mdfd);
-- 
2.33.0

Re: [PATCH 4/4] Fix memory leak in file mdadm

[Mariusz Tkaczyk]

On Thu, 23 Mar 2023 09:30:53 +0800
miaoguanqin <miaoguanqin@huawei.com> wrote:

> When we test mdadm with asan,we found some memory leaks in mdadm.c
> We fix these memory leaks based on code logic.
> 
space after comma.
> Signed-off-by: miaoguanqin <miaoguanqin@huawei.com>
> Signed-off-by: lixiaokeng <lixiaokeng@huawei.com>

Acked-by: Mariusz Tkaczyk <mariusz.tkaczyk@linux.intel.com>

[PATCH 0/4] Fix memory leak for Manage Assemble Kill mdadm

[Guanqin Miao]

When we test mdadm with asan,i we found some memory leaks.
We fix these memory leaks based on code logic.

Guanqin Miao (4):
  Fix memory leak in file Assemble
  Fix memory leak in file Kill
  Fix memory leak in file Manage
  Fix memory leak in file mdadm

 Assemble.c | 15 +++++++++++++--
 Kill.c     |  9 ++++++++-
 Manage.c   | 15 ++++++++++++++-
 mdadm.c    |  4 ++++
 4 files changed, 39 insertions(+), 4 deletions(-)

-- 
2.33.0

[PATCH 0/4] Fix memory leak for Manage Assemble Kill mdadm

[Guanqin Miao]

When we test mdadm with asan,i we found some memory leaks.
We fix these memory leaks based on code logic.

Guanqin Miao (4):
  Fix memory leak in file Assemble
  Fix memory leak in file Kill
  Fix memory leak in file Manage
  Fix memory leak in file mdadm

 Assemble.c | 13 +++++++++++--
 Kill.c     |  9 ++++++++-
 Manage.c   | 11 ++++++++++-
 mdadm.c    |  4 ++++
 4 files changed, 33 insertions(+), 4 deletions(-)

-- 
2.33.0

Re: [PATCH 0/4] Fix memory leak for Manage Assemble Kill mdadm

[Jes Sorensen]

On 4/24/23 04:06, Guanqin Miao wrote:
> When we test mdadm with asan,i we found some memory leaks.
> We fix these memory leaks based on code logic.
> 
> Guanqin Miao (4):
>   Fix memory leak in file Assemble
>   Fix memory leak in file Kill
>   Fix memory leak in file Manage
>   Fix memory leak in file mdadm
> 
>  Assemble.c | 13 +++++++++++--
>  Kill.c     |  9 ++++++++-
>  Manage.c   | 11 ++++++++++-
>  mdadm.c    |  4 ++++
>  4 files changed, 33 insertions(+), 4 deletions(-)
> 

I applied this, modulo a fix to the Manage fix, where it would try to
call free(tst) in the abort path, before tst was initialized.

Thanks,
Jes

Re: [PATCH 0/4] Fix memory leak for Manage Assemble Kill mdadm

[Jes Sorensen]

On 9/1/23 12:02, Jes Sorensen wrote:
> On 4/24/23 04:06, Guanqin Miao wrote:
>> When we test mdadm with asan,i we found some memory leaks.
>> We fix these memory leaks based on code logic.
>>
>> Guanqin Miao (4):
>>   Fix memory leak in file Assemble
>>   Fix memory leak in file Kill
>>   Fix memory leak in file Manage
>>   Fix memory leak in file mdadm
>>
>>  Assemble.c | 13 +++++++++++--
>>  Kill.c     |  9 ++++++++-
>>  Manage.c   | 11 ++++++++++-
>>  mdadm.c    |  4 ++++
>>  4 files changed, 33 insertions(+), 4 deletions(-)
>>
> 
> I applied this, modulo a fix to the Manage fix, where it would try to
> call free(tst) in the abort path, before tst was initialized.

Oh and a fix to Assemble where free(st) would goto loop and then
dereference st.

Jes