[1/7] xdiff: simplify freeing patterns around xdl_free_env()

Commit Message

Ævar Arnfjörð Bjarmason July 8, 2022, 2:20 p.m. UTC

Change the invocations of xdl_free_env() so that only the function
that allocated the "xe" variable frees it, rather than passing it to a
function that might use "&xe", and on failure having that function
free it.

This simplifies the allocation management, since due to the new "{ 0
}" initialization we can pass "&xe" to xdl_free_env() without
accounting for the "&xe" not being initialized yet.

This change was originally suggested as an amendment of the series
that since got merged in 47be28e51e6 (Merge branch
'pw/xdiff-alloc-fail', 2022-03-09) in [1]. The "avoid double free of
xe2" in that series is one of the pattern we can simplify here.

1. https://lore.kernel.org/git/[e-mail]/

Signed-off-by: Ævar Arnfjörð Bjarmason <[e-mail]>
---
 xdiff/xdiffi.c | 40 ++++++++++++++++------------------------
 xdiff/xmerge.c | 47 ++++++++++++++++++++++++++++-------------------
 xdiff/xutils.c | 12 ++++++++----
 3 files changed, 52 insertions(+), 47 deletions(-)

Patch

diff --git a/xdiff/xdiffi.c b/xdiff/xdiffi.c
index 53e803e6bcb..6fded43e87d 100644
--- a/xdiff/xdiffi.c
+++ b/xdiff/xdiffi.c
@@ -320,15 +320,11 @@  int xdl_do_diff(mmfile_t *mf1, mmfile_t *mf2, xpparam_t const *xpp,
 	if (xdl_prepare_env(mf1, mf2, xpp, xe) < 0)
 		return -1;
 
-	if (XDF_DIFF_ALG(xpp->flags) == XDF_PATIENCE_DIFF) {
-		res = xdl_do_patience_diff(mf1, mf2, xpp, xe);
-		goto out;
-	}
+	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) {
-		res = xdl_do_histogram_diff(mf1, mf2, xpp, xe);
-		goto out;
-	}
+	if (XDF_DIFF_ALG(xpp->flags) == XDF_HISTOGRAM_DIFF)
+		return xdl_do_histogram_diff(mf1, mf2, xpp, xe);
 
 	/*
 	 * Allocate and setup K vectors to be used by the differential
@@ -337,11 +333,8 @@  int xdl_do_diff(mmfile_t *mf1, mmfile_t *mf2, xpparam_t const *xpp,
 	 * One is to store the forward path and one to store the backward path.
 	 */
 	ndiags = xe->xdf1.nreff + xe->xdf2.nreff + 3;
-	if (!XDL_ALLOC_ARRAY(kvd, 2 * ndiags + 2)) {
-
-		xdl_free_env(xe);
+	if (!XDL_ALLOC_ARRAY(kvd, 2 * ndiags + 2))
 		return -1;
-	}
 	kvdf = kvd;
 	kvdb = kvdf + ndiags;
 	kvdf += xe->xdf2.nreff + 1;
@@ -366,9 +359,6 @@  int xdl_do_diff(mmfile_t *mf1, mmfile_t *mf2, xpparam_t const *xpp,
 			   kvdf, kvdb, (xpp->flags & XDF_NEED_MINIMAL) != 0,
 			   &xenv);
 	xdl_free(kvd);
- out:
-	if (res < 0)
-		xdl_free_env(xe);
 
 	return res;
 }
@@ -1054,19 +1044,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 +1068,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 af40c88a5b3..ac0cf52f3be 100644
--- a/xdiff/xmerge.c
+++ b/xdiff/xmerge.c
@@ -365,7 +365,7 @@  static int xdl_refine_conflicts(xdfenv_t *xe1, xdfenv_t *xe2, xdmerge_t *m,
 {
 	for (; m; m = m->next) {
 		mmfile_t t1, t2;
-		xdfenv_t xe;
+		xdfenv_t xe = { 0 };
 		xdchange_t *xscr, *x;
 		int i1 = m->i1, i2 = m->i2;
 
@@ -387,8 +387,10 @@  static int xdl_refine_conflicts(xdfenv_t *xe1, xdfenv_t *xe2, xdmerge_t *m,
 		t2.ptr = (char *)xe2->xdf2.recs[m->i2]->ptr;
 		t2.size = xe2->xdf2.recs[m->i2 + m->chg2 - 1]->ptr
 			+ xe2->xdf2.recs[m->i2 + m->chg2 - 1]->size - t2.ptr;
-		if (xdl_do_diff(&t1, &t2, xpp, &xe) < 0)
+		if (xdl_do_diff(&t1, &t2, xpp, &xe) < 0) {
+			xdl_free_env(&xe);
 			return -1;
+		}
 		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) {
@@ -684,30 +686,37 @@  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 = NULL, *xscr2 = NULL;
-	xdfenv_t xe1, xe2;
-	int status = -1;
+	xdchange_t *xscr1, *xscr2;
+	xdfenv_t xe1 = { 0 };
+	xdfenv_t xe2 = { 0 };
+	int status;
 	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)
-		goto free_xe1; /* avoid double free of xe2 */
-
+	if (xdl_do_diff(orig, mf1, xpp, &xe1) < 0) {
+		status = -1;
+		goto cleanup;
+	}
+	if (xdl_do_diff(orig, mf2, xpp, &xe2) < 0) {
+		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)
-		goto out;
-
+	    xdl_build_script(&xe1, &xscr1) < 0) {
+		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)
-		goto out;
-
+	    xdl_build_script(&xe2, &xscr2) < 0) {
+		xdl_free_script(xscr1);
+		status = -1;
+		goto cleanup;
+	}
+	status = 0;
 	if (!xscr1) {
 		result->ptr = xdl_malloc(mf2->size);
 		if (!result->ptr)
@@ -731,9 +740,9 @@  int xdl_merge(mmfile_t *orig, mmfile_t *mf1, mmfile_t *mf2,
 	xdl_free_script(xscr1);
 	xdl_free_script(xscr2);
 
-	xdl_free_env(&xe2);
- free_xe1:
+cleanup:
 	xdl_free_env(&xe1);
+	xdl_free_env(&xe2);
 
 	return status;
 }
diff --git a/xdiff/xutils.c b/xdiff/xutils.c
index 9e36f24875d..a6f10353cff 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,15 +423,18 @@  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;
 }
 
 void* xdl_alloc_grow_helper(void *p, long nr, long *alloc, size_t size)