Re: [dm-devel] [PATCH] libmultipath: fix memory leaks in coalesce_paths

[lixiaokeng <lixiaokeng@huawei.com>]

On 2020/10/17 3:51, Martin Wilck wrote:
> On Fri, 2020-10-16 at 14:23 +0800, lixiaokeng wrote:
>> When multipath -F are executed firstly and multipath -v2 or
>> -d are executed later, asan will warn memory leaks. The
>> reason is that the mpp allocated in coalesce_paths isn't
>> freed. Here we add newmp in configure(multipath) to store
>> mpp and free it.
>>
>> Signed-off-by: Lixiaokeng <lixiaokeng@huawei.com>
>> Signed-off-by: Zhiqiang Liu <liuzhiqiang26@huawei.com>
>> Signed-off-by: Linfeilong <linfeilong@huawei.com>
> 
> Besides what Ben noted already, I think you shouldn't force callers to
> pass a non-NULL "newmp". The tricky part is to make sure that paths
> aren't handled repeatedly in the CMD_DRY_RUN case. Currently pp->mpp !=
> NULL is the condition used by coalesce_paths() to check if a path has
> already been dealt with; if you simply call remove_map(), that won't
> work any more. I suppose you realized that, and that's why you
> introduced the non-NULL newmp in multipath (you should have mentioned
> that in the changelog message).
> 
> I suggest to have callers pass a "vector *pnewmp" instead of "vector
> newmp", always allocate newmp in coalesce_paths(), and upon return,
> either free newmp, or assign it to the pointer passed by the caller:
> 
>      if (pnewmp)
> 	    *pnewmp = newmp;
>      else
>             free_multipathvec(newmp, KEEP_PATHS);
> 
> Regards,
> Martin
>
Hi Martin,
   Thanks for your review. It is a great idea with your suggestion.
I think it's better that callers pass a "vector mpvec" instead of
"vector newmp", either copy mpvec to newmp or allocate newmp at the
beginning of coalesce_paths, and free newmp or not at the end:

int coalesce_paths (struct vectors *vecs, vector mpvec, char *refwwid,
		    int force_reload, enum mpath_cmds cmd)
	......

 	if (mpvec)
		newmp = mpvec;
	else
		newmp = vector_alloc();
	if (!newmp) {
		condlog(0, "can not allocate newmp");
		return ret;
	}
	......
	
 	if (!mpvec)
		free_multipathvec(newmp, KEEP_PATHS);

About conflict, can you give me the url of the code with your
patchset? If I just change coalease_paths, will it have some
conflicts?

Regards,
Lixiaokeng

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel