[dm-devel] [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 at huawei.com>
> > > Signed-off-by: lixiaokeng <lixiaokeng at 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