[PATCH 0/5 v2] mdadm: fix memory leak and double free

[PATCH 0/5 v2] mdadm: fix memory leak and double free

[Wu Guanghao]

Through tool scanning and code review, we found several memory leaks and double free.

v2:
- add empty lines to separate declarations and not related code
sections. [1/5]
- free super->buf and set NULL in load_imsm_mpb [3/5]
- optimize code to avoid code duplication. [4/5]

Wu Guanghao (5):
  parse_layout_faulty: fix memleak
  Detail: fix memleak
  load_imsm_mpb: fix double free
  find_disk_attached_hba: fix memleak
  get_vd_num_of_subarray: fix memleak

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

--
2.27.0

[PATCH 1/5 v2] parse_layout_faulty: fix memleak

[Wu Guanghao]

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

Signed-off-by: Wu Guanghao <wuguanghao3@huawei.com>
---
 util.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/util.c b/util.c
index cc94f96e..46b04afb 100644
--- a/util.c
+++ b/util.c
@@ -427,8 +427,11 @@ int parse_layout_faulty(char *layout)
        int ln = strcspn(layout, "0123456789");
        char *m = xstrdup(layout);
        int mode;
+
        m[ln] = 0;
        mode = map_name(faultylayout, m);
+       free(m);
+
        if (mode == UnSet)
                return -1;

--
2.27.0

[PATCH 2/5 v2] Detail: fix memleak

[Wu Guanghao]

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

Reported-by: Coverity
Signed-off-by: Wu Guanghao <wuguanghao3@huawei.com>
Acked-by: Mariusz Tkaczyk <mariusz.tkaczyk@linux.intel.com>
---
 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.27.0

[PATCH 3/5 v2] load_imsm_mpb: fix double free

[Wu Guanghao]

When free(super->buf) but not set super->buf = NULL, will be double free.

get_super_block
	err = load_and_parse_mpb
		load_imsm_mpb(.., s, ..)
			if (posix_memalign(&super->buf, MAX_SECTOR_SIZE, super->len) != 0) // true, super->buf != NULL
			if (posix_memalign(&super->migr_rec_buf, MAX_SECTOR_SIZE,); // false
				free(super->buf); //but super->buf not set NULL
				return 2;

	if err ! = 0
		if (s)
			free_imsm(s)
				 __free_imsm(s)
					if (s)
						free(s->buf); //double free

Signed-off-by: Wu Guanghao <wuguanghao3@huawei.com>
---
 super-intel.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/super-intel.c b/super-intel.c
index ba3bd41f..ee9e112e 100644
--- a/super-intel.c
+++ b/super-intel.c
@@ -4453,6 +4453,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.27.0

[PATCH 4/5 v2] find_disk_attached_hba: fix memleak

[Wu Guanghao]

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

Reported-by: Coverity
Signed-off-by: Wu Guanghao <wuguanghao3@huawei.com>
---
 super-intel.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/super-intel.c b/super-intel.c
index ee9e112e..e94f3f65 100644
--- a/super-intel.c
+++ b/super-intel.c
@@ -701,12 +701,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.27.0

[PATCH 5/5 v2] get_vd_num_of_subarray: fix memleak

[Wu Guanghao]

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

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

diff --git a/super-ddf.c b/super-ddf.c
index 8cda23a7..827e4ae7 100644
--- a/super-ddf.c
+++ b/super-ddf.c
@@ -1599,15 +1599,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.27.0

Re: [PATCH 1/5 v2] parse_layout_faulty: fix memleak

[Mariusz Tkaczyk]

On Thu, 9 Jun 2022 11:06:13 +0800
Wu Guanghao <wuguanghao3@huawei.com> wrote:

> char *m is allocated by xstrdup but not free() before return, will cause
> a memory leak.
> 
> Signed-off-by: Wu Guanghao <wuguanghao3@huawei.com>
> ---
>  util.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/util.c b/util.c
> index cc94f96e..46b04afb 100644
> --- a/util.c
> +++ b/util.c
> @@ -427,8 +427,11 @@ int parse_layout_faulty(char *layout)
>         int ln = strcspn(layout, "0123456789");
>         char *m = xstrdup(layout);
>         int mode;
> +
>         m[ln] = 0;
>         mode = map_name(faultylayout, m);
> +       free(m);
> +
>         if (mode == UnSet)
>                 return -1;
> 
> --
> 2.27.0

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

Re: [PATCH 3/5 v2] load_imsm_mpb: fix double free

[Mariusz Tkaczyk]

On Thu, 9 Jun 2022 11:07:56 +0800
Wu Guanghao <wuguanghao3@huawei.com> wrote:

> When free(super->buf) but not set super->buf = NULL, will be double free.
> 
> get_super_block
> 	err = load_and_parse_mpb
> 		load_imsm_mpb(.., s, ..)
> 			if (posix_memalign(&super->buf, MAX_SECTOR_SIZE,
> super->len) != 0) // true, super->buf != NULL if
> (posix_memalign(&super->migr_rec_buf, MAX_SECTOR_SIZE,); // false
> free(super->buf); //but super->buf not set NULL return 2;
> 
> 	if err ! = 0
> 		if (s)
> 			free_imsm(s)
> 				 __free_imsm(s)
> 					if (s)
> 						free(s->buf); //double free
> 
> Signed-off-by: Wu Guanghao <wuguanghao3@huawei.com>
> ---
>  super-intel.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/super-intel.c b/super-intel.c
> index ba3bd41f..ee9e112e 100644
> --- a/super-intel.c
> +++ b/super-intel.c
> @@ -4453,6 +4453,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.27.0

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

Re: [PATCH 4/5 v2] find_disk_attached_hba: fix memleak

[Mariusz Tkaczyk]

On Thu, 9 Jun 2022 11:08:39 +0800
Wu Guanghao <wuguanghao3@huawei.com> wrote:

> If disk_path = diskfd_to_devpath(), we need free(disk_path) before
> return, otherwise there will be a memory leak
> 
> Reported-by: Coverity
> Signed-off-by: Wu Guanghao <wuguanghao3@huawei.com>
> ---
>  super-intel.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/super-intel.c b/super-intel.c
> index ee9e112e..e94f3f65 100644
> --- a/super-intel.c
> +++ b/super-intel.c
> @@ -701,12 +701,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.27.0

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

Re: [PATCH 2/5 v2] Detail: fix memleak

[Coly Li]

Hi Guanghao,


> 2022年6月9日 11:06，Wu Guanghao <wuguanghao3@huawei.com> 写道：
> 
> char *sysdev = xstrdup() but not free() in for loop, will cause memory
> leak.
> 
> Reported-by: Coverity


The Reported-by tag might be incorrect. Maybe I am wrong but this is the first time I see a non-email reporter here.
Here I copy and past the Reported-by: tag explanation from Linux kernel document, I guess the meaning should be similar,

The Reported-by tag gives credit to people who find bugs and report them and it
hopefully inspires them to help us again in the future.  Please note that if
the bug was reported in private, then ask for permission first before using the
Reported-by tag. The tag is intended for bugs; please do not use it to credit
feature requests.

So you may have an email address for the Reported-by tag, for example the Hulk robot in Linux kernel patches from Huawei,
    Reported-by: Hulk Robot <hulkci@huawei.com>

Could you please to update all the Reported-by tags for the series? Then I will start to review the patches.

Thanks.

Coly Li


> Signed-off-by: Wu Guanghao <wuguanghao3@huawei.com>
> Acked-by: Mariusz Tkaczyk <mariusz.tkaczyk@linux.intel.com>
> ---
> 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.27.0

Re: [PATCH 1/5 v2] parse_layout_faulty: fix memleak

[Jes Sorensen]

On 6/8/22 23:06, Wu Guanghao wrote:
> char *m is allocated by xstrdup but not free() before return, will cause
> a memory leak.
> 
> Signed-off-by: Wu Guanghao <wuguanghao3@huawei.com>
> ---
>  util.c | 3 +++
>  1 file changed, 3 insertions(+)

Hi Wu

This no longer seems to apply, would you mind rebasing and resending the
series?

Thanks,
Jes

> diff --git a/util.c b/util.c
> index cc94f96e..46b04afb 100644
> --- a/util.c
> +++ b/util.c
> @@ -427,8 +427,11 @@ int parse_layout_faulty(char *layout)
>         int ln = strcspn(layout, "0123456789");
>         char *m = xstrdup(layout);
>         int mode;
> +
>         m[ln] = 0;
>         mode = map_name(faultylayout, m);
> +       free(m);
> +
>         if (mode == UnSet)
>                 return -1;
> 
> --
> 2.27.0

Re: [PATCH 1/5 v2] parse_layout_faulty: fix memleak

[Wu Guanghao]

在 2022/7/29 5:15, Jes Sorensen 写道:
> On 6/8/22 23:06, Wu Guanghao wrote:
>> char *m is allocated by xstrdup but not free() before return, will cause
>> a memory leak.
>>
>> Signed-off-by: Wu Guanghao <wuguanghao3@huawei.com>
>> ---
>>  util.c | 3 +++
>>  1 file changed, 3 insertions(+)
> 
> Hi Wu
> 
> This no longer seems to apply, would you mind rebasing and resending the
> series?
> 
> Thanks,
> Jes
OK, I will rebase and resend later.

Wu
> 
>> diff --git a/util.c b/util.c
>> index cc94f96e..46b04afb 100644
>> --- a/util.c
>> +++ b/util.c
>> @@ -427,8 +427,11 @@ int parse_layout_faulty(char *layout)
>>         int ln = strcspn(layout, "0123456789");
>>         char *m = xstrdup(layout);
>>         int mode;
>> +
>>         m[ln] = 0;
>>         mode = map_name(faultylayout, m);
>> +       free(m);
>> +
>>         if (mode == UnSet)
>>                 return -1;
>>
>> --
>> 2.27.0
> 
> .
>