xdiff: handle allocation failures

From: Phillip Wood <[email protected]>

Rather than having to remember exactly what to free after an allocation failure use the standard "goto out" pattern. This will simplify the next commit that starts handling currently unhandled failures.

Signed-off-by Phillip Wood <[email protected]>

---
 xdiff/xmerge.c | 36 ++++++++++++++++--------------------
 1 file changed, 16 insertions(+), 20 deletions(-)

xdiff/xmerge.c +3 -3

index fff0b594f9a..48c5e9e3f35 100644
@@ -684,35 +684,30 @@ static int xdl_do_merge(xdfenv_t *xe1, xdchange_t *xscr1,
 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;
 

Quoted 14 lines

int xdl_merge(mmfile_t *orig, mmfile_t *mf1, mmfile_t *mf2,

xdiff/xmerge.c +3 -3

 		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;

xdiff/xmerge.c static int xdl_do_merge(xdfenv_t *xe1, xdchange_t *xscr1, · line 701

Quoted 7 lines

xdiff/xmerge.c +0 -7

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

Quoted 14 lines · Junio C Hamano

Quoted 14 lines

int xdl_merge(mmfile_t *orig, mmfile_t *mf1, mmfile_t *mf2,
xmparam_t const *xmp, mmbuffer_t *result)
{

xdiff/xmerge.c +3 -3

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

Quoted 11 lines

xdiff/xmerge.c static int xdl_do_merge(xdfenv_t *xe1, xdchange_t *xscr1, · line 701

Quoted 7 lines

xdiff/xmerge.c +0 -7

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

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.

From: Phillip Wood <[email protected]>

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 <[email protected]>

Signed-off-by Phillip Wood <[email protected]>

---
 xdiff/xdiffi.c     | 33 +++++++++++++++++----------------
 xdiff/xhistogram.c |  3 ---
 xdiff/xpatience.c  |  4 ----
 3 files changed, 17 insertions(+), 23 deletions(-)

xdiff/xdiffi.c +11 -15

index 69689fab247..758410c11ac 100644
@@ -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,

Quoted 64 lines · Phillip Wood via GitGitGadget

From: Phillip Wood <[email protected]>

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 <[email protected]>

Signed-off-by Phillip Wood <[email protected]>

---
  xdiff/xdiffi.c     | 33 +++++++++++++++++----------------
  xdiff/xhistogram.c |  3 ---
  xdiff/xpatience.c  |  4 ----
  3 files changed, 17 insertions(+), 23 deletions(-)

xdiff/xdiffi.c +11 -15

index 69689fab247..758410c11ac 100644
@@ -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,

xdiff/xdiffi.c int xdl_do_diff(mmfile_t *mf1, mmfile_t *mf2, xpparam_t const *xpp, · line 374

Quoted 10 lines

xdiff/xdiffi.c +6 -1

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

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?

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 :)

By not trying to micro-optimize it like that the memory management and ownership becomes so much easier to deal with.

xdiff/xdiffi.c +11 -9

index 758410c11ac..6ad30a98b62 100644
@@ -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;
  }

xdiff/xmerge.c +11 -9

index fff0b594f9a..4751eab9c12 100644
@@ -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);
 

xdiff/xutils.c +8 -4

index cfa6e2220ff..63fb2eee975 100644
@@ -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;
  }

Hi Ævar

Thanks for taking a look at this patch

Quoted 88 lines · Ævar Arnfjörð Bjarmason

Quoted 58 lines · Phillip Wood via GitGitGadget

From: Phillip Wood <[email protected]>

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 <[email protected]>

Signed-off-by Phillip Wood <[email protected]>

---
  xdiff/xdiffi.c     | 33 +++++++++++++++++----------------
  xdiff/xhistogram.c |  3 ---
  xdiff/xpatience.c  |  4 ----
  3 files changed, 17 insertions(+), 23 deletions(-)

xdiff/xdiffi.c +10 -11

index 69689fab247..758410c11ac 100644
@@ -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;
-	}
-

xdiff/xdiffi.c int xdl_do_diff(mmfile_t *mf1, mmfile_t *mf2, xpparam_t const *xpp, · line 374

Quoted 10 lines

xdiff/xdiffi.c +7 -0

+	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;
	 }

Quoted 3 lines

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.

Quoted 4 lines

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.

Quoted 5 lines

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()

Quoted 8 lines

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.

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.

Quoted 132 lines

By not trying to micro-optimize it like that the memory management and ownership becomes so much easier to deal with.

xdiff/xdiffi.c +11 -8

index 758410c11ac..6ad30a98b62 100644
@@ -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;
  }

xdiff/xmerge.c +2 -1

index fff0b594f9a..4751eab9c12 100644
@@ -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) {

xdiff/xmerge.c +8 -8

-		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

xdiff/xutils.c +7 -3

@@ -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;
  }

Quoted 5 lines

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.

[...]

Quoted 10 lines

Quoted 4 lines

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.