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

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

[Wu Guanghao]

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

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 | 6 ++++--
 util.c        | 1 +
 4 files changed, 13 insertions(+), 4 deletions(-)

-- 
2.27.0

[PATCH 1/5] 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 | 1 +
 1 file changed, 1 insertion(+)

diff --git a/util.c b/util.c
index cc94f96e..da18a68d 100644
--- a/util.c
+++ b/util.c
@@ -429,6 +429,7 @@ int parse_layout_faulty(char *layout)
 	int mode;
 	m[ln] = 0;
 	mode = map_name(faultylayout, m);
+	free(m);
 	if (mode == UnSet)
 		return -1;

-- 
2.27.0

[PATCH 2/5] 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>
---
 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] 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 deletion(-)

diff --git a/super-intel.c b/super-intel.c
index ba3bd41f..ef21ffba 100644
--- a/super-intel.c
+++ b/super-intel.c
@@ -4452,7 +4452,6 @@ static int load_imsm_mpb(int fd, struct intel_super *super, char *devname)
 	if (posix_memalign(&super->migr_rec_buf, MAX_SECTOR_SIZE,
 	    MIGR_REC_BUF_SECTORS*MAX_SECTOR_SIZE) != 0) {
 		pr_err("could not allocate migr_rec buffer\n");
-		free(super->buf);
 		return 2;
 	}
 	super->clean_migration_record_by_mdmon = 0;
-- 
2.27.0

[PATCH 4/5] 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>
---
 super-intel.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/super-intel.c b/super-intel.c
index ef21ffba..98fb63d5 100644
--- a/super-intel.c
+++ b/super-intel.c
@@ -700,8 +700,11 @@ static struct sys_dev* find_disk_attached_hba(int fd, const char *devname)
 		return 0;

 	for (elem = list; elem; elem = elem->next)
-		if (path_attached_to_hba(disk_path, elem->path))
+		if (path_attached_to_hba(disk_path, elem->path)) {
+			if (disk_path != devname)
+				free(disk_path);
 			return elem;
+		}

 	if (disk_path != devname)
 		free(disk_path);
-- 
2.27.0

[PATCH 5/5] 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>
---
 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 0/5] mdadm: fix memory leak and double free

[Paul Menzel]

Dear Wu,


Thank you for the patches.

Am 31.05.22 um 08:48 schrieb Wu Guanghao:
> Through tool scanning and code review, we found several memory leaks and double free
> 
> 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 | 6 ++++--
>   util.c        | 1 +
>   4 files changed, 13 insertions(+), 4 deletions(-)

If an issues was found by a tool, could you please add a Reported-by tag 
to the corresponding commit message?

Another small nit, could you please add a dot/period at the end of 
sentences in the commit messages?


Kind regards,

Paul

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

[Mariusz Tkaczyk]

On Tue, 31 May 2022 14:49:17 +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 | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/util.c b/util.c
> index cc94f96e..da18a68d 100644
> --- a/util.c
> +++ b/util.c
> @@ -429,6 +429,7 @@ int parse_layout_faulty(char *layout)
>  	int mode;
>  	m[ln] = 0;
>  	mode = map_name(faultylayout, m);
> +	free(m);
>  	if (mode == UnSet)
>  		return -1;
> 

Hi,
Please add empty lines to separate declarations and not related code
sections.

Thanks,
Mariusz

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

[Mariusz Tkaczyk]

On Tue, 31 May 2022 14:49:46 +0800
Wu Guanghao <wuguanghao3@huawei.com> wrote:

> char *sysdev = xstrdup() but not free() in for loop, will cause memory
> leak
> 
> Signed-off-by: Wu Guanghao <wuguanghao3@huawei.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;

Acked-by: mariusz.tkaczyk@linux.intel.com

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

[Mariusz Tkaczyk]

On Tue, 31 May 2022 14:50:19 +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 deletion(-)
> 
> diff --git a/super-intel.c b/super-intel.c
> index ba3bd41f..ef21ffba 100644
> --- a/super-intel.c
> +++ b/super-intel.c
> @@ -4452,7 +4452,6 @@ static int load_imsm_mpb(int fd, struct intel_super
> *super, char *devname) if (posix_memalign(&super->migr_rec_buf,
> MAX_SECTOR_SIZE, MIGR_REC_BUF_SECTORS*MAX_SECTOR_SIZE) != 0) {
>  		pr_err("could not allocate migr_rec buffer\n");
> -		free(super->buf);
>  		return 2;
>  	}
>  	super->clean_migration_record_by_mdmon = 0;

On error, we should possibly clean-up ourselves so I would expect from 
load_imsm_mpb() to free super->buf in case when error occurs and set it
to NULL, especially that __free_imsm handles it.

Thanks,
Mariusz

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

[Mariusz Tkaczyk]

On Tue, 31 May 2022 14:50:44 +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
> 
> Signed-off-by: Wu Guanghao <wuguanghao3@huawei.com>
> ---
>  super-intel.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/super-intel.c b/super-intel.c
> index ef21ffba..98fb63d5 100644
> --- a/super-intel.c
> +++ b/super-intel.c
> @@ -700,8 +700,11 @@ static struct sys_dev* find_disk_attached_hba(int fd,
> const char *devname) return 0;
> 
>  	for (elem = list; elem; elem = elem->next)
> -		if (path_attached_to_hba(disk_path, elem->path))
> +		if (path_attached_to_hba(disk_path, elem->path)) {
> +			if (disk_path != devname)
> +				free(disk_path);
>  			return elem;
> +		}
> 
>  	if (disk_path != devname)
>  		free(disk_path);

