[PATCH V2] libmultipath: free pp if store_path fails in disassemble_map

[PATCH V2] libmultipath: free pp if store_path fails in disassemble_map

[Zhiqiang Liu]

In disassemble_map func, one pp will be allocated and stored in
pgp->paths. However, if store_path fails, pp will not be freed,
then memory leak problem occurs.

Here, we will call free_path to free pp when store_path fails.

Signed-off-by: Zhiqiang Liu <liuzhiqiang26@huawei.com>
Signed-off-by: lixiaokeng <lixiaokeng@huawei.com>
---
V1->V2: update based on ups/submit-2007 branch.

 libmultipath/dmparser.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/libmultipath/dmparser.c b/libmultipath/dmparser.c
index b9858fa5..8a0501ba 100644
--- a/libmultipath/dmparser.c
+++ b/libmultipath/dmparser.c
@@ -143,6 +143,7 @@ int disassemble_map(const struct _vector *pathvec,
 	int def_minio = 0;
 	struct path * pp;
 	struct pathgroup * pgp;
+	int pp_alloc_flag = 0;

 	assert(pathvec != NULL);
 	p = params;
@@ -292,6 +293,7 @@ int disassemble_map(const struct _vector *pathvec,

 		for (j = 0; j < num_paths; j++) {
 			pp = NULL;
+			pp_alloc_flag = 0;
 			p += get_word(p, &word);

 			if (!word)
@@ -304,13 +306,16 @@ int disassemble_map(const struct _vector *pathvec,

 				if (!pp)
 					goto out1;
-
+				pp_alloc_flag = 1;
 				strlcpy(pp->dev_t, word, BLK_DEV_SIZE);
 			}
 			FREE(word);

-			if (store_path(pgp->paths, pp))
+			if (store_path(pgp->paths, pp)) {
+				if (pp_alloc_flag)
+					free_path(pp);
 				goto out;
+			}

 			pgp->id ^= (long)pp;
 			pp->pgindex = i + 1;
-- 
2.24.0.windows.2

Re: [PATCH V2] libmultipath: free pp if store_path fails in disassemble_map

[Benjamin Marzinski]

On Fri, Jul 24, 2020 at 09:40:18AM +0800, Zhiqiang Liu wrote:
> In disassemble_map func, one pp will be allocated and stored in
> pgp->paths. However, if store_path fails, pp will not be freed,
> then memory leak problem occurs.
> 
> Here, we will call free_path to free pp when store_path fails.
> 
Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
> Signed-off-by: Zhiqiang Liu <liuzhiqiang26@huawei.com>
> Signed-off-by: lixiaokeng <lixiaokeng@huawei.com>
> ---
> V1->V2: update based on ups/submit-2007 branch.
> 
>  libmultipath/dmparser.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/libmultipath/dmparser.c b/libmultipath/dmparser.c
> index b9858fa5..8a0501ba 100644
> --- a/libmultipath/dmparser.c
> +++ b/libmultipath/dmparser.c
> @@ -143,6 +143,7 @@ int disassemble_map(const struct _vector *pathvec,
>  	int def_minio = 0;
>  	struct path * pp;
>  	struct pathgroup * pgp;
> +	int pp_alloc_flag = 0;
> 
>  	assert(pathvec != NULL);
>  	p = params;
> @@ -292,6 +293,7 @@ int disassemble_map(const struct _vector *pathvec,
> 
>  		for (j = 0; j < num_paths; j++) {
>  			pp = NULL;
> +			pp_alloc_flag = 0;
>  			p += get_word(p, &word);
> 
>  			if (!word)
> @@ -304,13 +306,16 @@ int disassemble_map(const struct _vector *pathvec,
> 
>  				if (!pp)
>  					goto out1;
> -
> +				pp_alloc_flag = 1;
>  				strlcpy(pp->dev_t, word, BLK_DEV_SIZE);
>  			}
>  			FREE(word);
> 
> -			if (store_path(pgp->paths, pp))
> +			if (store_path(pgp->paths, pp)) {
> +				if (pp_alloc_flag)
> +					free_path(pp);
>  				goto out;
> +			}
> 
>  			pgp->id ^= (long)pp;
>  			pp->pgindex = i + 1;
> -- 
> 2.24.0.windows.2
> 

Re: [PATCH V2] libmultipath: free pp if store_path fails in disassemble_map

[Martin Wilck]

Hello Liu,

On Fri, 2020-07-24 at 09:40 +0800, Zhiqiang Liu wrote:
> In disassemble_map func, one pp will be allocated and stored in
> pgp->paths. However, if store_path fails, pp will not be freed,
> then memory leak problem occurs.
> 
> Here, we will call free_path to free pp when store_path fails.
> 
> Signed-off-by: Zhiqiang Liu <liuzhiqiang26@huawei.com>
> Signed-off-by: lixiaokeng <lixiaokeng@huawei.com>
> ---
> V1->V2: update based on ups/submit-2007 branch.
> 
>  libmultipath/dmparser.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 

thanks for the patch. I'd suggest to do this without the pp_alloc_flag
variable, by pulling the store_path() call into the if (!pp)
conditional and treating failure differently in both branches of the
conditional. (Side note: if we introduce a boolean like this, we should
use "bool" as the variable type).

Unless you object, I'll add that change on top of my submit-2007
series, giving you appropriate credits.

@Ben, @Christophe: I've been considering for some time to start
handling cases like this with __attribute__((cleanup(f)))) (
https://gcc.gnu.org/onlinedocs/gcc/Common-Variable-Attributes.html).
That could reduce the amount of goto clauses for error handling
significantly. It would be a major change in coding style though. What
do you think?

Regards,
Martin


> diff --git a/libmultipath/dmparser.c b/libmultipath/dmparser.c
> index b9858fa5..8a0501ba 100644
> --- a/libmultipath/dmparser.c
> +++ b/libmultipath/dmparser.c
> @@ -143,6 +143,7 @@ int disassemble_map(const struct _vector
> *pathvec,
>  	int def_minio = 0;
>  	struct path * pp;
>  	struct pathgroup * pgp;
> +	int pp_alloc_flag = 0;
> 
>  	assert(pathvec != NULL);
>  	p = params;
> @@ -292,6 +293,7 @@ int disassemble_map(const struct _vector
> *pathvec,
> 
>  		for (j = 0; j < num_paths; j++) {
>  			pp = NULL;
> +			pp_alloc_flag = 0;
>  			p += get_word(p, &word);
> 
>  			if (!word)
> @@ -304,13 +306,16 @@ int disassemble_map(const struct _vector
> *pathvec,
> 
>  				if (!pp)
>  					goto out1;
> -
> +				pp_alloc_flag = 1;
>  				strlcpy(pp->dev_t, word, BLK_DEV_SIZE);
>  			}
>  			FREE(word);
> 
> -			if (store_path(pgp->paths, pp))
> +			if (store_path(pgp->paths, pp)) {
> +				if (pp_alloc_flag)
> +					free_path(pp);
>  				goto out;
> +			}
> 
>  			pgp->id ^= (long)pp;
>  			pp->pgindex = i + 1;

Re: [PATCH V2] libmultipath: free pp if store_path fails in disassemble_map

[Zhiqiang Liu]

On 2020/8/10 20:20, Martin Wilck wrote:
> Hello Liu,
> 
> On Fri, 2020-07-24 at 09:40 +0800, Zhiqiang Liu wrote:
>> In disassemble_map func, one pp will be allocated and stored in
>> pgp->paths. However, if store_path fails, pp will not be freed,
>> then memory leak problem occurs.
>>
>> Here, we will call free_path to free pp when store_path fails.
>>
>> Signed-off-by: Zhiqiang Liu <liuzhiqiang26@huawei.com>
>> Signed-off-by: lixiaokeng <lixiaokeng@huawei.com>
>> ---
>> V1->V2: update based on ups/submit-2007 branch.
>>
>>  libmultipath/dmparser.c | 9 +++++++--
>>  1 file changed, 7 insertions(+), 2 deletions(-)
>>
> 
> thanks for the patch. I'd suggest to do this without the pp_alloc_flag
> variable, by pulling the store_path() call into the if (!pp)
> conditional and treating failure differently in both branches of the
> conditional. (Side note: if we introduce a boolean like this, we should
> use "bool" as the variable type).
> 
> Unless you object, I'll add that change on top of my submit-2007
> series, giving you appropriate credits.
> 
Thanks for you review.

You are free to deal with the patch.


> @Ben, @Christophe: I've been considering for some time to start
> handling cases like this with __attribute__((cleanup(f)))) (
> https://gcc.gnu.org/onlinedocs/gcc/Common-Variable-Attributes.html).
> That could reduce the amount of goto clauses for error handling
> significantly. It would be a major change in coding style though. What
> do you think?
> 
> Regards,
> Martin
> 
> 
>> diff --git a/libmultipath/dmparser.c b/libmultipath/dmparser.c
>> index b9858fa5..8a0501ba 100644
>> --- a/libmultipath/dmparser.c
>> +++ b/libmultipath/dmparser.c
>> @@ -143,6 +143,7 @@ int disassemble_map(const struct _vector
>> *pathvec,
>>  	int def_minio = 0;
>>  	struct path * pp;
>>  	struct pathgroup * pgp;
>> +	int pp_alloc_flag = 0;
>>
>>  	assert(pathvec != NULL);
>>  	p = params;
>> @@ -292,6 +293,7 @@ int disassemble_map(const struct _vector
>> *pathvec,
>>
>>  		for (j = 0; j < num_paths; j++) {
>>  			pp = NULL;
>> +			pp_alloc_flag = 0;
>>  			p += get_word(p, &word);
>>
>>  			if (!word)
>> @@ -304,13 +306,16 @@ int disassemble_map(const struct _vector
>> *pathvec,
>>
>>  				if (!pp)
>>  					goto out1;
>> -
>> +				pp_alloc_flag = 1;
>>  				strlcpy(pp->dev_t, word, BLK_DEV_SIZE);
>>  			}
>>  			FREE(word);
>>
>> -			if (store_path(pgp->paths, pp))
>> +			if (store_path(pgp->paths, pp)) {
>> +				if (pp_alloc_flag)
>> +					free_path(pp);
>>  				goto out;
>> +			}
>>
>>  			pgp->id ^= (long)pp;
>>  			pp->pgindex = i + 1;
> 
> 
> 
> .
> 

Re: [PATCH V2] libmultipath: free pp if store_path fails in disassemble_map

[Benjamin Marzinski]

On Mon, Aug 10, 2020 at 02:20:27PM +0200, Martin Wilck wrote:
> Hello Liu,
> 
> On Fri, 2020-07-24 at 09:40 +0800, Zhiqiang Liu wrote:
> > In disassemble_map func, one pp will be allocated and stored in
> > pgp->paths. However, if store_path fails, pp will not be freed,
> > then memory leak problem occurs.
> > 
> > Here, we will call free_path to free pp when store_path fails.
> > 
> > Signed-off-by: Zhiqiang Liu <liuzhiqiang26@huawei.com>
> > Signed-off-by: lixiaokeng <lixiaokeng@huawei.com>
> > ---
> > V1->V2: update based on ups/submit-2007 branch.
> > 
> >  libmultipath/dmparser.c | 9 +++++++--
> >  1 file changed, 7 insertions(+), 2 deletions(-)
> > 
> 
> thanks for the patch. I'd suggest to do this without the pp_alloc_flag
> variable, by pulling the store_path() call into the if (!pp)
> conditional and treating failure differently in both branches of the
> conditional. (Side note: if we introduce a boolean like this, we should
> use "bool" as the variable type).
> 
> Unless you object, I'll add that change on top of my submit-2007
> series, giving you appropriate credits.
> 
> @Ben, @Christophe: I've been considering for some time to start
> handling cases like this with __attribute__((cleanup(f)))) (
> https://gcc.gnu.org/onlinedocs/gcc/Common-Variable-Attributes.html).
> That could reduce the amount of goto clauses for error handling
> significantly. It would be a major change in coding style though. What
> do you think?

So, I haven't really looked into the cleanup attribute. How would it
work here? We only want to free the path if we didn't store it. We can't
free it if it got stored.  Can you disable the cleanup? If we have to
make the cleanup function check if the path got stored, that seems like
more work than simply using the multiple goto labels like we do now.

-Ben

> 
> Regards,
> Martin
> 
> 
> > diff --git a/libmultipath/dmparser.c b/libmultipath/dmparser.c
> > index b9858fa5..8a0501ba 100644
> > --- a/libmultipath/dmparser.c
> > +++ b/libmultipath/dmparser.c
> > @@ -143,6 +143,7 @@ int disassemble_map(const struct _vector
> > *pathvec,
> >  	int def_minio = 0;
> >  	struct path * pp;
> >  	struct pathgroup * pgp;
> > +	int pp_alloc_flag = 0;
> > 
> >  	assert(pathvec != NULL);
> >  	p = params;
> > @@ -292,6 +293,7 @@ int disassemble_map(const struct _vector
> > *pathvec,
> > 
> >  		for (j = 0; j < num_paths; j++) {
> >  			pp = NULL;
> > +			pp_alloc_flag = 0;
> >  			p += get_word(p, &word);
> > 
> >  			if (!word)
> > @@ -304,13 +306,16 @@ int disassemble_map(const struct _vector
> > *pathvec,
> > 
> >  				if (!pp)
> >  					goto out1;
> > -
> > +				pp_alloc_flag = 1;
> >  				strlcpy(pp->dev_t, word, BLK_DEV_SIZE);
> >  			}
> >  			FREE(word);
> > 
> > -			if (store_path(pgp->paths, pp))
> > +			if (store_path(pgp->paths, pp)) {
> > +				if (pp_alloc_flag)
> > +					free_path(pp);
> >  				goto out;
> > +			}
> > 
> >  			pgp->id ^= (long)pp;
> >  			pp->pgindex = i + 1;
> 

Re: [PATCH V2] libmultipath: free pp if store_path fails in disassemble_map

[Martin Wilck]

On Mon, 2020-08-10 at 14:07 -0500, Benjamin Marzinski wrote:
> On Mon, Aug 10, 2020 at 02:20:27PM +0200, Martin Wilck wrote:
> > Hello Liu,
> > 
> > On Fri, 2020-07-24 at 09:40 +0800, Zhiqiang Liu wrote:
> > > In disassemble_map func, one pp will be allocated and stored in
> > > pgp->paths. However, if store_path fails, pp will not be freed,
> > > then memory leak problem occurs.
> > > 
> > > Here, we will call free_path to free pp when store_path fails.
> > > 
> > > Signed-off-by: Zhiqiang Liu <liuzhiqiang26@huawei.com>
> > > Signed-off-by: lixiaokeng <lixiaokeng@huawei.com>
> > > ---
> > > V1->V2: update based on ups/submit-2007 branch.
> > > 
> > >  libmultipath/dmparser.c | 9 +++++++--
> > >  1 file changed, 7 insertions(+), 2 deletions(-)
> > > 
> > 
> > thanks for the patch. I'd suggest to do this without the
> > pp_alloc_flag
> > variable, by pulling the store_path() call into the if (!pp)
> > conditional and treating failure differently in both branches of
> > the
> > conditional. (Side note: if we introduce a boolean like this, we
> > should
> > use "bool" as the variable type).
> > 
> > Unless you object, I'll add that change on top of my submit-2007
> > series, giving you appropriate credits.
> > 
> > @Ben, @Christophe: I've been considering for some time to start
> > handling cases like this with __attribute__((cleanup(f)))) (
> > https://gcc.gnu.org/onlinedocs/gcc/Common-Variable-Attributes.html)
> > .
> > That could reduce the amount of goto clauses for error handling
> > significantly. It would be a major change in coding style though.
> > What
> > do you think?
> 
> So, I haven't really looked into the cleanup attribute. How would it
> work here? We only want to free the path if we didn't store it. We
> can't
> free it if it got stored.  Can you disable the cleanup? If we have to
> make the cleanup function check if the path got stored, that seems
> like
> more work than simply using the multiple goto labels like we do now.

I need to think it through in more detail for this particular use case.

The general pattern makes use of the steal_ptr() macro, and the fact
that free(NULL) is a noop:

#define steal_ptr(x) ({ void *__p = x; x = NULL; __p; })

struct xyz;
void cleanup_xyz(struct xyz **p)
{
       free(*p);
}

struct xyz *func()
{
        struct xyz *xy __attribute__((cleanup(cleanup_xyz))) = NULL;

        xy = alloc_xyz();
        if (!xy)
                return;
        if (do_something_with(xy) == SUCCESS)
                /* avoid freeing xy by setting it to NULL */
                return steal_ptr(xy);
        else
                /* xy remains non-NULL and will be freed on return */
                return NULL;
}

More often than not, with this technique, gotos can be avoided
completely, and the readability is IMO improved. systemd uses this
pattern a lot.

Martin