git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date
Jul 8, 2022, 14:20 UTC
Message-ID
<patch-1.7-ad61f4e20b1-20220708T140354Z-avarab@gmail.com>
In-Reply-To
<cover-0.7-00000000000-20220708T140354Z-avarab@gmail.com>

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/220216.86tuczt7js.gmgdl@evledraar.gmail.com/
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 xdiff/xdiffi.c | 40 ++++++++++++++++------------------------
 xdiff/xmerge.c | 47 ++++++++++++++++++++++++++++-------------------
 xdiff/xutils.c | 12 ++++++++----
 3 files changed, 52 insertions(+), 47 deletions(-)
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)

-- 
2.37.0.913.g189dca38629
Previous: Ævar Arnfjörð BjarmasonNext: Ævar Arnfjörð Bjarmason
Message 15 of 52 in “xdiff: introduce memory allocation macros”
  1. 0/3 xdiff: introduce memory allocation macrosPhillip Wood via GitGitGadget, Jun 29, 2022
  2. 1/3 xdiff: introduce XDL_ALLOC_ARRAY()Phillip Wood via GitGitGadget, Jun 29, 2022
  3. 2/3 xdiff: introduce XDL_CALLOC_ARRAY()Phillip Wood via GitGitGadget, Jun 29, 2022
  4. Junio C HamanoJun 30, 2022
  5. Phillip WoodJul 6, 2022
  6. 3/3 xdiff: introduce XDL_ALLOC_GROW()Phillip Wood via GitGitGadget, Jun 29, 2022
  7. Ævar Arnfjörð BjarmasonJun 30, 2022
  8. Phillip WoodJun 30, 2022
  9. Phillip WoodJun 30, 2022
  10. Ævar Arnfjörð BjarmasonJun 30, 2022
  11. Phillip WoodJul 6, 2022
  12. Ævar Arnfjörð BjarmasonJul 7, 2022
  13. Phillip WoodJul 8, 2022
  14. 0/7 xdiff: use standard alloc macros, share them via git-shared-util.hÆvar Arnfjörð Bjarmason, Jul 8, 2022
  15. 1/7 xdiff: simplify freeing patterns around xdl_free_env()Ævar Arnfjörð Bjarmason, Jul 8, 2022
  16. 2/7 git-shared-util.h: move "shared" allocation utilities hereÆvar Arnfjörð Bjarmason, Jul 8, 2022
  17. 3/7 git-shared-util.h: add G*() versions of *ALLOC_*()Ævar Arnfjörð Bjarmason, Jul 8, 2022
  18. Phillip WoodJul 11, 2022
  19. 5/7 xdiff: use GALLOC_GROW(), not XDL_ALLOC_GROW()Ævar Arnfjörð Bjarmason, Jul 8, 2022
  20. Phillip WoodJul 11, 2022
  21. Ævar Arnfjörð BjarmasonJul 11, 2022
  22. Phillip WoodJul 13, 2022
  23. Ævar Arnfjörð BjarmasonJul 13, 2022
  24. Phillip WoodJul 13, 2022
  25. 4/7 xdiff: use G[C]ALLOC_ARRAY(), not XDL_CALLOC_ARRAY()Ævar Arnfjörð Bjarmason, Jul 8, 2022
  26. Phillip WoodJul 11, 2022
  27. 6/7 xdiff: remove xdl_malloc() wrapper, use malloc(), not xmalloc()Ævar Arnfjörð Bjarmason, Jul 8, 2022
  28. Phillip WoodJul 8, 2022
  29. Ævar Arnfjörð BjarmasonJul 8, 2022
  30. Jeff KingJul 8, 2022
  31. Ævar Arnfjörð BjarmasonJul 8, 2022
  32. Jeff KingJul 11, 2022
  33. 7/7 xdiff: remove xdl_free(), use free() insteadÆvar Arnfjörð Bjarmason, Jul 8, 2022
  34. Phillip WoodJul 8, 2022
  35. Ævar Arnfjörð BjarmasonJul 8, 2022
  36. Phillip WoodJul 11, 2022
  37. Phillip WoodJul 11, 2022
  38. Ævar Arnfjörð BjarmasonJul 11, 2022
  39. Phillip WoodJul 13, 2022
  40. Ævar Arnfjörð BjarmasonJul 13, 2022
  41. Junio C HamanoJun 30, 2022
  42. Phillip WoodJul 6, 2022
  43. Ævar Arnfjörð BjarmasonJun 30, 2022
  44. 0/4 xdiff: introduce memory allocation macrosPhillip Wood via GitGitGadget, Jul 8, 2022
  45. 2/4 xdiff: introduce xdl_callocPhillip Wood via GitGitGadget, Jul 8, 2022
  46. 1/4 xdiff: introduce XDL_ALLOC_ARRAY()Phillip Wood via GitGitGadget, Jul 8, 2022
  47. 4/4 xdiff: introduce XDL_ALLOC_GROW()Phillip Wood via GitGitGadget, Jul 8, 2022
  48. Ævar Arnfjörð BjarmasonJul 8, 2022
  49. Phillip WoodJul 11, 2022
  50. Jeff KingJul 12, 2022
  51. Phillip WoodJul 13, 2022
  52. 3/4 xdiff: introduce XDL_CALLOC_ARRAY()Phillip Wood via GitGitGadget, Jul 8, 2022

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.