Re: [dm-devel] [PATCH V2] libmultipath: free pp if store_path fails in disassemble_map
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 > > > Signed-off-by: lixiaokeng > > > --- > > > 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 -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] [PATCH V2] libmultipath: free pp if store_path fails in disassemble_map
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 > > Signed-off-by: lixiaokeng > > --- > > 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, ); > > > > 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; > -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] [PATCH V2] libmultipath: free pp if store_path fails in disassemble_map
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 >> Signed-off-by: lixiaokeng >> --- >> 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, ); >> >> 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; > > > > . > -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] [PATCH V2] libmultipath: free pp if store_path fails in disassemble_map
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 > Signed-off-by: lixiaokeng > --- > 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, ); > > 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; -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] [PATCH V2] libmultipath: free pp if store_path fails in disassemble_map
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 > Signed-off-by: Zhiqiang Liu > Signed-off-by: lixiaokeng > --- > 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, ); > > 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 > -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
[dm-devel] [PATCH V2] libmultipath: free pp if store_path fails in disassemble_map
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 Signed-off-by: lixiaokeng --- 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, ); 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 -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel