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@xxxxxxxxxx> > Signed-off-by: lixiaokeng <lixiaokeng@xxxxxxxxxx> > --- > 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; -- dm-devel mailing list dm-devel@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/dm-devel
