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

[PATCH 0/5 v2 resend] 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>
Acked-by: Mariusz Tkaczyk <mariusz.tkaczyk@linux.intel.com>
---
 util.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/util.c b/util.c
index 38f0420e..2e0f1de7 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

[Coly Li]

> 2022年8月2日 10:15，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>

Thanks.

Coly Li

> ---
> util.c | 3 +++
> 1 file changed, 3 insertions(+)
> 
> diff --git a/util.c b/util.c
> index 38f0420e..2e0f1de7 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

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 2/5 v2] Detail: fix memleak

[Coly Li]

> 2022年8月2日 10:15，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>

Thanks.

Coly Li

> ---
> 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>
Reviewed-by: Mariusz Tkaczyk <mariusz.tkaczyk@linux.intel.com>
---
 super-intel.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/super-intel.c b/super-intel.c
index 4ddfcf94..ddbdd3e1 100644
--- a/super-intel.c
+++ b/super-intel.c
@@ -4510,6 +4510,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

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

[Coly Li]

> 2022年8月2日 10:16，Wu Guanghao <wuguanghao3@huawei.com> 写道：
> 
> 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>
> Reviewed-by: Mariusz Tkaczyk <mariusz.tkaczyk@linux.intel.com>

Acked-by: Coly Li <colyli@suse.de>

Thanks.

Coly Li


> ---
> super-intel.c | 1 +
> 1 file changed, 1 insertion(+)
> 
> diff --git a/super-intel.c b/super-intel.c
> index 4ddfcf94..ddbdd3e1 100644
> --- a/super-intel.c
> +++ b/super-intel.c
> @@ -4510,6 +4510,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

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

diff --git a/super-intel.c b/super-intel.c
index ddbdd3e1..2a4019e7 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.27.0

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

[Coly Li]

> 2022年8月2日 10:16，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>

Thanks.

Coly Li

> ---
> super-intel.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/super-intel.c b/super-intel.c
> index ddbdd3e1..2a4019e7 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.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()

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 abbc8b09..6d4618fe 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 5/5 v2] get_vd_num_of_subarray: fix memleak

[Coly Li]

> 2022年8月2日 10:17，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>

Hi Guanghao,

I have a question for this patch, please correct me if I am wrong.



> ---
> super-ddf.c | 9 +++++++--
> 1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/super-ddf.c b/super-ddf.c
> index abbc8b09..6d4618fe 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;


Before return vcnum, should call sysfs_free(sra) ?

Thank you in advance.

Coly Li



> }
> --
> 2.27.0

Re: [PATCH 5/5 v2] get_vd_num_of_subarray: fix memleak

[Coly Li]

> 2022年9月3日 16:25，Coly Li <colyli@suse.de> 写道：
> 
> 
> 
>> 2022年8月2日 10:17，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>
> 
> Hi Guanghao,
> 
> I have a question for this patch, please correct me if I am wrong.
> 
> 
> 
>> ---
>> super-ddf.c | 9 +++++++--
>> 1 file changed, 7 insertions(+), 2 deletions(-)
>> 
>> diff --git a/super-ddf.c b/super-ddf.c
>> index abbc8b09..6d4618fe 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;
> 
> 
> Before return vcnum, should call sysfs_free(sra) ?
> 
> Thank you in advance.

Ping?

Coly Li

[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 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

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