[PATCH] libmultipath: fix a memory leak in disassemble_status func

[PATCH] libmultipath: fix a memory leak in disassemble_status func

[Zhiqiang Liu]

In disassemble_status func, for dealing with selector args,
word is allocated by get_word func. However, word is not freed.
Then a memory leak occurs.

Here, we call FREE(word) to free word.

Signed-off-by: Zhiqiang Liu <liuzhiqiang26@huawei.com>
Signed-off-by: lixiaokeng <lixiaokeng@huawei.com>
---
 libmultipath/dmparser.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/libmultipath/dmparser.c b/libmultipath/dmparser.c
index 3dc77242..a4a989b2 100644
--- a/libmultipath/dmparser.c
+++ b/libmultipath/dmparser.c
@@ -584,6 +584,7 @@ int disassemble_status(char *params, struct multipath *mpp)
 						   &def_minio) == 1 &&
 					    def_minio != mpp->minio)
 							mpp->minio = def_minio;
+					FREE(word);
 				} else
 					p += get_word(p, NULL);
 			}
-- 
2.24.0.windows.2

Re: [PATCH] libmultipath: fix a memory leak in disassemble_status func

[Benjamin Marzinski]

On Tue, Jul 28, 2020 at 09:48:06PM +0800, Zhiqiang Liu wrote:
> 
> In disassemble_status func, for dealing with selector args,
> word is allocated by get_word func. However, word is not freed.
> Then a memory leak occurs.
> 
> Here, we call FREE(word) to free word.

Err... At the risk of sounding stupid, There is a least-pending
selector?  Hannes, you added this code (commit 35ad40b4 "leastpending IO
loadbalancing is not displayed properly"). Was this a Suse thing?  Is it
still a Suse thing, or have queue-length and service-time replaced it?

At any rate, for the patch itself (assuming that the correct answer
isn't to just delete the least-pending code),

Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
 
> Signed-off-by: Zhiqiang Liu <liuzhiqiang26@huawei.com>
> Signed-off-by: lixiaokeng <lixiaokeng@huawei.com>
> ---
>  libmultipath/dmparser.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/libmultipath/dmparser.c b/libmultipath/dmparser.c
> index 3dc77242..a4a989b2 100644
> --- a/libmultipath/dmparser.c
> +++ b/libmultipath/dmparser.c
> @@ -584,6 +584,7 @@ int disassemble_status(char *params, struct multipath *mpp)
>  						   &def_minio) == 1 &&
>  					    def_minio != mpp->minio)
>  							mpp->minio = def_minio;
> +					FREE(word);
>  				} else
>  					p += get_word(p, NULL);
>  			}
> -- 
> 2.24.0.windows.2
> 

Re: [PATCH] libmultipath: fix a memory leak in disassemble_status func

[Zhiqiang Liu]

On 2020/7/29 0:20, Benjamin Marzinski wrote:
> On Tue, Jul 28, 2020 at 09:48:06PM +0800, Zhiqiang Liu wrote:
>>
>> In disassemble_status func, for dealing with selector args,
>> word is allocated by get_word func. However, word is not freed.
>> Then a memory leak occurs.
>>
>> Here, we call FREE(word) to free word.
> 
> Err... At the risk of sounding stupid, There is a least-pending
> selector?  Hannes, you added this code (commit 35ad40b4 "leastpending IO
> loadbalancing is not displayed properly"). Was this a Suse thing?  Is it
> still a Suse thing, or have queue-length and service-time replaced it?
> 
Thanks for your reply.
I am sorry that I cannot answer these questions. I found the memory leak through
code review.
In addition, I forget to add a check of whether work is null. I will send the
v2 patch.

> At any rate, for the patch itself (assuming that the correct answer
> isn't to just delete the least-pending code),
> 
> Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
>  
>> Signed-off-by: Zhiqiang Liu <liuzhiqiang26@huawei.com>
>> Signed-off-by: lixiaokeng <lixiaokeng@huawei.com>
>> ---
>>  libmultipath/dmparser.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/libmultipath/dmparser.c b/libmultipath/dmparser.c
>> index 3dc77242..a4a989b2 100644
>> --- a/libmultipath/dmparser.c
>> +++ b/libmultipath/dmparser.c
>> @@ -584,6 +584,7 @@ int disassemble_status(char *params, struct multipath *mpp)
>>  						   &def_minio) == 1 &&
>>  					    def_minio != mpp->minio)
>>  							mpp->minio = def_minio;
>> +					FREE(word);
>>  				} else
>>  					p += get_word(p, NULL);
>>  			}
>> -- 
>> 2.24.0.windows.2
>>
> 
> 
> .
> 

Re: [PATCH] libmultipath: fix a memory leak in disassemble_status func

[Martin Wilck]

Hi Ben,

On Tue, 2020-07-28 at 11:20 -0500, Benjamin Marzinski wrote:
> On Tue, Jul 28, 2020 at 09:48:06PM +0800, Zhiqiang Liu wrote:
> > In disassemble_status func, for dealing with selector args,
> > word is allocated by get_word func. However, word is not freed.
> > Then a memory leak occurs.
> > 
> > Here, we call FREE(word) to free word.
> 
> Err... At the risk of sounding stupid, There is a least-pending
> selector?  Hannes, you added this code (commit 35ad40b4 "leastpending
> IO
> loadbalancing is not displayed properly"). Was this a Suse thing?  Is
> it
> still a Suse thing, or have queue-length and service-time replaced
> it?

Yes, it's a "SUSE thing" (originally developed by HP). SLE15 still
supports it; I don't know if it's actually used. 
It was discussed here last year, see:
https://patchwork.kernel.org/patch/10878905/

Regards,
Martin