[PATCH] Fix memory leak for function Manage_subdevs Manage_add Kill V2

[PATCH] Fix memory leak for function Manage_subdevs Manage_add Kill V2

[miaoguanqin]

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

Signed-off-by: miaoguanqin <miaoguanqin@huawei.com>
---
  Assemble.c | 16 +++++++++++++---
  Kill.c     | 10 +++++++++-
  Manage.c   | 16 +++++++++++++++-
  mdadm.c    |  6 ++++++
  4 files changed, 43 insertions(+), 5 deletions(-)

diff --git a/Assemble.c b/Assemble.c
index 3ef4b29..c23567f 100644
--- a/Assemble.c
+++ b/Assemble.c
@@ -281,8 +281,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;
  		}

@@ -357,6 +359,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)
@@ -365,6 +368,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;
@@ -473,6 +478,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;
@@ -486,8 +492,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 */
@@ -778,6 +786,7 @@ static int load_devices(struct devs *devices, char 
*devmap,
  				close(mdfd);
  				free(devices);
  				free(devmap);
+				free(best);
  				*stp = st;
  				return -1;
  			}
@@ -1882,7 +1891,8 @@ out:
  		}
  	} else if (mdfd >= 0)
  		close(mdfd);
-
+	if (best)
+		free(best);
  	/* '2' means 'OK, but not started yet' */
  	if (rv == -1) {
  		free(devices);
diff --git a/Kill.c b/Kill.c
index d4767e2..073288e 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,11 @@ int Kill(char *dev, struct supertype *st, int force, 
int verbose, int noexcl)
  			rv = 0;
  		}
  	}
+	if (flags == 1 && st) {
+		if (st->sb)
+			free(st->sb);
+		free(st);
+	}
  	close(fd);
  	return rv;
  }
diff --git a/Manage.c b/Manage.c
index ffe55f8..60c6d12 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'
@@ -819,8 +821,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') {
@@ -1649,6 +1659,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)
@@ -1656,6 +1668,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;
diff --git a/mdadm.c b/mdadm.c
index da66c76..981fa98 100644
--- a/mdadm.c
+++ b/mdadm.c
@@ -1765,6 +1765,12 @@ int main(int argc, char *argv[])
  		autodetect();
  		break;
  	}
+	if (ss) {
+		if (ss->sb)
+			free(ss->sb);
+		free(ss);
+	
+	}
  	if (locked)
  		cluster_release_dlmlock();
  	if (mdfd > 0)
-- 
2.33.0

Re: [PATCH] Fix memory leak for function Manage_subdevs Manage_add Kill V2

[Mariusz Tkaczyk]

On Wed, 22 Feb 2023 16:30:53 +0800
miaoguanqin <miaoguanqin@huawei.com> wrote:

> When we test mdadm with asan,we found some memory leaks.
> We fix these memory leaks based on code logic.
> 
> Signed-off-by: miaoguanqin <miaoguanqin@huawei.com>
> ---
>   Assemble.c | 16 +++++++++++++---
>   Kill.c     | 10 +++++++++-
>   Manage.c   | 16 +++++++++++++++-
>   mdadm.c    |  6 ++++++
>   4 files changed, 43 insertions(+), 5 deletions(-)
> 
Acked-by: Mariusz Tkaczyk <mariusz.tkaczyk@linux.intel.com>

Coly, could you please take a look?

Thanks,
Mariusz

Re: [PATCH] Fix memory leak for function Manage_subdevs Manage_add Kill V2

[Coly Li]

On Wed, Feb 22, 2023 at 04:30:53PM +0800, miaoguanqin wrote:
> When we test mdadm with asan,we found some memory leaks.
> We fix these memory leaks based on code logic.
> 
> Signed-off-by: miaoguanqin <miaoguanqin@huawei.com>
> ---
>  Assemble.c | 16 +++++++++++++---
>  Kill.c     | 10 +++++++++-
>  Manage.c   | 16 +++++++++++++++-
>  mdadm.c    |  6 ++++++
>  4 files changed, 43 insertions(+), 5 deletions(-)
>

[snipped]
 
> diff --git a/Kill.c b/Kill.c
> index d4767e2..073288e 100644
> --- a/Kill.c
> +++ b/Kill.c

[snipped]

> @@ -77,6 +80,11 @@ int Kill(char *dev, struct supertype *st, int force, int
> verbose, int noexcl)
>  			rv = 0;
>  		}
>  	}
> +	if (flags == 1 && st) {
> +		if (st->sb)
> +			free(st->sb);

May I ask why not call st->ss->free_super(st) ?


> +		free(st);
> +	}
>  	close(fd);
>  	return rv;
>  }

[snipped]

> diff --git a/mdadm.c b/mdadm.c
> index da66c76..981fa98 100644
> --- a/mdadm.c
> +++ b/mdadm.c
> @@ -1765,6 +1765,12 @@ int main(int argc, char *argv[])
>  		autodetect();
>  		break;
>  	}
> +	if (ss) {
> +		if (ss->sb)
> +			free(ss->sb);

Same question, why not call ss->ss->free_super(ss) ?

> +		free(ss);
> +	
> +	}
>  	if (locked)
>  		cluster_release_dlmlock();
>  	if (mdfd > 0)


Overall the patch is fine to me. But it might be better to split it into
multiple patches that each has the changes for a single file.

Thanks.

-- 
Coly Li

Re: [PATCH] Fix memory leak for function Manage_subdevs Manage_add Kill V2

[Jes Sorensen]

On 2/22/23 03:30, miaoguanqin wrote:
> When we test mdadm with asan,we found some memory leaks.
> We fix these memory leaks based on code logic.
> 
> Signed-off-by: miaoguanqin <miaoguanqin@huawei.com>

Hi,

Your mail client sent the patch with format=flowed which is bad and
prevents it from being applied. Can you please resend.

Thanks,
Jes