[dm-devel] [PATCH v2 03/17] libmultipath: fix leak in foreign code

[Benjamin Marzinski]

On Tue, Feb 11, 2020 at 10:36:19AM +0100, Martin Wilck wrote:
> Hi Ben, hi Christophe,
> 
> On Wed, 2020-02-05 at 12:58 -0600, Benjamin Marzinski wrote:
> > If scandir fails or finds no foreign libraries, enable_re needs to be
> > freed before exitting.
> > 
> > Fixes: 8d03eda4 'multipath.conf: add "enable_foreign" parameter'
> > Reviewed-by: Martin Wilck <mwilck at suse.com>
> > Signed-off-by: Benjamin Marzinski <bmarzins at redhat.com>
> > ---
> >  libmultipath/foreign.c | 11 +++++++----
> >  1 file changed, 7 insertions(+), 4 deletions(-)
> 
> While trying to merge your series into my tree, I realized that this
> patch conflicts with my previously submitted patch "libmultipath:
> _init_foreign(): fix possible memory leak"
> https://www.redhat.com/archives/dm-devel/2019-October/msg00104.html
> which fixed the same issue.
> 
> The two patches are almost equal, so I really don't care which one
> is eventually taken. I just wanted to alert Christophe about the
> conflict.
> 
> Anyway, I thought that you'd ACK'd my October 72-part patch series in
> the following messages:
> 
> https://www.redhat.com/archives/dm-devel/2019-October/msg00214.html
>   (Ben: ACK on v2 of 72-part series "multipath-tools: cleanup and
> warning enablement", except 16/72 "libmultipath: make path_discovery()
> pthread_cancel()-safe")
> https://www.redhat.com/archives/dm-devel/2019-November/msg00016.html
>   (Ben: ACK on 16/72 "libmultipath: make path_discovery()
> pthread_cancel()-safe", after discussion)
> https://www.redhat.com/archives/dm-devel/2019-November/msg00085.html
>   (Ben: Ack on v2->v3 change, updated patch v3 45/72 "libmultipath: fix
> -Wsign-compare warnings with snprintf()")
> 
> Please clarify if I misunderstood that and the series still needs work
> from your PoV.

No. Your fix is fine. It's my bad. I didn't rebase my branch on top of
your fixes.

-Ben
 
> Regards
> Martin
> 
> 
> > 
> > diff --git a/libmultipath/foreign.c b/libmultipath/foreign.c
> > index 4b34e141..68e9a9b8 100644
> > --- a/libmultipath/foreign.c
> > +++ b/libmultipath/foreign.c
> > @@ -129,7 +129,7 @@ static int _init_foreign(const char
> > *multipath_dir, const char *enable)
> >  	char pathbuf[PATH_MAX];
> >  	struct dirent **di;
> >  	struct scandir_result sr;
> > -	int r, i;
> > +	int r, i, ret = 0;
> >  	regex_t *enable_re = NULL;
> >  
> >  	foreigns = vector_alloc();
> > @@ -157,13 +157,15 @@ static int _init_foreign(const char
> > *multipath_dir, const char *enable)
> >  	if (r == 0) {
> >  		condlog(3, "%s: no foreign multipath libraries found",
> >  			__func__);
> > -		return 0;
> > +		ret = 0;
> > +		goto out;
> >  	} else if (r < 0) {
> >  		r = errno;
> >  		condlog(1, "%s: error %d scanning foreign multipath
> > libraries",
> >  			__func__, r);
> >  		_cleanup_foreign();
> > -		return -r;
> > +		ret = -r;
> > +		goto out;
> >  	}
> >  
> >  	sr.di = di;
> > @@ -250,8 +252,9 @@ static int _init_foreign(const char
> > *multipath_dir, const char *enable)
> >  		free_foreign(fgn);
> >  	}
> >  	pthread_cleanup_pop(1); /* free_scandir_result */
> > +out:
> >  	pthread_cleanup_pop(1); /* free_pre */
> > -	return 0;
> > +	return ret;
> >  }
> >  
> >  int init_foreign(const char *multipath_dir, const char *enable)
>