From: Phillip Wood <phillip.wood123@gmail.com>
Date: Fri, 11 Feb 2022 15:19:29 GMT
Subject: Re: [PATCH 2/3] xdiff: refactor a function
Message-ID: <9c5b3acb-aabc-3a0d-f4e7-e10cec410dbe@gmail.com>
In-Reply-To: <xmqq8rujxmkf.fsf@gitster.g>

Hi Junio

On 10/02/2022 06:28, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
> 
>>>   int xdl_merge(mmfile_t *orig, mmfile_t *mf1, mmfile_t *mf2,
>>>   		xmparam_t const *xmp, mmbuffer_t *result)
>>>   {
>>> -	xdchange_t *xscr1, *xscr2;
>>> -	xdfenv_t xe1, xe2;
>>> -	int status;
>>> +	xdchange_t *xscr1 = NULL, *xscr2 = NULL;
>>> +	xdfenv_t xe1 = { 0 }, xe2 = { 0 };
>>> +	int status = -1;
>>>   	xpparam_t const *xpp = &xmp->xpp;
>>>   
>>>   	result->ptr = NULL;
>>>   	result->size = 0;
>>>   
>>> -	if (xdl_do_diff(orig, mf1, xpp, &xe1) < 0) {
>>> -		return -1;
>>> -	}
>>> -	if (xdl_do_diff(orig, mf2, xpp, &xe2) < 0) {
>>> -		xdl_free_env(&xe1);
>>> -		return -1;
>>> -	}
>>
>> OK, xdl_do_diff() calls xdl_free_env(xe) before an error return (I
>> didn't check if patience and histogram also do so correctly), so the
>> original was not leaking xe1 or xe2.
> 
> After I wrote the above, I took a brief look at patience and histogram,
> and it does not seem to release resources held by "env" when it signals
> a failure by returning a negative value.  So it seems that the original
> used with patience or histogram were leaking env when it failed, and
> this patch plugs that small leak.
> 
> If that is indeed the case, please note it in the proposed log
> message.

Oh well spotted, xdl_do_diff() only frees "env" if the myers algorithm 
has an error, if the patience or histogram algorithms have an error then 
they do not free "env" and it is not freed by xdl_do_diff(). This patch 
inadvertently fixes that leak when merging but not when calling 
xdl_do_diff() to compact conflicts in zealous mode or when doing a plain 
diff. I think the simplest fix is to have xdl_do_diff() free "env" when 
there is an error what ever algorithm is used.

I'll try to put a patch together to fix the other cases. If we fix this 
leak in xdl_do_diff() then maybe we should go back to returning -1 in 
the hunk above and explain in the log message why that is ok.

Best Wishes

Phillip

> Thanks.