Patch is obviously correct but we can avoid code duplication:

	for (elem = list; elem; elem = elem->next)
		if (path_attached_to_hba(disk_path, elem->path))
			break;

	if (disk_path != devname)
		free(disk_path);

	return elem;

Last list element will be NULL anyway. Could you adopt it?

Thanks,
Mariusz

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

[Mariusz Tkaczyk]

On Tue, 31 May 2022 14:51:13 +0800
Wu Guanghao <wuguanghao3@huawei.com> wrote:

> sra = sysfs_read() should be free before return in
> get_vd_num_of_subarray()
> 
> Signed-off-by: Wu Guanghao <wuguanghao3@huawei.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;
>  }

Acked-by:mariusz.tkaczyk@linux.intel.com

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

[Paul Menzel]

Dear Mariusz,


Am 31.05.22 um 10:11 schrieb Mariusz Tkaczyk:
> On Tue, 31 May 2022 14:51:13 +0800 Wu Guanghao wrote:
> 
>> sra = sysfs_read() should be free before return in
>> get_vd_num_of_subarray()
>>
>> Signed-off-by: Wu Guanghao <wuguanghao3@huawei.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;
>>   }
> 
> Acked-by:mariusz.tkaczyk@linux.intel.com

Thank you for your review. The common format is:

Acked-by: Name <address>


Kind regards,

Paul

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

[Mariusz Tkaczyk]

On Tue, 31 May 2022 10:25:31 +0200
Paul Menzel <pmenzel@molgen.mpg.de> wrote:

> Dear Mariusz,
> 
> 
> Am 31.05.22 um 10:11 schrieb Mariusz Tkaczyk:
> > On Tue, 31 May 2022 14:51:13 +0800 Wu Guanghao wrote:
> >   
> >> sra = sysfs_read() should be free before return in
> >> get_vd_num_of_subarray()
> >>
> >> Signed-off-by: Wu Guanghao <wuguanghao3@huawei.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;
> >>   }  
> > 
> > Acked-by:mariusz.tkaczyk@linux.intel.com  
> 
> Thank you for your review. The common format is:
> 
> Acked-by: Name <address>
> 
> 
Hi Paul,
Thanks, fixed.

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

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

[Wu Guanghao]

在 2022/5/31 15:36, Mariusz Tkaczyk 写道:
> On Tue, 31 May 2022 14:49:17 +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 | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/util.c b/util.c
>> index cc94f96e..da18a68d 100644
>> --- a/util.c
>> +++ b/util.c
>> @@ -429,6 +429,7 @@ int parse_layout_faulty(char *layout)
>>  	int mode;
>>  	m[ln] = 0;
>>  	mode = map_name(faultylayout, m);
>> +	free(m);
>>  	if (mode == UnSet)
>>  		return -1;
>>
> 
> Hi,
> Please add empty lines to separate declarations and not related code
> sections.
> 

Thanks for your reply. I will be modified in PATCH v2.

> Thanks,
> Mariusz
> 
> .
> 

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

[Wu Guanghao]

在 2022/5/31 16:04, Mariusz Tkaczyk 写道:
> On Tue, 31 May 2022 14:50:44 +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
>>
>> Signed-off-by: Wu Guanghao <wuguanghao3@huawei.com>
>> ---
>>  super-intel.c | 5 ++++-
>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/super-intel.c b/super-intel.c
>> index ef21ffba..98fb63d5 100644
>> --- a/super-intel.c
>> +++ b/super-intel.c
>> @@ -700,8 +700,11 @@ static struct sys_dev* find_disk_attached_hba(int fd,
>> const char *devname) return 0;
>>
>>  	for (elem = list; elem; elem = elem->next)
>> -		if (path_attached_to_hba(disk_path, elem->path))
>> +		if (path_attached_to_hba(disk_path, elem->path)) {
>> +			if (disk_path != devname)
>> +				free(disk_path);
>>  			return elem;
>> +		}
>>
>>  	if (disk_path != devname)
>>  		free(disk_path);
> 
> Patch is obviously correct but we can avoid code duplication:
> 
> 	for (elem = list; elem; elem = elem->next)
> 		if (path_attached_to_hba(disk_path, elem->path))
> 			break;
> 
> 	if (disk_path != devname)
> 		free(disk_path);
> 
> 	return elem;
> 
> Last list element will be NULL anyway. Could you adopt it?
> 
Sure, I follow your advices and keep you updated about patch V2.
> Thanks,
> Mariusz
> 
> .
> 

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

[Wu Guanghao]

在 2022/5/31 14:57, Paul Menzel 写道:
> Dear Wu,
> 
> 
> Thank you for the patches.
> 
> Am 31.05.22 um 08:48 schrieb Wu Guanghao:
>> Through tool scanning and code review, we found several memory leaks and double free
>>
>> 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 | 6 ++++--
>>   util.c        | 1 +
>>   4 files changed, 13 insertions(+), 4 deletions(-)
> 
> If an issues was found by a tool, could you please add a Reported-by tag to the corresponding commit message?
> 
> Another small nit, could you please add a dot/period at the end of sentences in the commit messages?
> 
Sure, I will add in patch V2.

> Kind regards,
> 
> Paul
> .