Re: [PATCH 2/3] xdiff: refactor a function

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 10, 2022, 06:28 UTC
Message-ID: <xmqq8rujxmkf.fsf@gitster.g>
In-Reply-To: <xmqqmtiz9aro.fsf@gitster.g>

Junio C Hamano <gitster@pobox.com> writes:

Show 25 quoted lines

>>  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.

Thanks.