Re: [PATCH v2 1/4] xdiff: fix a memory leak

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Wed, Feb 16 2022, Phillip Wood wrote:

> Hi Ævar
>
> Thanks for taking a look at this patch
>
> On 16/02/2022 10:28, Ævar Arnfjörð Bjarmason wrote:
>> On Wed, Feb 16 2022, Phillip Wood via GitGitGadget wrote:
>> 
>>> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>>>
>>> Although the patience and histogram algorithms initialize the
>>> environment they do not free it if there is an error. In contrast for
>>> the Myers algorithm the environment is initalized in xdl_do_diff() and
>>> it is freed if there is an error. Fix this by always initializing the
>>> environment in xdl_do_diff() and freeing it there if there is an
>>> error. Remove the comment in do_patience_diff() about the environment
>>> being freed by xdl_diff() as it is not accurate because (a) xdl_diff()
>>> does not do that if there is an error and (b) xdl_diff() is not the
>>> only caller.
>>>
>>> Reported-by: Junio C Hamano <gitster@pobox.com>
>>> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
>>> ---
>>>   xdiff/xdiffi.c     | 33 +++++++++++++++++----------------
>>>   xdiff/xhistogram.c |  3 ---
>>>   xdiff/xpatience.c  |  4 ----
>>>   3 files changed, 17 insertions(+), 23 deletions(-)
>>>
>>> diff --git a/xdiff/xdiffi.c b/xdiff/xdiffi.c
>>> index 69689fab247..758410c11ac 100644
>>> --- a/xdiff/xdiffi.c
>>> +++ b/xdiff/xdiffi.c
>>> @@ -315,16 +315,19 @@ int xdl_do_diff(mmfile_t *mf1, mmfile_t *mf2, xpparam_t const *xpp,
>>>   	long *kvd, *kvdf, *kvdb;
>>>   	xdalgoenv_t xenv;
>>>   	diffdata_t dd1, dd2;
>>> +	int res;
>>>   -	if (XDF_DIFF_ALG(xpp->flags) == XDF_PATIENCE_DIFF)
>>> -		return xdl_do_patience_diff(mf1, mf2, xpp, xe);
>>> -
>>> -	if (XDF_DIFF_ALG(xpp->flags) == XDF_HISTOGRAM_DIFF)
>>> -		return xdl_do_histogram_diff(mf1, mf2, xpp, xe);
>>> +	if (xdl_prepare_env(mf1, mf2, xpp, xe) < 0)
>>> +		return -1;
>>>   -	if (xdl_prepare_env(mf1, mf2, xpp, xe) < 0) {
>>> +	if (XDF_DIFF_ALG(xpp->flags) == XDF_PATIENCE_DIFF) {
>>> +		res = xdl_do_patience_diff(mf1, mf2, xpp, xe);
>>> +		goto out;
>>> +	}
>>>   -		return -1;
>>> +	if (XDF_DIFF_ALG(xpp->flags) == XDF_HISTOGRAM_DIFF) {
>>> +		res = xdl_do_histogram_diff(mf1, mf2, xpp, xe);
>>> +		goto out;
>>>   	}
>>>     	/*
>>> @@ -359,17 +362,15 @@ int xdl_do_diff(mmfile_t *mf1, mmfile_t *mf2, xpparam_t const *xpp,
>>>   	dd2.rchg = xe->xdf2.rchg;
>>>   	dd2.rindex = xe->xdf2.rindex;
>>>   -	if (xdl_recs_cmp(&dd1, 0, dd1.nrec, &dd2, 0, dd2.nrec,
>>> -			 kvdf, kvdb, (xpp->flags & XDF_NEED_MINIMAL) != 0, &xenv) < 0) {
>>> -
>>> -		xdl_free(kvd);
>>> -		xdl_free_env(xe);
>>> -		return -1;
>>> -	}
>>> -
>>> +	res = xdl_recs_cmp(&dd1, 0, dd1.nrec, &dd2, 0, dd2.nrec,
>>> +			   kvdf, kvdb, (xpp->flags & XDF_NEED_MINIMAL) != 0,
>>> +			   &xenv);
>>>   	xdl_free(kvd);
>>> + out:
>>> +	if (res < 0)
>>> +		xdl_free_env(xe);
>>>   -	return 0;
>>> +	return res;
>>>   }
>> I wanted to test if this made some diff test pass under
>> SANITIZE=leak
>> that didn't before, and my usual quick way to discover that is to run
>> the tests with something like this for testing:
>> 	
>> 	diff --git a/xdiff/xdiffi.c b/xdiff/xdiffi.c
>> 	index 758410c11ac..7811ce2a1d3 100644
>> 	--- a/xdiff/xdiffi.c
>> 	+++ b/xdiff/xdiffi.c
>> 	@@ -368,7 +368,7 @@ int xdl_do_diff(mmfile_t *mf1, mmfile_t *mf2, xpparam_t const *xpp,
>> 	 	xdl_free(kvd);
>> 	  out:
>> 	 	if (res < 0)
>> 	-		xdl_free_env(xe);
>> 	+		BUG("needed freeing");
>> 	
>> 	 	return res;
>> 	 }
>> But doing so has all tests passing, so either this code is
>> unreachable
>> or not reachable by any of our tests.
>
> Yeah, it is fixing a leak in the error path. The only possible error
> is an allocation failure and we die rather than returning in that case
> so the test suite is not going to tell us anything about this patch.

Indeed. I meant that comment as "here's the first thing I tried, and
when that didn't do anything I went digging further, and...".

>> More generally I think this patch is taking the wrong approach, why are
>> we trying to the members of a stack variable in xdl_do_diff(), when that
>> variable isn't ours, but is created by our caller? Why aren't they
>> dealing with it?
>
> They are not dealing with it because they do not initialize it - it is
> an "out" parameter that is used to return data to the caller. This
> patch changes the logic to "whoever initializes it is responsible for
> freeing it if there is an error". By doing that we localize the error
> handling to xdl_do_diff() and can leave the callers unchanged.

Yes, I'm saying that we're needlessly piling on complexity by continuing
with this pattern in the xdiff/ codebase. I think it's fair to question
the direction in general.

>> The main issue the memory handling is such a pain here is because of
>> that mixed ownership.
>> The below POC (and seems to pass tests) shows a way to do that,
>> which I
>> think in general is *much* simpler. I.e. sure we'll call free()
>> redundantly some of the time, but that'll be safe as long as we
>> zero-initialize the relevant struct.
>> The xdiff code is much harder to read & maintain than it needs to be
>> because it's trying to micro-optimize these sort of patterns.
>> E.g. with the diff below we'll now redundantly call the free on a
>> xe2
>> when we only used a xe1, but "who cares?" is my considered opinion on
>> that topic :)
>
> Then you should reread the cover letter and commit message and code
> comment in patch 3 that all mention avoiding a double free in that
> case. If you have any ideas how to make those clearer please let me
> know.

To use FREE_AND_NULL() instead of free()? Then double-freeing stops
being an issue, and we don't need to do the accounting.

> If xdl_do_diff(orig, mf2, xpp, &xe2) fails it frees any members of xe2
> that it allocated but it does not NULL them. If we then call 
> xdl_free_env(&xe2) we will have a double free error.

Right. I'm saying that if you follow the chain from xdl_do_diff() down
we call e.g. xdl_do_patience_diff(), which then calls xdl_prepare_env(),
which will on failure call e.g. xdl_free_ctx(&xe->xdf2), i.e. freeing
specific things inside the "xe" struct that it specifically knows it
allocated on those codepaths.

If we instead stick to the rule that he who created the stack variable
is responsible for freeing it, we generally speaking avoid the need for
FREE_AND_NULL() over free(), but in either case the control flow becomes
much simpler and less verbose.

I.e. we do away with the incremental freeing because some function 3
stack frames down knows it populated 1/10 struct fields, instead it all
becomes:

 1. Do we own this variable? No, return -1 then. No freeing here.
 2. The thing that owns the variable calls free_thingy(&x)
 3. We created "x" with "x = { 0 }" so calling free() on all its fields is safe
 4. We do just that.

It's a lot simpler at the cost of a tiny bit of redundant work, but in
these cases we're on the error path anyway, so it won't be a performance
issue.

That's how we tend to do it with all of git's internal APIs as far as
freeing is concerned, xdiff doesn't do it because it's externally
imported. But I think it's a better direction, and something that wolud
work for both git.git and libgit2.git (and the diff/code size isn't that
different).

>
>> By not trying to micro-optimize it like that the memory management and
>> ownership becomes so much easier to deal with.
>> diff --git a/xdiff/xdiffi.c b/xdiff/xdiffi.c
>> index 758410c11ac..6ad30a98b62 100644
>> --- a/xdiff/xdiffi.c
>> +++ b/xdiff/xdiffi.c
>> @@ -1054,19 +1054,19 @@ static void xdl_mark_ignorable_regex(xdchange_t *xscr, const xdfenv_t *xe,
>>   int xdl_diff(mmfile_t *mf1, mmfile_t *mf2, xpparam_t const *xpp,
>>   	     xdemitconf_t const *xecfg, xdemitcb_t *ecb) {
>>   	xdchange_t *xscr;
>> -	xdfenv_t xe;
>> +	xdfenv_t xe = { 0 };
>>   	emit_func_t ef = xecfg->hunk_func ? xdl_call_hunk_func : xdl_emit_diff;
>> +	int status = 0;
>>     	if (xdl_do_diff(mf1, mf2, xpp, &xe) < 0) {
>> -
>> -		return -1;
>> +		status = -1;
>> +		goto cleanup;
>>   	}
>>   	if (xdl_change_compact(&xe.xdf1, &xe.xdf2, xpp->flags) < 0 ||
>>   	    xdl_change_compact(&xe.xdf2, &xe.xdf1, xpp->flags) < 0 ||
>>   	    xdl_build_script(&xe, &xscr) < 0) {
>> -
>> -		xdl_free_env(&xe);
>> -		return -1;
>> +		status = -1;
>> +		goto cleanup;
>>   	}
>>   	if (xscr) {
>>   		if (xpp->flags & XDF_IGNORE_BLANK_LINES)
>> @@ -1078,12 +1078,14 @@ int xdl_diff(mmfile_t *mf1, mmfile_t *mf2, xpparam_t const *xpp,
>>   		if (ef(&xe, xscr, ecb, xecfg) < 0) {
>>     			xdl_free_script(xscr);
>> -			xdl_free_env(&xe);
>> -			return -1;
>> +			status = -1;
>> +			goto cleanup;
>>   		}
>>   		xdl_free_script(xscr);
>>   	}
>> +
>> +cleanup:
>>   	xdl_free_env(&xe);
>>   -	return 0;
>> +	return status;
>>   }
>> diff --git a/xdiff/xmerge.c b/xdiff/xmerge.c
>> index fff0b594f9a..4751eab9c12 100644
>> --- a/xdiff/xmerge.c
>> +++ b/xdiff/xmerge.c
>> @@ -685,7 +685,8 @@ 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;
>> +	xdfenv_t xe1 = { 0 };
>> +	xdfenv_t xe2 = { 0 };
>>   	int status;
>>   	xpparam_t const *xpp = &xmp->xpp;
>>   @@ -693,25 +694,25 @@ int xdl_merge(mmfile_t *orig, mmfile_t *mf1,
>> mmfile_t *mf2,
>>   	result->size = 0;
>>     	if (xdl_do_diff(orig, mf1, xpp, &xe1) < 0) {
>> -		return -1;
>> +		status = -1;
>> +		goto cleanup;
>>   	}
>>   	if (xdl_do_diff(orig, mf2, xpp, &xe2) < 0) {
>> -		xdl_free_env(&xe1);
>> -		return -1;
>> +		status = -1;
>> +		goto cleanup;
>>   	}
>>   	if (xdl_change_compact(&xe1.xdf1, &xe1.xdf2, xpp->flags) < 0 ||
>>   	    xdl_change_compact(&xe1.xdf2, &xe1.xdf1, xpp->flags) < 0 ||
>>   	    xdl_build_script(&xe1, &xscr1) < 0) {
>> -		xdl_free_env(&xe1);
>> -		return -1;
>> +		status = -1;
>> +		goto cleanup;
>>   	}
>>   	if (xdl_change_compact(&xe2.xdf1, &xe2.xdf2, xpp->flags) < 0 ||
>>   	    xdl_change_compact(&xe2.xdf2, &xe2.xdf1, xpp->flags) < 0 ||
>>   	    xdl_build_script(&xe2, &xscr2) < 0) {
>>   		xdl_free_script(xscr1);
>> -		xdl_free_env(&xe1);
>> -		xdl_free_env(&xe2);
>> -		return -1;
>> +		status = -1;
>> +		goto cleanup;
>>   	}
>>   	status = 0;
>>   	if (!xscr1) {
>> @@ -730,6 +731,7 @@ int xdl_merge(mmfile_t *orig, mmfile_t *mf1, mmfile_t *mf2,
>>   	xdl_free_script(xscr1);
>>   	xdl_free_script(xscr2);
>>   +cleanup:
>>   	xdl_free_env(&xe1);
>>   	xdl_free_env(&xe2);
>>   diff --git a/xdiff/xutils.c b/xdiff/xutils.c
>> index cfa6e2220ff..63fb2eee975 100644
>> --- a/xdiff/xutils.c
>> +++ b/xdiff/xutils.c
>> @@ -414,7 +414,8 @@ int xdl_fall_back_diff(xdfenv_t *diff_env, xpparam_t const *xpp,
>>   	 * ranges of lines instead of the whole files.
>>   	 */
>>   	mmfile_t subfile1, subfile2;
>> -	xdfenv_t env;
>> +	xdfenv_t env = { 0 };
>> +	int status = 0;
>>     	subfile1.ptr = (char *)diff_env->xdf1.recs[line1 - 1]->ptr;
>>   	subfile1.size = diff_env->xdf1.recs[line1 + count1 - 2]->ptr +
>> @@ -422,13 +423,16 @@ int xdl_fall_back_diff(xdfenv_t *diff_env, xpparam_t const *xpp,
>>   	subfile2.ptr = (char *)diff_env->xdf2.recs[line2 - 1]->ptr;
>>   	subfile2.size = diff_env->xdf2.recs[line2 + count2 - 2]->ptr +
>>   		diff_env->xdf2.recs[line2 + count2 - 2]->size - subfile2.ptr;
>> -	if (xdl_do_diff(&subfile1, &subfile2, xpp, &env) < 0)
>> -		return -1;
>> +	if (xdl_do_diff(&subfile1, &subfile2, xpp, &env) < 0) {
>> +		status = -1;
>> +		goto cleanup;
>> +	}
>>     	memcpy(diff_env->xdf1.rchg + line1 - 1, env.xdf1.rchg,
>> count1);
>>   	memcpy(diff_env->xdf2.rchg + line2 - 1, env.xdf2.rchg, count2);
>>   +cleanup:
>>   	xdl_free_env(&env);
>>   -	return 0;
>> +	return status;
>>   }