[PATCH 0/6] apply: fix leaking buffer of `struct image`

[PATCH 0/6] apply: fix leaking buffer of `struct image`

[Patrick Steinhardt]

Hi,

this patch series addresses memory leaks in "apply.c" revolving around
`struct image`. I've split the series out of the usual leak fix series
because I found the code really hard to reason about. So instead of just
trying to plug the leak with a least-effort patch I refactored it a bit
to make it more straight-forward. Most importantly, the last patch
converts the code to use a proper `struct strbuf` to modify the pre- and
postimages, which is both easier and allows for proper lifecycle
management of its buffer.

This series builds on top of ed155187b4 (Sync with Git 2.46.1,
2024-09-13).

Thanks!

Patrick

Patrick Steinhardt (6):
  apply: reorder functions to move image-related things together
  apply: rename functions operating on `struct image`
  apply: introduce macro and function to init images
  apply: refactor code to drop `line_allocated`
  apply: rename members that track line count and allocation length
  apply: refactor `struct image` to use a `struct strbuf`

 apply.c                            | 447 +++++++++++++----------------
 t/t3436-rebase-more-options.sh     |   1 +
 t/t4107-apply-ignore-whitespace.sh |   4 +-
 t/t4124-apply-ws-rule.sh           |   1 +
 t/t4125-apply-ws-fuzz.sh           |   1 +
 t/t4138-apply-ws-expansion.sh      |   1 +
 6 files changed, 204 insertions(+), 251 deletions(-)

-- 
2.46.0.551.gc5ee8f2d1c.dirty

[PATCH 1/6] apply: reorder functions to move image-related things together

[Patrick Steinhardt]

While most of the functions relating to `struct image` are relatively
close to one another, `fuzzy_matchlines()` sits in between those even
though it is rather unrelated.

Reorder functions such that `struct image`-related functions are next to
each other. While at it, move `clear_image()` to the top such that it is
close to the struct definition itself. This makes this lifecycle-related
thing easy to discover.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 apply.c | 106 ++++++++++++++++++++++++++++----------------------------
 1 file changed, 53 insertions(+), 53 deletions(-)

diff --git a/apply.c b/apply.c
index 6e1060a952..9dd2f4d215 100644
--- a/apply.c
+++ b/apply.c
@@ -285,6 +285,13 @@ struct image {
 	struct line *line;
 };
 
+static void clear_image(struct image *image)
+{
+	free(image->buf);
+	free(image->line_allocated);
+	memset(image, 0, sizeof(*image));
+}
+
 static uint32_t hash_line(const char *cp, size_t len)
 {
 	size_t i;
@@ -297,42 +304,6 @@ static uint32_t hash_line(const char *cp, size_t len)
 	return h;
 }
 
-/*
- * Compare lines s1 of length n1 and s2 of length n2, ignoring
- * whitespace difference. Returns 1 if they match, 0 otherwise
- */
-static int fuzzy_matchlines(const char *s1, size_t n1,
-			    const char *s2, size_t n2)
-{
-	const char *end1 = s1 + n1;
-	const char *end2 = s2 + n2;
-
-	/* ignore line endings */
-	while (s1 < end1 && (end1[-1] == '\r' || end1[-1] == '\n'))
-		end1--;
-	while (s2 < end2 && (end2[-1] == '\r' || end2[-1] == '\n'))
-		end2--;
-
-	while (s1 < end1 && s2 < end2) {
-		if (isspace(*s1)) {
-			/*
-			 * Skip whitespace. We check on both buffers
-			 * because we don't want "a b" to match "ab".
-			 */
-			if (!isspace(*s2))
-				return 0;
-			while (s1 < end1 && isspace(*s1))
-				s1++;
-			while (s2 < end2 && isspace(*s2))
-				s2++;
-		} else if (*s1++ != *s2++)
-			return 0;
-	}
-
-	/* If we reached the end on one side only, lines don't match. */
-	return s1 == end1 && s2 == end2;
-}
-
 static void add_line_info(struct image *img, const char *bol, size_t len, unsigned flag)
 {
 	ALLOC_GROW(img->line_allocated, img->nr + 1, img->alloc);
@@ -373,11 +344,17 @@ static void prepare_image(struct image *image, char *buf, size_t len,
 	image->line = image->line_allocated;
 }
 
-static void clear_image(struct image *image)
+static void remove_first_line(struct image *img)
 {
-	free(image->buf);
-	free(image->line_allocated);
-	memset(image, 0, sizeof(*image));
+	img->buf += img->line[0].len;
+	img->len -= img->line[0].len;
+	img->line++;
+	img->nr--;
+}
+
+static void remove_last_line(struct image *img)
+{
+	img->len -= img->line[--img->nr].len;
 }
 
 /* fmt must contain _one_ %s and no other substitution */
@@ -2419,6 +2396,42 @@ static void update_pre_post_images(struct image *preimage,
 	postimage->nr -= reduced;
 }
 
+/*
+ * Compare lines s1 of length n1 and s2 of length n2, ignoring
+ * whitespace difference. Returns 1 if they match, 0 otherwise
+ */
+static int fuzzy_matchlines(const char *s1, size_t n1,
+			    const char *s2, size_t n2)
+{
+	const char *end1 = s1 + n1;
+	const char *end2 = s2 + n2;
+
+	/* ignore line endings */
+	while (s1 < end1 && (end1[-1] == '\r' || end1[-1] == '\n'))
+		end1--;
+	while (s2 < end2 && (end2[-1] == '\r' || end2[-1] == '\n'))
+		end2--;
+
+	while (s1 < end1 && s2 < end2) {
+		if (isspace(*s1)) {
+			/*
+			 * Skip whitespace. We check on both buffers
+			 * because we don't want "a b" to match "ab".
+			 */
+			if (!isspace(*s2))
+				return 0;
+			while (s1 < end1 && isspace(*s1))
+				s1++;
+			while (s2 < end2 && isspace(*s2))
+				s2++;
+		} else if (*s1++ != *s2++)
+			return 0;
+	}
+
+	/* If we reached the end on one side only, lines don't match. */
+	return s1 == end1 && s2 == end2;
+}
+
 static int line_by_line_fuzzy_match(struct image *img,
 				    struct image *preimage,
 				    struct image *postimage,
@@ -2804,19 +2817,6 @@ static int find_pos(struct apply_state *state,
 	return -1;
 }
 
-static void remove_first_line(struct image *img)
-{
-	img->buf += img->line[0].len;
-	img->len -= img->line[0].len;
-	img->line++;
-	img->nr--;
-}
-
-static void remove_last_line(struct image *img)
-{
-	img->len -= img->line[--img->nr].len;
-}
-
 /*
  * The change from "preimage" and "postimage" has been found to
  * apply at applied_pos (counts in line numbers) in "img".
-- 
2.46.0.551.gc5ee8f2d1c.dirty

[PATCH 2/6] apply: rename functions operating on `struct image`

[Patrick Steinhardt]

Rename functions operating on `struct image` to have a `image_` prefix
to match our modern code style.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 apply.c | 61 ++++++++++++++++++++++++++++-----------------------------
 1 file changed, 30 insertions(+), 31 deletions(-)

diff --git a/apply.c b/apply.c
index 9dd2f4d215..ac21c21297 100644
--- a/apply.c
+++ b/apply.c
@@ -285,11 +285,10 @@ struct image {
 	struct line *line;
 };
 
-static void clear_image(struct image *image)
+static void image_clear(struct image *image)
 {
 	free(image->buf);
 	free(image->line_allocated);
-	memset(image, 0, sizeof(*image));
 }
 
 static uint32_t hash_line(const char *cp, size_t len)
@@ -304,7 +303,7 @@ static uint32_t hash_line(const char *cp, size_t len)
 	return h;
 }
 
-static void add_line_info(struct image *img, const char *bol, size_t len, unsigned flag)
+static void image_add_line(struct image *img, const char *bol, size_t len, unsigned flag)
 {
 	ALLOC_GROW(img->line_allocated, img->nr + 1, img->alloc);
 	img->line_allocated[img->nr].len = len;
@@ -318,7 +317,7 @@ static void add_line_info(struct image *img, const char *bol, size_t len, unsign
  * attach it to "image" and add line-based index to it.
  * "image" now owns the "buf".
  */
-static void prepare_image(struct image *image, char *buf, size_t len,
+static void image_prepare(struct image *image, char *buf, size_t len,
 			  int prepare_linetable)
 {
 	const char *cp, *ep;
@@ -338,13 +337,13 @@ static void prepare_image(struct image *image, char *buf, size_t len,
 			;
 		if (next < ep)
 			next++;
-		add_line_info(image, cp, next - cp, 0);
+		image_add_line(image, cp, next - cp, 0);
 		cp = next;
 	}
 	image->line = image->line_allocated;
 }
 
-static void remove_first_line(struct image *img)
+static void image_remove_first_line(struct image *img)
 {
 	img->buf += img->line[0].len;
 	img->len -= img->line[0].len;
@@ -352,7 +351,7 @@ static void remove_first_line(struct image *img)
 	img->nr--;
 }
 
-static void remove_last_line(struct image *img)
+static void image_remove_last_line(struct image *img)
 {
 	img->len -= img->line[--img->nr].len;
 }
@@ -2322,7 +2321,7 @@ static void update_pre_post_images(struct image *preimage,
 	 * are not losing preimage->buf -- apply_one_fragment() will
 	 * free "oldlines".
 	 */
-	prepare_image(&fixed_preimage, buf, len, 1);
+	image_prepare(&fixed_preimage, buf, len, 1);
 	assert(postlen
 	       ? fixed_preimage.nr == preimage->nr
 	       : fixed_preimage.nr <= preimage->nr);
@@ -2874,7 +2873,7 @@ static void update_image(struct apply_state *state,
 	nr = img->nr + postimage->nr - preimage_limit;
 	if (preimage_limit < postimage->nr) {
 		/*
-		 * NOTE: this knows that we never call remove_first_line()
+		 * NOTE: this knows that we never call image_remove_first_line()
 		 * on anything other than pre/post image.
 		 */
 		REALLOC_ARRAY(img->line, nr);
@@ -2957,8 +2956,8 @@ static int apply_one_fragment(struct apply_state *state,
 				break;
 			*old++ = '\n';
 			strbuf_addch(&newlines, '\n');
-			add_line_info(&preimage, "\n", 1, LINE_COMMON);
-			add_line_info(&postimage, "\n", 1, LINE_COMMON);
+			image_add_line(&preimage, "\n", 1, LINE_COMMON);
+			image_add_line(&postimage, "\n", 1, LINE_COMMON);
 			is_blank_context = 1;
 			break;
 		case ' ':
@@ -2968,7 +2967,7 @@ static int apply_one_fragment(struct apply_state *state,
 			/* fallthrough */
 		case '-':
 			memcpy(old, patch + 1, plen);
-			add_line_info(&preimage, old, plen,
+			image_add_line(&preimage, old, plen,
 				      (first == ' ' ? LINE_COMMON : 0));
 			old += plen;
 			if (first == '-')
@@ -2988,7 +2987,7 @@ static int apply_one_fragment(struct apply_state *state,
 			else {
 				ws_fix_copy(&newlines, patch + 1, plen, ws_rule, &state->applied_after_fixing_ws);
 			}
-			add_line_info(&postimage, newlines.buf + start, newlines.len - start,
+			image_add_line(&postimage, newlines.buf + start, newlines.len - start,
 				      (first == '+' ? 0 : LINE_COMMON));
 			if (first == '+' &&
 			    (ws_rule & WS_BLANK_AT_EOF) &&
@@ -3082,14 +3081,14 @@ static int apply_one_fragment(struct apply_state *state,
 		 * just reduce the larger context.
 		 */
 		if (leading >= trailing) {
-			remove_first_line(&preimage);
-			remove_first_line(&postimage);
+			image_remove_first_line(&preimage);
+			image_remove_first_line(&postimage);
 			pos--;
 			leading--;
 		}
 		if (trailing > leading) {
-			remove_last_line(&preimage);
-			remove_last_line(&postimage);
+			image_remove_last_line(&preimage);
+			image_remove_last_line(&postimage);
 			trailing--;
 		}
 	}
@@ -3103,7 +3102,7 @@ static int apply_one_fragment(struct apply_state *state,
 					found_new_blank_lines_at_end);
 			if (state->ws_error_action == correct_ws_error) {
 				while (new_blank_lines_at_end--)
-					remove_last_line(&postimage);
+					image_remove_last_line(&postimage);
 			}
 			/*
 			 * We would want to prevent write_out_results()
@@ -3181,12 +3180,12 @@ static int apply_binary_fragment(struct apply_state *state,
 				  fragment->size, &len);
 		if (!dst)
 			return -1;
-		clear_image(img);
+		image_clear(img);
 		img->buf = dst;
 		img->len = len;
 		return 0;
 	case BINARY_LITERAL_DEFLATED:
-		clear_image(img);
+		image_clear(img);
 		img->len = fragment->size;
 		img->buf = xmemdupz(fragment->patch, img->len);
 		return 0;
@@ -3241,7 +3240,7 @@ static int apply_binary(struct apply_state *state,
 
 	get_oid_hex(patch->new_oid_prefix, &oid);
 	if (is_null_oid(&oid)) {
-		clear_image(img);
+		image_clear(img);
 		return 0; /* deletion patch */
 	}
 
@@ -3257,7 +3256,7 @@ static int apply_binary(struct apply_state *state,
 			return error(_("the necessary postimage %s for "
 				       "'%s' cannot be read"),
 				     patch->new_oid_prefix, name);
-		clear_image(img);
+		image_clear(img);
 		img->buf = result;
 		img->len = size;
 	} else {
@@ -3533,7 +3532,7 @@ static int load_preimage(struct apply_state *state,
 	}
 
 	img = strbuf_detach(&buf, &len);
-	prepare_image(image, img, len, !patch->is_binary);
+	image_prepare(image, img, len, !patch->is_binary);
 	return 0;
 }
 
@@ -3542,7 +3541,7 @@ static int resolve_to(struct image *image, const struct object_id *result_id)
 	unsigned long size;
 	enum object_type type;
 
-	clear_image(image);
+	image_clear(image);
 
 	image->buf = repo_read_object_file(the_repository, result_id, &type,
 					   &size);
@@ -3589,7 +3588,7 @@ static int three_way_merge(struct apply_state *state,
 		free(result.ptr);
 		return -1;
 	}
-	clear_image(image);
+	image_clear(image);
 	image->buf = result.ptr;
 	image->len = result.size;
 
@@ -3636,7 +3635,7 @@ static int load_current(struct apply_state *state,
 	else if (status)
 		return -1;
 	img = strbuf_detach(&buf, &len);
-	prepare_image(image, img, len, !patch->is_binary);
+	image_prepare(image, img, len, !patch->is_binary);
 	return 0;
 }
 
@@ -3671,15 +3670,15 @@ static int try_threeway(struct apply_state *state,
 		fprintf(stderr, _("Performing three-way merge...\n"));
 
 	img = strbuf_detach(&buf, &len);
-	prepare_image(&tmp_image, img, len, 1);
+	image_prepare(&tmp_image, img, len, 1);
 	/* Apply the patch to get the post image */
 	if (apply_fragments(state, &tmp_image, patch) < 0) {
-		clear_image(&tmp_image);
+		image_clear(&tmp_image);
 		return -1;
 	}
 	/* post_oid is theirs */
 	write_object_file(tmp_image.buf, tmp_image.len, OBJ_BLOB, &post_oid);
-	clear_image(&tmp_image);
+	image_clear(&tmp_image);
 
 	/* our_oid is ours */
 	if (patch->is_new) {
@@ -3692,7 +3691,7 @@ static int try_threeway(struct apply_state *state,
 				     patch->old_name);
 	}
 	write_object_file(tmp_image.buf, tmp_image.len, OBJ_BLOB, &our_oid);
-	clear_image(&tmp_image);
+	image_clear(&tmp_image);
 
 	/* in-core three-way merge between post and our using pre as base */
 	status = three_way_merge(state, image, patch->new_name,
@@ -3740,7 +3739,7 @@ static int apply_data(struct apply_state *state, struct patch *patch,
 
 		/* Note: with --reject, apply_fragments() returns 0 */
 		if (patch->direct_to_threeway || apply_fragments(state, &image, patch) < 0) {
-			clear_image(&image);
+			image_clear(&image);
 			return -1;
 		}
 	}
-- 
2.46.0.551.gc5ee8f2d1c.dirty

[PATCH 3/6] apply: introduce macro and function to init images

[Patrick Steinhardt]

We're about to convert the `struct image` to gain a `struct strbuf`
member, which requires more careful initialization than just memsetting
it to zeros. Introduce the `IMAGE_INIT` macro and `image_init()`
function to prepare for this change.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 apply.c | 22 ++++++++++++++--------
 1 file changed, 14 insertions(+), 8 deletions(-)

diff --git a/apply.c b/apply.c
index ac21c21297..76f7777d4c 100644
--- a/apply.c
+++ b/apply.c
@@ -284,11 +284,19 @@ struct image {
 	struct line *line_allocated;
 	struct line *line;
 };
+#define IMAGE_INIT { 0 }
+
+static void image_init(struct image *image)
+{
+	struct image empty = IMAGE_INIT;
+	memcpy(image, &empty, sizeof(*image));
+}
 
 static void image_clear(struct image *image)
 {
 	free(image->buf);
 	free(image->line_allocated);
+	image_init(image);
 }
 
 static uint32_t hash_line(const char *cp, size_t len)
@@ -322,7 +330,7 @@ static void image_prepare(struct image *image, char *buf, size_t len,
 {
 	const char *cp, *ep;
 
-	memset(image, 0, sizeof(*image));
+	image_clear(image);
 	image->buf = buf;
 	image->len = len;
 
@@ -2314,7 +2322,7 @@ static void update_pre_post_images(struct image *preimage,
 {
 	int i, ctx, reduced;
 	char *new_buf, *old_buf, *fixed;
-	struct image fixed_preimage;
+	struct image fixed_preimage = IMAGE_INIT;
 
 	/*
 	 * Update the preimage with whitespace fixes.  Note that we
@@ -2910,11 +2918,9 @@ static int apply_one_fragment(struct apply_state *state,
 	int hunk_linenr = frag->linenr;
 	unsigned long leading, trailing;
 	int pos, applied_pos;
-	struct image preimage;
-	struct image postimage;
+	struct image preimage = IMAGE_INIT;
+	struct image postimage = IMAGE_INIT;
 
-	memset(&preimage, 0, sizeof(preimage));
-	memset(&postimage, 0, sizeof(postimage));
 	oldlines = xmalloc(size);
 	strbuf_init(&newlines, size);
 
@@ -3650,7 +3656,7 @@ static int try_threeway(struct apply_state *state,
 	size_t len;
 	int status;
 	char *img;
-	struct image tmp_image;
+	struct image tmp_image = IMAGE_INIT;
 
 	/* No point falling back to 3-way merge in these cases */
 	if (patch->is_delete ||
@@ -3727,7 +3733,7 @@ static int try_threeway(struct apply_state *state,
 static int apply_data(struct apply_state *state, struct patch *patch,
 		      struct stat *st, const struct cache_entry *ce)
 {
-	struct image image;
+	struct image image = IMAGE_INIT;
 
 	if (load_preimage(state, &image, patch, st, ce) < 0)
 		return -1;
-- 
2.46.0.551.gc5ee8f2d1c.dirty

[PATCH 4/6] apply: refactor code to drop `line_allocated`

[Patrick Steinhardt]

The `struct image` has two members `line` and `line_allocated`. The
former member is the one that should be used throughougt the code,
whereas the latter one is used to track whether the lines have been
allocated or not.

In practice, the array of lines is always allocated. The reason why we
have `line_allocated` is that `remove_first_line()` will advance the
array pointer to drop the first entry, and thus it point into the array
instead of to the array header.

Refactor the function to use memmove(3P) instead, which allows us to get
rid of this double bookkeeping. We call this function at most once per
image anyway, so this shouldn't cause any performance regressions.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 apply.c | 33 ++++++++++++++-------------------
 1 file changed, 14 insertions(+), 19 deletions(-)

diff --git a/apply.c b/apply.c
index 76f7777d4c..985564ac76 100644
--- a/apply.c
+++ b/apply.c
@@ -281,7 +281,6 @@ struct image {
 	size_t len;
 	size_t nr;
 	size_t alloc;
-	struct line *line_allocated;
 	struct line *line;
 };
 #define IMAGE_INIT { 0 }
@@ -295,7 +294,7 @@ static void image_init(struct image *image)
 static void image_clear(struct image *image)
 {
 	free(image->buf);
-	free(image->line_allocated);
+	free(image->line);
 	image_init(image);
 }
 
@@ -313,10 +312,10 @@ static uint32_t hash_line(const char *cp, size_t len)
 
 static void image_add_line(struct image *img, const char *bol, size_t len, unsigned flag)
 {
-	ALLOC_GROW(img->line_allocated, img->nr + 1, img->alloc);
-	img->line_allocated[img->nr].len = len;
-	img->line_allocated[img->nr].hash = hash_line(bol, len);
-	img->line_allocated[img->nr].flag = flag;
+	ALLOC_GROW(img->line, img->nr + 1, img->alloc);
+	img->line[img->nr].len = len;
+	img->line[img->nr].hash = hash_line(bol, len);
+	img->line[img->nr].flag = flag;
 	img->nr++;
 }
 
@@ -348,15 +347,15 @@ static void image_prepare(struct image *image, char *buf, size_t len,
 		image_add_line(image, cp, next - cp, 0);
 		cp = next;
 	}
-	image->line = image->line_allocated;
 }
 
 static void image_remove_first_line(struct image *img)
 {
 	img->buf += img->line[0].len;
 	img->len -= img->line[0].len;
-	img->line++;
 	img->nr--;
+	if (img->nr)
+		MOVE_ARRAY(img->line, img->line + 1, img->nr);
 }
 
 static void image_remove_last_line(struct image *img)
@@ -2335,7 +2334,7 @@ static void update_pre_post_images(struct image *preimage,
 	       : fixed_preimage.nr <= preimage->nr);
 	for (i = 0; i < fixed_preimage.nr; i++)
 		fixed_preimage.line[i].flag = preimage->line[i].flag;
-	free(preimage->line_allocated);
+	free(preimage->line);
 	*preimage = fixed_preimage;
 
 	/*
@@ -2879,14 +2878,12 @@ static void update_image(struct apply_state *state,
 
 	/* Adjust the line table */
 	nr = img->nr + postimage->nr - preimage_limit;
-	if (preimage_limit < postimage->nr) {
+	if (preimage_limit < postimage->nr)
 		/*
 		 * NOTE: this knows that we never call image_remove_first_line()
 		 * on anything other than pre/post image.
 		 */
 		REALLOC_ARRAY(img->line, nr);
-		img->line_allocated = img->line;
-	}
 	if (preimage_limit != postimage->nr)
 		MOVE_ARRAY(img->line + applied_pos + postimage->nr,
 			   img->line + applied_pos + preimage_limit,
@@ -3027,8 +3024,8 @@ static int apply_one_fragment(struct apply_state *state,
 	    newlines.len > 0 && newlines.buf[newlines.len - 1] == '\n') {
 		old--;
 		strbuf_setlen(&newlines, newlines.len - 1);
-		preimage.line_allocated[preimage.nr - 1].len--;
-		postimage.line_allocated[postimage.nr - 1].len--;
+		preimage.line[preimage.nr - 1].len--;
+		postimage.line[postimage.nr - 1].len--;
 	}
 
 	leading = frag->leading;
@@ -3062,8 +3059,6 @@ static int apply_one_fragment(struct apply_state *state,
 	preimage.len = old - oldlines;
 	postimage.buf = newlines.buf;
 	postimage.len = newlines.len;
-	preimage.line = preimage.line_allocated;
-	postimage.line = postimage.line_allocated;
 
 	for (;;) {
 
@@ -3151,8 +3146,8 @@ static int apply_one_fragment(struct apply_state *state,
 out:
 	free(oldlines);
 	strbuf_release(&newlines);
-	free(preimage.line_allocated);
-	free(postimage.line_allocated);
+	free(preimage.line);
+	free(postimage.line);
 
 	return (applied_pos < 0);
 }
@@ -3752,7 +3747,7 @@ static int apply_data(struct apply_state *state, struct patch *patch,
 	patch->result = image.buf;
 	patch->resultsize = image.len;
 	add_to_fn_table(state, patch);
-	free(image.line_allocated);
+	free(image.line);
 
 	if (0 < patch->is_delete && patch->resultsize)
 		return error(_("removal patch leaves file contents"));
-- 
2.46.0.551.gc5ee8f2d1c.dirty

Re: [PATCH 4/6] apply: refactor code to drop `line_allocated`

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> Refactor the function to use memmove(3P) instead, which allows us to get
> rid of this double bookkeeping. We call this function at most once per
> image anyway, so this shouldn't cause any performance regressions.

Don't we call remove_first_line() as long as leading is larger than
trailing repeatedly?  Is "at most once" accurate?

As to the correctness, I think nobody takes the address of an
element in the line[] array and expects the address to stay valid
across a call to remove_first_line(), so this should be safe.

Thanks.

> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  apply.c | 33 ++++++++++++++-------------------
>  1 file changed, 14 insertions(+), 19 deletions(-)

Re: [PATCH 4/6] apply: refactor code to drop `line_allocated`

[Patrick Steinhardt]

On Mon, Sep 16, 2024 at 11:56:16AM -0700, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> 
> > Refactor the function to use memmove(3P) instead, which allows us to get
> > rid of this double bookkeeping. We call this function at most once per
> > image anyway, so this shouldn't cause any performance regressions.
> 
> Don't we call remove_first_line() as long as leading is larger than
> trailing repeatedly?  Is "at most once" accurate?
> 
> As to the correctness, I think nobody takes the address of an
> element in the line[] array and expects the address to stay valid
> across a call to remove_first_line(), so this should be safe.

Oh, you're right. I did search for a loop surrounding
`image_remove_first_line()`, but somehow I completely missed the obvious
`for (;;)` loop around it. No idea how I was able to miss it.

I still very much doubt that this will cause performance issues in
practice (even though it's only by gut feeling), but the statement is
obviously incorrect. In case the assumption ever turns out to be wrong
we can likely refactor the loop to only trim contents after we have
found how many lines to remove, at which point we can remove them with a
single call to `strbuf_remove()`.

Patrick

Re: [PATCH 4/6] apply: refactor code to drop `line_allocated`

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> former member is the one that should be used throughougt the code,

"throughout" (I'll amend while queuing).

[PATCH 5/6] apply: rename members that track line count and allocation length

[Patrick Steinhardt]

The `struct image` has two members `nr` and `alloc` that track the
number of lines as well as how large its array is. It is somewhat easy
to confuse these members with `len` though, which tracks the length of
the `buf` member.

Rename these members to `line_nr` and `line_alloc` respectively to avoid
confusion. This is in line with how we typically name variables that
track an array in this way.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 apply.c | 91 ++++++++++++++++++++++++++++-----------------------------
 1 file changed, 45 insertions(+), 46 deletions(-)

diff --git a/apply.c b/apply.c
index 985564ac76..3340bb1fc0 100644
--- a/apply.c
+++ b/apply.c
@@ -279,9 +279,8 @@ struct line {
 struct image {
 	char *buf;
 	size_t len;
-	size_t nr;
-	size_t alloc;
 	struct line *line;
+	size_t line_nr, line_alloc;
 };
 #define IMAGE_INIT { 0 }
 
@@ -312,11 +311,11 @@ static uint32_t hash_line(const char *cp, size_t len)
 
 static void image_add_line(struct image *img, const char *bol, size_t len, unsigned flag)
 {
-	ALLOC_GROW(img->line, img->nr + 1, img->alloc);
-	img->line[img->nr].len = len;
-	img->line[img->nr].hash = hash_line(bol, len);
-	img->line[img->nr].flag = flag;
-	img->nr++;
+	ALLOC_GROW(img->line, img->line_nr + 1, img->line_alloc);
+	img->line[img->line_nr].len = len;
+	img->line[img->line_nr].hash = hash_line(bol, len);
+	img->line[img->line_nr].flag = flag;
+	img->line_nr++;
 }
 
 /*
@@ -353,14 +352,14 @@ static void image_remove_first_line(struct image *img)
 {
 	img->buf += img->line[0].len;
 	img->len -= img->line[0].len;
-	img->nr--;
-	if (img->nr)
-		MOVE_ARRAY(img->line, img->line + 1, img->nr);
+	img->line_nr--;
+	if (img->line_nr)
+		MOVE_ARRAY(img->line, img->line + 1, img->line_nr);
 }
 
 static void image_remove_last_line(struct image *img)
 {
-	img->len -= img->line[--img->nr].len;
+	img->len -= img->line[--img->line_nr].len;
 }
 
 /* fmt must contain _one_ %s and no other substitution */
@@ -2330,9 +2329,9 @@ static void update_pre_post_images(struct image *preimage,
 	 */
 	image_prepare(&fixed_preimage, buf, len, 1);
 	assert(postlen
-	       ? fixed_preimage.nr == preimage->nr
-	       : fixed_preimage.nr <= preimage->nr);
-	for (i = 0; i < fixed_preimage.nr; i++)
+	       ? fixed_preimage.line_nr == preimage->line_nr
+	       : fixed_preimage.line_nr <= preimage->line_nr);
+	for (i = 0; i < fixed_preimage.line_nr; i++)
 		fixed_preimage.line[i].flag = preimage->line[i].flag;
 	free(preimage->line);
 	*preimage = fixed_preimage;
@@ -2353,7 +2352,7 @@ static void update_pre_post_images(struct image *preimage,
 		new_buf = old_buf;
 	fixed = preimage->buf;
 
-	for (i = reduced = ctx = 0; i < postimage->nr; i++) {
+	for (i = reduced = ctx = 0; i < postimage->line_nr; i++) {
 		size_t l_len = postimage->line[i].len;
 		if (!(postimage->line[i].flag & LINE_COMMON)) {
 			/* an added line -- no counterparts in preimage */
@@ -2367,7 +2366,7 @@ static void update_pre_post_images(struct image *preimage,
 		old_buf += l_len;
 
 		/* and find the corresponding one in the fixed preimage */
-		while (ctx < preimage->nr &&
+		while (ctx < preimage->line_nr &&
 		       !(preimage->line[ctx].flag & LINE_COMMON)) {
 			fixed += preimage->line[ctx].len;
 			ctx++;
@@ -2377,7 +2376,7 @@ static void update_pre_post_images(struct image *preimage,
 		 * preimage is expected to run out, if the caller
 		 * fixed addition of trailing blank lines.
 		 */
-		if (preimage->nr <= ctx) {
+		if (preimage->line_nr <= ctx) {
 			reduced++;
 			continue;
 		}
@@ -2399,7 +2398,7 @@ static void update_pre_post_images(struct image *preimage,
 
 	/* Fix the length of the whole thing */
 	postimage->len = new_buf - postimage->buf;
-	postimage->nr -= reduced;
+	postimage->line_nr -= reduced;
 }
 
 /*
@@ -2482,7 +2481,7 @@ static int line_by_line_fuzzy_match(struct image *img,
 	 * we are removing blank lines at the end of the file.)
 	 */
 	buf = preimage_eof = preimage->buf + preoff;
-	for ( ; i < preimage->nr; i++)
+	for ( ; i < preimage->line_nr; i++)
 		preoff += preimage->line[i].len;
 	preimage_end = preimage->buf + preoff;
 	for ( ; buf < preimage_end; buf++)
@@ -2522,12 +2521,12 @@ static int match_fragment(struct apply_state *state,
 	int preimage_limit;
 	int ret;
 
-	if (preimage->nr + current_lno <= img->nr) {
+	if (preimage->line_nr + current_lno <= img->line_nr) {
 		/*
 		 * The hunk falls within the boundaries of img.
 		 */
-		preimage_limit = preimage->nr;
-		if (match_end && (preimage->nr + current_lno != img->nr)) {
+		preimage_limit = preimage->line_nr;
+		if (match_end && (preimage->line_nr + current_lno != img->line_nr)) {
 			ret = 0;
 			goto out;
 		}
@@ -2540,7 +2539,7 @@ static int match_fragment(struct apply_state *state,
 		 * match with img, and the remainder of the preimage
 		 * must be blank.
 		 */
-		preimage_limit = img->nr - current_lno;
+		preimage_limit = img->line_nr - current_lno;
 	} else {
 		/*
 		 * The hunk extends beyond the end of the img and
@@ -2565,7 +2564,7 @@ static int match_fragment(struct apply_state *state,
 		}
 	}
 
-	if (preimage_limit == preimage->nr) {
+	if (preimage_limit == preimage->line_nr) {
 		/*
 		 * Do we have an exact match?  If we were told to match
 		 * at the end, size must be exactly at current+fragsize,
@@ -2637,7 +2636,7 @@ static int match_fragment(struct apply_state *state,
 
 	/* First count added lines in postimage */
 	postlen = 0;
-	for (i = 0; i < postimage->nr; i++) {
+	for (i = 0; i < postimage->line_nr; i++) {
 		if (!(postimage->line[i].flag & LINE_COMMON))
 			postlen += postimage->line[i].len;
 	}
@@ -2699,7 +2698,7 @@ static int match_fragment(struct apply_state *state,
 	 * empty or only contain whitespace (if WS_BLANK_AT_EOL is
 	 * false).
 	 */
-	for ( ; i < preimage->nr; i++) {
+	for ( ; i < preimage->line_nr; i++) {
 		size_t fixstart = fixed.len; /* start of the fixed preimage */
 		size_t oldlen = preimage->line[i].len;
 		int j;
@@ -2754,7 +2753,7 @@ static int find_pos(struct apply_state *state,
 	 * than `match_beginning`.
 	 */
 	if (state->allow_overlap && match_beginning && match_end &&
-	    img->nr - preimage->nr != 0)
+	    img->line_nr - preimage->line_nr != 0)
 		match_beginning = 0;
 
 	/*
@@ -2765,15 +2764,15 @@ static int find_pos(struct apply_state *state,
 	if (match_beginning)
 		line = 0;
 	else if (match_end)
-		line = img->nr - preimage->nr;
+		line = img->line_nr - preimage->line_nr;
 
 	/*
 	 * Because the comparison is unsigned, the following test
 	 * will also take care of a negative line number that can
 	 * result when match_end and preimage is larger than the target.
 	 */
-	if ((size_t) line > img->nr)
-		line = img->nr;
+	if ((size_t) line > img->line_nr)
+		line = img->line_nr;
 
 	current = 0;
 	for (i = 0; i < line; i++)
@@ -2796,7 +2795,7 @@ static int find_pos(struct apply_state *state,
 			return current_lno;
 
 	again:
-		if (backwards_lno == 0 && forwards_lno == img->nr)
+		if (backwards_lno == 0 && forwards_lno == img->line_nr)
 			break;
 
 		if (i & 1) {
@@ -2809,7 +2808,7 @@ static int find_pos(struct apply_state *state,
 			current = backwards;
 			current_lno = backwards_lno;
 		} else {
-			if (forwards_lno == img->nr) {
+			if (forwards_lno == img->line_nr) {
 				i++;
 				goto again;
 			}
@@ -2852,9 +2851,9 @@ static void update_image(struct apply_state *state,
 	 * to the number of lines in the preimage that falls
 	 * within the boundaries.
 	 */
-	preimage_limit = preimage->nr;
-	if (preimage_limit > img->nr - applied_pos)
-		preimage_limit = img->nr - applied_pos;
+	preimage_limit = preimage->line_nr;
+	if (preimage_limit > img->line_nr - applied_pos)
+		preimage_limit = img->line_nr - applied_pos;
 
 	for (i = 0; i < applied_pos; i++)
 		applied_at += img->line[i].len;
@@ -2877,22 +2876,22 @@ static void update_image(struct apply_state *state,
 	result[img->len] = '\0';
 
 	/* Adjust the line table */
-	nr = img->nr + postimage->nr - preimage_limit;
-	if (preimage_limit < postimage->nr)
+	nr = img->line_nr + postimage->line_nr - preimage_limit;
+	if (preimage_limit < postimage->line_nr)
 		/*
 		 * NOTE: this knows that we never call image_remove_first_line()
 		 * on anything other than pre/post image.
 		 */
 		REALLOC_ARRAY(img->line, nr);
-	if (preimage_limit != postimage->nr)
-		MOVE_ARRAY(img->line + applied_pos + postimage->nr,
+	if (preimage_limit != postimage->line_nr)
+		MOVE_ARRAY(img->line + applied_pos + postimage->line_nr,
 			   img->line + applied_pos + preimage_limit,
-			   img->nr - (applied_pos + preimage_limit));
-	COPY_ARRAY(img->line + applied_pos, postimage->line, postimage->nr);
+			   img->line_nr - (applied_pos + preimage_limit));
+	COPY_ARRAY(img->line + applied_pos, postimage->line, postimage->line_nr);
 	if (!state->allow_overlap)
-		for (i = 0; i < postimage->nr; i++)
+		for (i = 0; i < postimage->line_nr; i++)
 			img->line[applied_pos + i].flag |= LINE_PATCHED;
-	img->nr = nr;
+	img->line_nr = nr;
 }
 
 /*
@@ -3024,8 +3023,8 @@ static int apply_one_fragment(struct apply_state *state,
 	    newlines.len > 0 && newlines.buf[newlines.len - 1] == '\n') {
 		old--;
 		strbuf_setlen(&newlines, newlines.len - 1);
-		preimage.line[preimage.nr - 1].len--;
-		postimage.line[postimage.nr - 1].len--;
+		preimage.line[preimage.line_nr - 1].len--;
+		postimage.line[postimage.line_nr - 1].len--;
 	}
 
 	leading = frag->leading;
@@ -3096,7 +3095,7 @@ static int apply_one_fragment(struct apply_state *state,
 
 	if (applied_pos >= 0) {
 		if (new_blank_lines_at_end &&
-		    preimage.nr + applied_pos >= img->nr &&
+		    preimage.line_nr + applied_pos >= img->line_nr &&
 		    (ws_rule & WS_BLANK_AT_EOF) &&
 		    state->ws_error_action != nowarn_ws_error) {
 			record_ws_error(state, WS_BLANK_AT_EOF, "+", 1,
-- 
2.46.0.551.gc5ee8f2d1c.dirty

[PATCH 6/6] apply: refactor `struct image` to use a `struct strbuf`

[Patrick Steinhardt]

The `struct image` uses a character array to track the pre- or postimage
of a patch operation. This has multiple downsides:

  - It is somewhat hard to track memory ownership. In fact, we have
    several memory leaks in git-apply(1) because we do not (and cannot
    easily) free the buffer in all situations.

  - We have to reinvent the wheel and manually implement a lot of
    functionality that would already be provided by `struct strbuf`.

  - We have to carefully track whether `update_pre_post_images()` can do
    an in-place update of the postimage or whether it has to allocate a
    new buffer for it.

This is all rather cumbersome, and especially `update_pre_post_images()`
is really hard to understand as a consequence even though what it is
doing is rather trivial.

Refactor the code to use a `struct strbuf` instead, addressing all of
the above. Like this we can easily perform in-place updates in all
situations, the logic to perform those updates becomes way simpler and
the lifetime of the buffer becomes a ton easier to track.

This refactoring also plugs some leaking buffers as a side effect.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 apply.c                            | 192 +++++++++++------------------
 t/t3436-rebase-more-options.sh     |   1 +
 t/t4107-apply-ignore-whitespace.sh |   4 +-
 t/t4124-apply-ws-rule.sh           |   1 +
 t/t4125-apply-ws-fuzz.sh           |   1 +
 t/t4138-apply-ws-expansion.sh      |   1 +
 6 files changed, 77 insertions(+), 123 deletions(-)

diff --git a/apply.c b/apply.c
index 3340bb1fc0..d283dd4f5e 100644
--- a/apply.c
+++ b/apply.c
@@ -277,12 +277,13 @@ struct line {
  * This represents a "file", which is an array of "lines".
  */
 struct image {
-	char *buf;
-	size_t len;
+	struct strbuf buf;
 	struct line *line;
 	size_t line_nr, line_alloc;
 };
-#define IMAGE_INIT { 0 }
+#define IMAGE_INIT { \
+	.buf = STRBUF_INIT, \
+}
 
 static void image_init(struct image *image)
 {
@@ -292,7 +293,7 @@ static void image_init(struct image *image)
 
 static void image_clear(struct image *image)
 {
-	free(image->buf);
+	strbuf_release(&image->buf);
 	free(image->line);
 	image_init(image);
 }
@@ -329,14 +330,13 @@ static void image_prepare(struct image *image, char *buf, size_t len,
 	const char *cp, *ep;
 
 	image_clear(image);
-	image->buf = buf;
-	image->len = len;
+	strbuf_attach(&image->buf, buf, len, len + 1);
 
 	if (!prepare_linetable)
 		return;
 
-	ep = image->buf + image->len;
-	cp = image->buf;
+	ep = image->buf.buf + image->buf.len;
+	cp = image->buf.buf;
 	while (cp < ep) {
 		const char *next;
 		for (next = cp; next < ep && *next != '\n'; next++)
@@ -350,8 +350,7 @@ static void image_prepare(struct image *image, char *buf, size_t len,
 
 static void image_remove_first_line(struct image *img)
 {
-	img->buf += img->line[0].len;
-	img->len -= img->line[0].len;
+	strbuf_remove(&img->buf, 0, img->line[0].len);
 	img->line_nr--;
 	if (img->line_nr)
 		MOVE_ARRAY(img->line, img->line + 1, img->line_nr);
@@ -359,7 +358,7 @@ static void image_remove_first_line(struct image *img)
 
 static void image_remove_last_line(struct image *img)
 {
-	img->len -= img->line[--img->line_nr].len;
+	strbuf_setlen(&img->buf, img->buf.len - img->line[--img->line_nr].len);
 }
 
 /* fmt must contain _one_ %s and no other substitution */
@@ -2308,19 +2307,16 @@ static int read_old_data(struct stat *st, struct patch *patch,
 
 /*
  * Update the preimage, and the common lines in postimage,
- * from buffer buf of length len. If postlen is 0 the postimage
- * is updated in place, otherwise it's updated on a new buffer
- * of length postlen
+ * from buffer buf of length len.
  */
-
 static void update_pre_post_images(struct image *preimage,
 				   struct image *postimage,
-				   char *buf,
-				   size_t len, size_t postlen)
+				   char *buf, size_t len)
 {
-	int i, ctx, reduced;
-	char *new_buf, *old_buf, *fixed;
 	struct image fixed_preimage = IMAGE_INIT;
+	size_t insert_pos = 0;
+	int i, ctx, reduced;
+	const char *fixed;
 
 	/*
 	 * Update the preimage with whitespace fixes.  Note that we
@@ -2328,43 +2324,24 @@ static void update_pre_post_images(struct image *preimage,
 	 * free "oldlines".
 	 */
 	image_prepare(&fixed_preimage, buf, len, 1);
-	assert(postlen
-	       ? fixed_preimage.line_nr == preimage->line_nr
-	       : fixed_preimage.line_nr <= preimage->line_nr);
 	for (i = 0; i < fixed_preimage.line_nr; i++)
 		fixed_preimage.line[i].flag = preimage->line[i].flag;
-	free(preimage->line);
+	image_clear(preimage);
 	*preimage = fixed_preimage;
+	fixed = preimage->buf.buf;
 
 	/*
-	 * Adjust the common context lines in postimage. This can be
-	 * done in-place when we are shrinking it with whitespace
-	 * fixing, but needs a new buffer when ignoring whitespace or
-	 * expanding leading tabs to spaces.
-	 *
-	 * We trust the caller to tell us if the update can be done
-	 * in place (postlen==0) or not.
+	 * Adjust the common context lines in postimage.
 	 */
-	old_buf = postimage->buf;
-	if (postlen)
-		new_buf = postimage->buf = xmalloc(postlen);
-	else
-		new_buf = old_buf;
-	fixed = preimage->buf;
-
 	for (i = reduced = ctx = 0; i < postimage->line_nr; i++) {
 		size_t l_len = postimage->line[i].len;
+
 		if (!(postimage->line[i].flag & LINE_COMMON)) {
 			/* an added line -- no counterparts in preimage */
-			memmove(new_buf, old_buf, l_len);
-			old_buf += l_len;
-			new_buf += l_len;
+			insert_pos += l_len;
 			continue;
 		}
 
-		/* a common context -- skip it in the original postimage */
-		old_buf += l_len;
-
 		/* and find the corresponding one in the fixed preimage */
 		while (ctx < preimage->line_nr &&
 		       !(preimage->line[ctx].flag & LINE_COMMON)) {
@@ -2383,21 +2360,15 @@ static void update_pre_post_images(struct image *preimage,
 
 		/* and copy it in, while fixing the line length */
 		l_len = preimage->line[ctx].len;
-		memcpy(new_buf, fixed, l_len);
-		new_buf += l_len;
+		strbuf_splice(&postimage->buf, insert_pos, postimage->line[i].len,
+			      fixed, l_len);
+		insert_pos += l_len;
 		fixed += l_len;
 		postimage->line[i].len = l_len;
 		ctx++;
 	}
 
-	if (postlen
-	    ? postlen < new_buf - postimage->buf
-	    : postimage->len < new_buf - postimage->buf)
-		BUG("caller miscounted postlen: asked %d, orig = %d, used = %d",
-		    (int)postlen, (int) postimage->len, (int)(new_buf - postimage->buf));
-
 	/* Fix the length of the whole thing */
-	postimage->len = new_buf - postimage->buf;
 	postimage->line_nr -= reduced;
 }
 
@@ -2447,7 +2418,6 @@ static int line_by_line_fuzzy_match(struct image *img,
 	int i;
 	size_t imgoff = 0;
 	size_t preoff = 0;
-	size_t postlen = postimage->len;
 	size_t extra_chars;
 	char *buf;
 	char *preimage_eof;
@@ -2460,11 +2430,9 @@ static int line_by_line_fuzzy_match(struct image *img,
 		size_t prelen = preimage->line[i].len;
 		size_t imglen = img->line[current_lno+i].len;
 
-		if (!fuzzy_matchlines(img->buf + current + imgoff, imglen,
-				      preimage->buf + preoff, prelen))
+		if (!fuzzy_matchlines(img->buf.buf + current + imgoff, imglen,
+				      preimage->buf.buf + preoff, prelen))
 			return 0;
-		if (preimage->line[i].flag & LINE_COMMON)
-			postlen += imglen - prelen;
 		imgoff += imglen;
 		preoff += prelen;
 	}
@@ -2480,10 +2448,10 @@ static int line_by_line_fuzzy_match(struct image *img,
 	 * are whitespace characters. (This can only happen if
 	 * we are removing blank lines at the end of the file.)
 	 */
-	buf = preimage_eof = preimage->buf + preoff;
+	buf = preimage_eof = preimage->buf.buf + preoff;
 	for ( ; i < preimage->line_nr; i++)
 		preoff += preimage->line[i].len;
-	preimage_end = preimage->buf + preoff;
+	preimage_end = preimage->buf.buf + preoff;
 	for ( ; buf < preimage_end; buf++)
 		if (!isspace(*buf))
 			return 0;
@@ -2497,11 +2465,11 @@ static int line_by_line_fuzzy_match(struct image *img,
 	 */
 	extra_chars = preimage_end - preimage_eof;
 	strbuf_init(&fixed, imgoff + extra_chars);
-	strbuf_add(&fixed, img->buf + current, imgoff);
+	strbuf_add(&fixed, img->buf.buf + current, imgoff);
 	strbuf_add(&fixed, preimage_eof, extra_chars);
 	fixed_buf = strbuf_detach(&fixed, &fixed_len);
 	update_pre_post_images(preimage, postimage,
-			       fixed_buf, fixed_len, postlen);
+			       fixed_buf, fixed_len);
 	return 1;
 }
 
@@ -2517,7 +2485,8 @@ static int match_fragment(struct apply_state *state,
 	int i;
 	const char *orig, *target;
 	struct strbuf fixed = STRBUF_INIT;
-	size_t postlen;
+	char *fixed_buf;
+	size_t fixed_len;
 	int preimage_limit;
 	int ret;
 
@@ -2573,9 +2542,9 @@ static int match_fragment(struct apply_state *state,
 		 * exactly.
 		 */
 		if ((match_end
-		     ? (current + preimage->len == img->len)
-		     : (current + preimage->len <= img->len)) &&
-		    !memcmp(img->buf + current, preimage->buf, preimage->len)) {
+		     ? (current + preimage->buf.len == img->buf.len)
+		     : (current + preimage->buf.len <= img->buf.len)) &&
+		    !memcmp(img->buf.buf + current, preimage->buf.buf, preimage->buf.len)) {
 			ret = 1;
 			goto out;
 		}
@@ -2589,7 +2558,7 @@ static int match_fragment(struct apply_state *state,
 		 */
 		const char *buf, *buf_end;
 
-		buf = preimage->buf;
+		buf = preimage->buf.buf;
 		buf_end = buf;
 		for (i = 0; i < preimage_limit; i++)
 			buf_end += preimage->line[i].len;
@@ -2634,21 +2603,14 @@ static int match_fragment(struct apply_state *state,
 	 * fixed.
 	 */
 
-	/* First count added lines in postimage */
-	postlen = 0;
-	for (i = 0; i < postimage->line_nr; i++) {
-		if (!(postimage->line[i].flag & LINE_COMMON))
-			postlen += postimage->line[i].len;
-	}
-
 	/*
 	 * The preimage may extend beyond the end of the file,
 	 * but in this loop we will only handle the part of the
 	 * preimage that falls within the file.
 	 */
-	strbuf_grow(&fixed, preimage->len + 1);
-	orig = preimage->buf;
-	target = img->buf + current;
+	strbuf_grow(&fixed, preimage->buf.len + 1);
+	orig = preimage->buf.buf;
+	target = img->buf.buf + current;
 	for (i = 0; i < preimage_limit; i++) {
 		size_t oldlen = preimage->line[i].len;
 		size_t tgtlen = img->line[current_lno + i].len;
@@ -2677,10 +2639,6 @@ static int match_fragment(struct apply_state *state,
 			 !memcmp(tgtfix.buf, fixed.buf + fixstart,
 					     fixed.len - fixstart));
 
-		/* Add the length if this is common with the postimage */
-		if (preimage->line[i].flag & LINE_COMMON)
-			postlen += tgtfix.len;
-
 		strbuf_release(&tgtfix);
 		if (!match) {
 			ret = 0;
@@ -2722,10 +2680,9 @@ static int match_fragment(struct apply_state *state,
 	 * has whitespace breakages unfixed, and fixing them makes the
 	 * hunk match.  Update the context lines in the postimage.
 	 */
-	if (postlen < postimage->len)
-		postlen = 0;
+	fixed_buf = strbuf_detach(&fixed, &fixed_len);
 	update_pre_post_images(preimage, postimage,
-			       fixed.buf, fixed.len, postlen);
+			       fixed_buf, fixed_len);
 
 	ret = 1;
 
@@ -2839,6 +2796,7 @@ static void update_image(struct apply_state *state,
 	 */
 	int i, nr;
 	size_t remove_count, insert_count, applied_at = 0;
+	size_t result_alloc;
 	char *result;
 	int preimage_limit;
 
@@ -2861,19 +2819,18 @@ static void update_image(struct apply_state *state,
 	remove_count = 0;
 	for (i = 0; i < preimage_limit; i++)
 		remove_count += img->line[applied_pos + i].len;
-	insert_count = postimage->len;
+	insert_count = postimage->buf.len;
 
 	/* Adjust the contents */
-	result = xmalloc(st_add3(st_sub(img->len, remove_count), insert_count, 1));
-	memcpy(result, img->buf, applied_at);
-	memcpy(result + applied_at, postimage->buf, postimage->len);
-	memcpy(result + applied_at + postimage->len,
-	       img->buf + (applied_at + remove_count),
-	       img->len - (applied_at + remove_count));
-	free(img->buf);
-	img->buf = result;
-	img->len += insert_count - remove_count;
-	result[img->len] = '\0';
+	result_alloc = st_add3(st_sub(img->buf.len, remove_count), insert_count, 1);
+	result = xmalloc(result_alloc);
+	memcpy(result, img->buf.buf, applied_at);
+	memcpy(result + applied_at, postimage->buf.buf, postimage->buf.len);
+	memcpy(result + applied_at + postimage->buf.len,
+	       img->buf.buf + (applied_at + remove_count),
+	       img->buf.len - (applied_at + remove_count));
+	strbuf_attach(&img->buf, result, postimage->buf.len + img->buf.len - remove_count,
+		      result_alloc);
 
 	/* Adjust the line table */
 	nr = img->line_nr + postimage->line_nr - preimage_limit;
@@ -3054,10 +3011,8 @@ static int apply_one_fragment(struct apply_state *state,
 	match_end = !state->unidiff_zero && !trailing;
 
 	pos = frag->newpos ? (frag->newpos - 1) : 0;
-	preimage.buf = oldlines;
-	preimage.len = old - oldlines;
-	postimage.buf = newlines.buf;
-	postimage.len = newlines.len;
+	strbuf_add(&preimage.buf, oldlines, old - oldlines);
+	strbuf_swap(&postimage.buf, &newlines);
 
 	for (;;) {
 
@@ -3145,8 +3100,8 @@ static int apply_one_fragment(struct apply_state *state,
 out:
 	free(oldlines);
 	strbuf_release(&newlines);
-	free(preimage.line);
-	free(postimage.line);
+	image_clear(&preimage);
+	image_clear(&postimage);
 
 	return (applied_pos < 0);
 }
@@ -3176,18 +3131,16 @@ static int apply_binary_fragment(struct apply_state *state,
 	}
 	switch (fragment->binary_patch_method) {
 	case BINARY_DELTA_DEFLATED:
-		dst = patch_delta(img->buf, img->len, fragment->patch,
+		dst = patch_delta(img->buf.buf, img->buf.len, fragment->patch,
 				  fragment->size, &len);
 		if (!dst)
 			return -1;
 		image_clear(img);
-		img->buf = dst;
-		img->len = len;
+		strbuf_attach(&img->buf, dst, len, len + 1);
 		return 0;
 	case BINARY_LITERAL_DEFLATED:
 		image_clear(img);
-		img->len = fragment->size;
-		img->buf = xmemdupz(fragment->patch, img->len);
+		strbuf_add(&img->buf, fragment->patch, fragment->size);
 		return 0;
 	}
 	return -1;
@@ -3223,8 +3176,8 @@ static int apply_binary(struct apply_state *state,
 		 * See if the old one matches what the patch
 		 * applies to.
 		 */
-		hash_object_file(the_hash_algo, img->buf, img->len, OBJ_BLOB,
-				 &oid);
+		hash_object_file(the_hash_algo, img->buf.buf, img->buf.len,
+				 OBJ_BLOB, &oid);
 		if (strcmp(oid_to_hex(&oid), patch->old_oid_prefix))
 			return error(_("the patch applies to '%s' (%s), "
 				       "which does not match the "
@@ -3233,7 +3186,7 @@ static int apply_binary(struct apply_state *state,
 	}
 	else {
 		/* Otherwise, the old one must be empty. */
-		if (img->len)
+		if (img->buf.len)
 			return error(_("the patch applies to an empty "
 				       "'%s' but it is not empty"), name);
 	}
@@ -3257,8 +3210,7 @@ static int apply_binary(struct apply_state *state,
 				       "'%s' cannot be read"),
 				     patch->new_oid_prefix, name);
 		image_clear(img);
-		img->buf = result;
-		img->len = size;
+		strbuf_attach(&img->buf, result, size, size + 1);
 	} else {
 		/*
 		 * We have verified buf matches the preimage;
@@ -3270,7 +3222,7 @@ static int apply_binary(struct apply_state *state,
 				     name);
 
 		/* verify that the result matches */
-		hash_object_file(the_hash_algo, img->buf, img->len, OBJ_BLOB,
+		hash_object_file(the_hash_algo, img->buf.buf, img->buf.len, OBJ_BLOB,
 				 &oid);
 		if (strcmp(oid_to_hex(&oid), patch->new_oid_prefix))
 			return error(_("binary patch to '%s' creates incorrect result (expecting %s, got %s)"),
@@ -3540,14 +3492,14 @@ static int resolve_to(struct image *image, const struct object_id *result_id)
 {
 	unsigned long size;
 	enum object_type type;
+	char *data;
 
 	image_clear(image);
 
-	image->buf = repo_read_object_file(the_repository, result_id, &type,
-					   &size);
-	if (!image->buf || type != OBJ_BLOB)
+	data = repo_read_object_file(the_repository, result_id, &type, &size);
+	if (!data || type != OBJ_BLOB)
 		die("unable to read blob object %s", oid_to_hex(result_id));
-	image->len = size;
+	strbuf_attach(&image->buf, data, size, size + 1);
 
 	return 0;
 }
@@ -3589,8 +3541,7 @@ static int three_way_merge(struct apply_state *state,
 		return -1;
 	}
 	image_clear(image);
-	image->buf = result.ptr;
-	image->len = result.size;
+	strbuf_attach(&image->buf, result.ptr, result.size, result.size);
 
 	return status;
 }
@@ -3677,7 +3628,7 @@ static int try_threeway(struct apply_state *state,
 		return -1;
 	}
 	/* post_oid is theirs */
-	write_object_file(tmp_image.buf, tmp_image.len, OBJ_BLOB, &post_oid);
+	write_object_file(tmp_image.buf.buf, tmp_image.buf.len, OBJ_BLOB, &post_oid);
 	image_clear(&tmp_image);
 
 	/* our_oid is ours */
@@ -3690,7 +3641,7 @@ static int try_threeway(struct apply_state *state,
 			return error(_("cannot read the current contents of '%s'"),
 				     patch->old_name);
 	}
-	write_object_file(tmp_image.buf, tmp_image.len, OBJ_BLOB, &our_oid);
+	write_object_file(tmp_image.buf.buf, tmp_image.buf.len, OBJ_BLOB, &our_oid);
 	image_clear(&tmp_image);
 
 	/* in-core three-way merge between post and our using pre as base */
@@ -3743,8 +3694,7 @@ static int apply_data(struct apply_state *state, struct patch *patch,
 			return -1;
 		}
 	}
-	patch->result = image.buf;
-	patch->resultsize = image.len;
+	patch->result = strbuf_detach(&image.buf, &patch->resultsize);
 	add_to_fn_table(state, patch);
 	free(image.line);
 
diff --git a/t/t3436-rebase-more-options.sh b/t/t3436-rebase-more-options.sh
index 94671d3c46..4d9744e5fc 100755
--- a/t/t3436-rebase-more-options.sh
+++ b/t/t3436-rebase-more-options.sh
@@ -5,6 +5,7 @@
 
 test_description='tests to ensure compatibility between am and interactive backends'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 . "$TEST_DIRECTORY"/lib-rebase.sh
diff --git a/t/t4107-apply-ignore-whitespace.sh b/t/t4107-apply-ignore-whitespace.sh
index ac72eeaf27..5e6e203aa5 100755
--- a/t/t4107-apply-ignore-whitespace.sh
+++ b/t/t4107-apply-ignore-whitespace.sh
@@ -3,9 +3,9 @@
 # Copyright (c) 2009 Giuseppe Bilotta
 #
 
-test_description='git-apply --ignore-whitespace.
+test_description='git-apply --ignore-whitespace.'
 
-'
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 # This primes main.c file that indents without using HT at all.
diff --git a/t/t4124-apply-ws-rule.sh b/t/t4124-apply-ws-rule.sh
index 485c7d2d12..cdffee0273 100755
--- a/t/t4124-apply-ws-rule.sh
+++ b/t/t4124-apply-ws-rule.sh
@@ -2,6 +2,7 @@
 
 test_description='core.whitespace rules and git apply'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 prepare_test_file () {
diff --git a/t/t4125-apply-ws-fuzz.sh b/t/t4125-apply-ws-fuzz.sh
index 090987c89b..f248cc2a00 100755
--- a/t/t4125-apply-ws-fuzz.sh
+++ b/t/t4125-apply-ws-fuzz.sh
@@ -2,6 +2,7 @@
 
 test_description='applying patch that has broken whitespaces in context'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success setup '
diff --git a/t/t4138-apply-ws-expansion.sh b/t/t4138-apply-ws-expansion.sh
index 8bbf8260fa..7981931b4e 100755
--- a/t/t4138-apply-ws-expansion.sh
+++ b/t/t4138-apply-ws-expansion.sh
@@ -5,6 +5,7 @@
 
 test_description='git apply test patches with whitespace expansion.'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success setup '
-- 
2.46.0.551.gc5ee8f2d1c.dirty

Re: [PATCH 6/6] apply: refactor `struct image` to use a `struct strbuf`

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> Refactor the code to use a `struct strbuf` instead, addressing all of
> the above. Like this we can easily perform in-place updates in all
> situations, the logic to perform those updates becomes way simpler and
> the lifetime of the buffer becomes a ton easier to track.
>
> This refactoring also plugs some leaking buffers as a side effect.

Nice.

In short, the leaks were in the original code where it was making
direct assignment to image->buf, and we now use strbuf_attach(),
which releases the current buffer before replacing it with a piece
of memory allocated outside the control of strbuf API?

>  static void image_remove_last_line(struct image *img)
>  {
> -	img->len -= img->line[--img->line_nr].len;
> +	strbuf_setlen(&img->buf, img->buf.len - img->line[--img->line_nr].len);
>  }

I feel that this, while technically is faithful to the original, got
a bit too complex to understand what is going on.  Perhaps split it
into two statements with an intermediate variable?  I dunno.

Thanks.

[PATCH v2 0/6] apply: fix leaking buffer of `struct image`

[Patrick Steinhardt]

Hi,

this is the second version of my patch series that refactors lifecycle
management of `struct image` in "apply.c" to plug a bunch of memory
leaks.

Changes compared to v1:
 
  - Fix two typos.

  - Correct the statement that we don't loop around
    `image_remove_first_line()`. No idea how I missed that loop.

  - Split up an overly complex line into multiple lines.

Thanks!

Patrick

Patrick Steinhardt (6):
  apply: reorder functions to move image-related things together
  apply: rename functions operating on `struct image`
  apply: introduce macro and function to init images
  apply: refactor code to drop `line_allocated`
  apply: rename members that track line count and allocation length
  apply: refactor `struct image` to use a `struct strbuf`

 apply.c                            | 449 +++++++++++++----------------
 t/t3436-rebase-more-options.sh     |   1 +
 t/t4107-apply-ignore-whitespace.sh |   4 +-
 t/t4124-apply-ws-rule.sh           |   1 +
 t/t4125-apply-ws-fuzz.sh           |   1 +
 t/t4138-apply-ws-expansion.sh      |   1 +
 6 files changed, 206 insertions(+), 251 deletions(-)

Range-diff against v1:
1:  7b6903ecdd = 1:  a713a7aef0 apply: reorder functions to move image-related things together
2:  3f188412f6 = 2:  be8f98881f apply: rename functions operating on `struct image`
3:  1b49e39bcd = 3:  506c787d68 apply: introduce macro and function to init images
4:  0427cb7250 ! 4:  6ac37186f2 apply: refactor code to drop `line_allocated`
    @@ Commit message
         apply: refactor code to drop `line_allocated`
     
         The `struct image` has two members `line` and `line_allocated`. The
    -    former member is the one that should be used throughougt the code,
    +    former member is the one that should be used throughout the code,
         whereas the latter one is used to track whether the lines have been
         allocated or not.
     
         In practice, the array of lines is always allocated. The reason why we
         have `line_allocated` is that `remove_first_line()` will advance the
    -    array pointer to drop the first entry, and thus it point into the array
    +    array pointer to drop the first entry, and thus it points into the array
         instead of to the array header.
     
         Refactor the function to use memmove(3P) instead, which allows us to get
    -    rid of this double bookkeeping. We call this function at most once per
    -    image anyway, so this shouldn't cause any performance regressions.
    +    rid of this double bookkeeping. This is less efficient, but I doubt that
    +    this matters much in practice. If this judgement call is found to be
    +    wrong at a later point in time we can likely refactor the surrounding
    +    loop such that we first calculate the number of leading context lines to
    +    remove and then remove them in a single call to memmove(3P).
     
         Signed-off-by: Patrick Steinhardt <ps@pks.im>
     
5:  e35816ed56 = 5:  5497708428 apply: rename members that track line count and allocation length
6:  6cf45daf84 ! 6:  42880dcf04 apply: refactor `struct image` to use a `struct strbuf`
    @@ apply.c: static void image_remove_first_line(struct image *img)
      static void image_remove_last_line(struct image *img)
      {
     -	img->len -= img->line[--img->line_nr].len;
    -+	strbuf_setlen(&img->buf, img->buf.len - img->line[--img->line_nr].len);
    ++	size_t last_line_len = img->line[img->line_nr - 1].len;
    ++	strbuf_setlen(&img->buf, img->buf.len - last_line_len);
    ++	img->line_nr--;
      }
      
      /* fmt must contain _one_ %s and no other substitution */

base-commit: ed155187b429a2a6b6475efe1767053df37ccfe1
-- 
2.46.0.551.gc5ee8f2d1c.dirty

[PATCH v2 1/6] apply: reorder functions to move image-related things together

[Patrick Steinhardt]

While most of the functions relating to `struct image` are relatively
close to one another, `fuzzy_matchlines()` sits in between those even
though it is rather unrelated.

Reorder functions such that `struct image`-related functions are next to
each other. While at it, move `clear_image()` to the top such that it is
close to the struct definition itself. This makes this lifecycle-related
thing easy to discover.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 apply.c | 106 ++++++++++++++++++++++++++++----------------------------
 1 file changed, 53 insertions(+), 53 deletions(-)

diff --git a/apply.c b/apply.c
index 6e1060a952..9dd2f4d215 100644
--- a/apply.c
+++ b/apply.c
@@ -285,6 +285,13 @@ struct image {
 	struct line *line;
 };
 
+static void clear_image(struct image *image)
+{
+	free(image->buf);
+	free(image->line_allocated);
+	memset(image, 0, sizeof(*image));
+}
+
 static uint32_t hash_line(const char *cp, size_t len)
 {
 	size_t i;
@@ -297,42 +304,6 @@ static uint32_t hash_line(const char *cp, size_t len)
 	return h;
 }
 
-/*
- * Compare lines s1 of length n1 and s2 of length n2, ignoring
- * whitespace difference. Returns 1 if they match, 0 otherwise
- */
-static int fuzzy_matchlines(const char *s1, size_t n1,
-			    const char *s2, size_t n2)
-{
-	const char *end1 = s1 + n1;
-	const char *end2 = s2 + n2;
-
-	/* ignore line endings */
-	while (s1 < end1 && (end1[-1] == '\r' || end1[-1] == '\n'))
-		end1--;
-	while (s2 < end2 && (end2[-1] == '\r' || end2[-1] == '\n'))
-		end2--;
-
-	while (s1 < end1 && s2 < end2) {
-		if (isspace(*s1)) {
-			/*
-			 * Skip whitespace. We check on both buffers
-			 * because we don't want "a b" to match "ab".
-			 */
-			if (!isspace(*s2))
-				return 0;
-			while (s1 < end1 && isspace(*s1))
-				s1++;
-			while (s2 < end2 && isspace(*s2))
-				s2++;
-		} else if (*s1++ != *s2++)
-			return 0;
-	}
-
-	/* If we reached the end on one side only, lines don't match. */
-	return s1 == end1 && s2 == end2;
-}
-
 static void add_line_info(struct image *img, const char *bol, size_t len, unsigned flag)
 {
 	ALLOC_GROW(img->line_allocated, img->nr + 1, img->alloc);
@@ -373,11 +344,17 @@ static void prepare_image(struct image *image, char *buf, size_t len,
 	image->line = image->line_allocated;
 }
 
-static void clear_image(struct image *image)
+static void remove_first_line(struct image *img)
 {
-	free(image->buf);
-	free(image->line_allocated);
-	memset(image, 0, sizeof(*image));
+	img->buf += img->line[0].len;
+	img->len -= img->line[0].len;
+	img->line++;
+	img->nr--;
+}
+
+static void remove_last_line(struct image *img)
+{
+	img->len -= img->line[--img->nr].len;
 }
 
 /* fmt must contain _one_ %s and no other substitution */
@@ -2419,6 +2396,42 @@ static void update_pre_post_images(struct image *preimage,
 	postimage->nr -= reduced;
 }
 
+/*
+ * Compare lines s1 of length n1 and s2 of length n2, ignoring
+ * whitespace difference. Returns 1 if they match, 0 otherwise
+ */
+static int fuzzy_matchlines(const char *s1, size_t n1,
+			    const char *s2, size_t n2)
+{
+	const char *end1 = s1 + n1;
+	const char *end2 = s2 + n2;
+
+	/* ignore line endings */
+	while (s1 < end1 && (end1[-1] == '\r' || end1[-1] == '\n'))
+		end1--;
+	while (s2 < end2 && (end2[-1] == '\r' || end2[-1] == '\n'))
+		end2--;
+
+	while (s1 < end1 && s2 < end2) {
+		if (isspace(*s1)) {
+			/*
+			 * Skip whitespace. We check on both buffers
+			 * because we don't want "a b" to match "ab".
+			 */
+			if (!isspace(*s2))
+				return 0;
+			while (s1 < end1 && isspace(*s1))
+				s1++;
+			while (s2 < end2 && isspace(*s2))
+				s2++;
+		} else if (*s1++ != *s2++)
+			return 0;
+	}
+
+	/* If we reached the end on one side only, lines don't match. */
+	return s1 == end1 && s2 == end2;
+}
+
 static int line_by_line_fuzzy_match(struct image *img,
 				    struct image *preimage,
 				    struct image *postimage,
@@ -2804,19 +2817,6 @@ static int find_pos(struct apply_state *state,
 	return -1;
 }
 
-static void remove_first_line(struct image *img)
-{
-	img->buf += img->line[0].len;
-	img->len -= img->line[0].len;
-	img->line++;
-	img->nr--;
-}
-
-static void remove_last_line(struct image *img)
-{
-	img->len -= img->line[--img->nr].len;
-}
-
 /*
  * The change from "preimage" and "postimage" has been found to
  * apply at applied_pos (counts in line numbers) in "img".
-- 
2.46.0.551.gc5ee8f2d1c.dirty

[PATCH v2 2/6] apply: rename functions operating on `struct image`

[Patrick Steinhardt]

Rename functions operating on `struct image` to have a `image_` prefix
to match our modern code style.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 apply.c | 61 ++++++++++++++++++++++++++++-----------------------------
 1 file changed, 30 insertions(+), 31 deletions(-)

diff --git a/apply.c b/apply.c
index 9dd2f4d215..ac21c21297 100644
--- a/apply.c
+++ b/apply.c
@@ -285,11 +285,10 @@ struct image {
 	struct line *line;
 };
 
-static void clear_image(struct image *image)
+static void image_clear(struct image *image)
 {
 	free(image->buf);
 	free(image->line_allocated);
-	memset(image, 0, sizeof(*image));
 }
 
 static uint32_t hash_line(const char *cp, size_t len)
@@ -304,7 +303,7 @@ static uint32_t hash_line(const char *cp, size_t len)
 	return h;
 }
 
-static void add_line_info(struct image *img, const char *bol, size_t len, unsigned flag)
+static void image_add_line(struct image *img, const char *bol, size_t len, unsigned flag)
 {
 	ALLOC_GROW(img->line_allocated, img->nr + 1, img->alloc);
 	img->line_allocated[img->nr].len = len;
@@ -318,7 +317,7 @@ static void add_line_info(struct image *img, const char *bol, size_t len, unsign
  * attach it to "image" and add line-based index to it.
  * "image" now owns the "buf".
  */
-static void prepare_image(struct image *image, char *buf, size_t len,
+static void image_prepare(struct image *image, char *buf, size_t len,
 			  int prepare_linetable)
 {
 	const char *cp, *ep;
@@ -338,13 +337,13 @@ static void prepare_image(struct image *image, char *buf, size_t len,
 			;
 		if (next < ep)
 			next++;
-		add_line_info(image, cp, next - cp, 0);
+		image_add_line(image, cp, next - cp, 0);
 		cp = next;
 	}
 	image->line = image->line_allocated;
 }
 
-static void remove_first_line(struct image *img)
+static void image_remove_first_line(struct image *img)
 {
 	img->buf += img->line[0].len;
 	img->len -= img->line[0].len;
@@ -352,7 +351,7 @@ static void remove_first_line(struct image *img)
 	img->nr--;
 }
 
-static void remove_last_line(struct image *img)
+static void image_remove_last_line(struct image *img)
 {
 	img->len -= img->line[--img->nr].len;
 }
@@ -2322,7 +2321,7 @@ static void update_pre_post_images(struct image *preimage,
 	 * are not losing preimage->buf -- apply_one_fragment() will
 	 * free "oldlines".
 	 */
-	prepare_image(&fixed_preimage, buf, len, 1);
+	image_prepare(&fixed_preimage, buf, len, 1);
 	assert(postlen
 	       ? fixed_preimage.nr == preimage->nr
 	       : fixed_preimage.nr <= preimage->nr);
@@ -2874,7 +2873,7 @@ static void update_image(struct apply_state *state,
 	nr = img->nr + postimage->nr - preimage_limit;
 	if (preimage_limit < postimage->nr) {
 		/*
-		 * NOTE: this knows that we never call remove_first_line()
+		 * NOTE: this knows that we never call image_remove_first_line()
 		 * on anything other than pre/post image.
 		 */
 		REALLOC_ARRAY(img->line, nr);
@@ -2957,8 +2956,8 @@ static int apply_one_fragment(struct apply_state *state,
 				break;
 			*old++ = '\n';
 			strbuf_addch(&newlines, '\n');
-			add_line_info(&preimage, "\n", 1, LINE_COMMON);
-			add_line_info(&postimage, "\n", 1, LINE_COMMON);
+			image_add_line(&preimage, "\n", 1, LINE_COMMON);
+			image_add_line(&postimage, "\n", 1, LINE_COMMON);
 			is_blank_context = 1;
 			break;
 		case ' ':
@@ -2968,7 +2967,7 @@ static int apply_one_fragment(struct apply_state *state,
 			/* fallthrough */
 		case '-':
 			memcpy(old, patch + 1, plen);
-			add_line_info(&preimage, old, plen,
+			image_add_line(&preimage, old, plen,
 				      (first == ' ' ? LINE_COMMON : 0));
 			old += plen;
 			if (first == '-')
@@ -2988,7 +2987,7 @@ static int apply_one_fragment(struct apply_state *state,
 			else {
 				ws_fix_copy(&newlines, patch + 1, plen, ws_rule, &state->applied_after_fixing_ws);
 			}
-			add_line_info(&postimage, newlines.buf + start, newlines.len - start,
+			image_add_line(&postimage, newlines.buf + start, newlines.len - start,
 				      (first == '+' ? 0 : LINE_COMMON));
 			if (first == '+' &&
 			    (ws_rule & WS_BLANK_AT_EOF) &&
@@ -3082,14 +3081,14 @@ static int apply_one_fragment(struct apply_state *state,
 		 * just reduce the larger context.
 		 */
 		if (leading >= trailing) {
-			remove_first_line(&preimage);
-			remove_first_line(&postimage);
+			image_remove_first_line(&preimage);
+			image_remove_first_line(&postimage);
 			pos--;
 			leading--;
 		}
 		if (trailing > leading) {
-			remove_last_line(&preimage);
-			remove_last_line(&postimage);
+			image_remove_last_line(&preimage);
+			image_remove_last_line(&postimage);
 			trailing--;
 		}
 	}
@@ -3103,7 +3102,7 @@ static int apply_one_fragment(struct apply_state *state,
 					found_new_blank_lines_at_end);
 			if (state->ws_error_action == correct_ws_error) {
 				while (new_blank_lines_at_end--)
-					remove_last_line(&postimage);
+					image_remove_last_line(&postimage);
 			}
 			/*
 			 * We would want to prevent write_out_results()
@@ -3181,12 +3180,12 @@ static int apply_binary_fragment(struct apply_state *state,
 				  fragment->size, &len);
 		if (!dst)
 			return -1;
-		clear_image(img);
+		image_clear(img);
 		img->buf = dst;
 		img->len = len;
 		return 0;
 	case BINARY_LITERAL_DEFLATED:
-		clear_image(img);
+		image_clear(img);
 		img->len = fragment->size;
 		img->buf = xmemdupz(fragment->patch, img->len);
 		return 0;
@@ -3241,7 +3240,7 @@ static int apply_binary(struct apply_state *state,
 
 	get_oid_hex(patch->new_oid_prefix, &oid);
 	if (is_null_oid(&oid)) {
-		clear_image(img);
+		image_clear(img);
 		return 0; /* deletion patch */
 	}
 
@@ -3257,7 +3256,7 @@ static int apply_binary(struct apply_state *state,
 			return error(_("the necessary postimage %s for "
 				       "'%s' cannot be read"),
 				     patch->new_oid_prefix, name);
-		clear_image(img);
+		image_clear(img);
 		img->buf = result;
 		img->len = size;
 	} else {
@@ -3533,7 +3532,7 @@ static int load_preimage(struct apply_state *state,
 	}
 
 	img = strbuf_detach(&buf, &len);
-	prepare_image(image, img, len, !patch->is_binary);
+	image_prepare(image, img, len, !patch->is_binary);
 	return 0;
 }
 
@@ -3542,7 +3541,7 @@ static int resolve_to(struct image *image, const struct object_id *result_id)
 	unsigned long size;
 	enum object_type type;
 
-	clear_image(image);
+	image_clear(image);
 
 	image->buf = repo_read_object_file(the_repository, result_id, &type,
 					   &size);
@@ -3589,7 +3588,7 @@ static int three_way_merge(struct apply_state *state,
 		free(result.ptr);
 		return -1;
 	}
-	clear_image(image);
+	image_clear(image);
 	image->buf = result.ptr;
 	image->len = result.size;
 
@@ -3636,7 +3635,7 @@ static int load_current(struct apply_state *state,
 	else if (status)
 		return -1;
 	img = strbuf_detach(&buf, &len);
-	prepare_image(image, img, len, !patch->is_binary);
+	image_prepare(image, img, len, !patch->is_binary);
 	return 0;
 }
 
@@ -3671,15 +3670,15 @@ static int try_threeway(struct apply_state *state,
 		fprintf(stderr, _("Performing three-way merge...\n"));
 
 	img = strbuf_detach(&buf, &len);
-	prepare_image(&tmp_image, img, len, 1);
+	image_prepare(&tmp_image, img, len, 1);
 	/* Apply the patch to get the post image */
 	if (apply_fragments(state, &tmp_image, patch) < 0) {
-		clear_image(&tmp_image);
+		image_clear(&tmp_image);
 		return -1;
 	}
 	/* post_oid is theirs */
 	write_object_file(tmp_image.buf, tmp_image.len, OBJ_BLOB, &post_oid);
-	clear_image(&tmp_image);
+	image_clear(&tmp_image);
 
 	/* our_oid is ours */
 	if (patch->is_new) {
@@ -3692,7 +3691,7 @@ static int try_threeway(struct apply_state *state,
 				     patch->old_name);
 	}
 	write_object_file(tmp_image.buf, tmp_image.len, OBJ_BLOB, &our_oid);
-	clear_image(&tmp_image);
+	image_clear(&tmp_image);
 
 	/* in-core three-way merge between post and our using pre as base */
 	status = three_way_merge(state, image, patch->new_name,
@@ -3740,7 +3739,7 @@ static int apply_data(struct apply_state *state, struct patch *patch,
 
 		/* Note: with --reject, apply_fragments() returns 0 */
 		if (patch->direct_to_threeway || apply_fragments(state, &image, patch) < 0) {
-			clear_image(&image);
+			image_clear(&image);
 			return -1;
 		}
 	}
-- 
2.46.0.551.gc5ee8f2d1c.dirty

[PATCH v2 3/6] apply: introduce macro and function to init images

[Patrick Steinhardt]

We're about to convert the `struct image` to gain a `struct strbuf`
member, which requires more careful initialization than just memsetting
it to zeros. Introduce the `IMAGE_INIT` macro and `image_init()`
function to prepare for this change.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 apply.c | 22 ++++++++++++++--------
 1 file changed, 14 insertions(+), 8 deletions(-)

diff --git a/apply.c b/apply.c
index ac21c21297..76f7777d4c 100644
--- a/apply.c
+++ b/apply.c
@@ -284,11 +284,19 @@ struct image {
 	struct line *line_allocated;
 	struct line *line;
 };
+#define IMAGE_INIT { 0 }
+
+static void image_init(struct image *image)
+{
+	struct image empty = IMAGE_INIT;
+	memcpy(image, &empty, sizeof(*image));
+}
 
 static void image_clear(struct image *image)
 {
 	free(image->buf);
 	free(image->line_allocated);
+	image_init(image);
 }
 
 static uint32_t hash_line(const char *cp, size_t len)
@@ -322,7 +330,7 @@ static void image_prepare(struct image *image, char *buf, size_t len,
 {
 	const char *cp, *ep;
 
-	memset(image, 0, sizeof(*image));
+	image_clear(image);
 	image->buf = buf;
 	image->len = len;
 
@@ -2314,7 +2322,7 @@ static void update_pre_post_images(struct image *preimage,
 {
 	int i, ctx, reduced;
 	char *new_buf, *old_buf, *fixed;
-	struct image fixed_preimage;
+	struct image fixed_preimage = IMAGE_INIT;
 
 	/*
 	 * Update the preimage with whitespace fixes.  Note that we
@@ -2910,11 +2918,9 @@ static int apply_one_fragment(struct apply_state *state,
 	int hunk_linenr = frag->linenr;
 	unsigned long leading, trailing;
 	int pos, applied_pos;
-	struct image preimage;
-	struct image postimage;
+	struct image preimage = IMAGE_INIT;
+	struct image postimage = IMAGE_INIT;
 
-	memset(&preimage, 0, sizeof(preimage));
-	memset(&postimage, 0, sizeof(postimage));
 	oldlines = xmalloc(size);
 	strbuf_init(&newlines, size);
 
@@ -3650,7 +3656,7 @@ static int try_threeway(struct apply_state *state,
 	size_t len;
 	int status;
 	char *img;
-	struct image tmp_image;
+	struct image tmp_image = IMAGE_INIT;
 
 	/* No point falling back to 3-way merge in these cases */
 	if (patch->is_delete ||
@@ -3727,7 +3733,7 @@ static int try_threeway(struct apply_state *state,
 static int apply_data(struct apply_state *state, struct patch *patch,
 		      struct stat *st, const struct cache_entry *ce)
 {
-	struct image image;
+	struct image image = IMAGE_INIT;
 
 	if (load_preimage(state, &image, patch, st, ce) < 0)
 		return -1;
-- 
2.46.0.551.gc5ee8f2d1c.dirty

[PATCH v2 4/6] apply: refactor code to drop `line_allocated`

[Patrick Steinhardt]

The `struct image` has two members `line` and `line_allocated`. The
former member is the one that should be used throughout the code,
whereas the latter one is used to track whether the lines have been
allocated or not.

In practice, the array of lines is always allocated. The reason why we
have `line_allocated` is that `remove_first_line()` will advance the
array pointer to drop the first entry, and thus it points into the array
instead of to the array header.

Refactor the function to use memmove(3P) instead, which allows us to get
rid of this double bookkeeping. This is less efficient, but I doubt that
this matters much in practice. If this judgement call is found to be
wrong at a later point in time we can likely refactor the surrounding
loop such that we first calculate the number of leading context lines to
remove and then remove them in a single call to memmove(3P).

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 apply.c | 33 ++++++++++++++-------------------
 1 file changed, 14 insertions(+), 19 deletions(-)

diff --git a/apply.c b/apply.c
index 76f7777d4c..985564ac76 100644
--- a/apply.c
+++ b/apply.c
@@ -281,7 +281,6 @@ struct image {
 	size_t len;
 	size_t nr;
 	size_t alloc;
-	struct line *line_allocated;
 	struct line *line;
 };
 #define IMAGE_INIT { 0 }
@@ -295,7 +294,7 @@ static void image_init(struct image *image)
 static void image_clear(struct image *image)
 {
 	free(image->buf);
-	free(image->line_allocated);
+	free(image->line);
 	image_init(image);
 }
 
@@ -313,10 +312,10 @@ static uint32_t hash_line(const char *cp, size_t len)
 
 static void image_add_line(struct image *img, const char *bol, size_t len, unsigned flag)
 {
-	ALLOC_GROW(img->line_allocated, img->nr + 1, img->alloc);
-	img->line_allocated[img->nr].len = len;
-	img->line_allocated[img->nr].hash = hash_line(bol, len);
-	img->line_allocated[img->nr].flag = flag;
+	ALLOC_GROW(img->line, img->nr + 1, img->alloc);
+	img->line[img->nr].len = len;
+	img->line[img->nr].hash = hash_line(bol, len);
+	img->line[img->nr].flag = flag;
 	img->nr++;
 }
 
@@ -348,15 +347,15 @@ static void image_prepare(struct image *image, char *buf, size_t len,
 		image_add_line(image, cp, next - cp, 0);
 		cp = next;
 	}
-	image->line = image->line_allocated;
 }
 
 static void image_remove_first_line(struct image *img)
 {
 	img->buf += img->line[0].len;
 	img->len -= img->line[0].len;
-	img->line++;
 	img->nr--;
+	if (img->nr)
+		MOVE_ARRAY(img->line, img->line + 1, img->nr);
 }
 
 static void image_remove_last_line(struct image *img)
@@ -2335,7 +2334,7 @@ static void update_pre_post_images(struct image *preimage,
 	       : fixed_preimage.nr <= preimage->nr);
 	for (i = 0; i < fixed_preimage.nr; i++)
 		fixed_preimage.line[i].flag = preimage->line[i].flag;
-	free(preimage->line_allocated);
+	free(preimage->line);
 	*preimage = fixed_preimage;
 
 	/*
@@ -2879,14 +2878,12 @@ static void update_image(struct apply_state *state,
 
 	/* Adjust the line table */
 	nr = img->nr + postimage->nr - preimage_limit;
-	if (preimage_limit < postimage->nr) {
+	if (preimage_limit < postimage->nr)
 		/*
 		 * NOTE: this knows that we never call image_remove_first_line()
 		 * on anything other than pre/post image.
 		 */
 		REALLOC_ARRAY(img->line, nr);
-		img->line_allocated = img->line;
-	}
 	if (preimage_limit != postimage->nr)
 		MOVE_ARRAY(img->line + applied_pos + postimage->nr,
 			   img->line + applied_pos + preimage_limit,
@@ -3027,8 +3024,8 @@ static int apply_one_fragment(struct apply_state *state,
 	    newlines.len > 0 && newlines.buf[newlines.len - 1] == '\n') {
 		old--;
 		strbuf_setlen(&newlines, newlines.len - 1);
-		preimage.line_allocated[preimage.nr - 1].len--;
-		postimage.line_allocated[postimage.nr - 1].len--;
+		preimage.line[preimage.nr - 1].len--;
+		postimage.line[postimage.nr - 1].len--;
 	}
 
 	leading = frag->leading;
@@ -3062,8 +3059,6 @@ static int apply_one_fragment(struct apply_state *state,
 	preimage.len = old - oldlines;
 	postimage.buf = newlines.buf;
 	postimage.len = newlines.len;
-	preimage.line = preimage.line_allocated;
-	postimage.line = postimage.line_allocated;
 
 	for (;;) {
 
@@ -3151,8 +3146,8 @@ static int apply_one_fragment(struct apply_state *state,
 out:
 	free(oldlines);
 	strbuf_release(&newlines);
-	free(preimage.line_allocated);
-	free(postimage.line_allocated);
+	free(preimage.line);
+	free(postimage.line);
 
 	return (applied_pos < 0);
 }
@@ -3752,7 +3747,7 @@ static int apply_data(struct apply_state *state, struct patch *patch,
 	patch->result = image.buf;
 	patch->resultsize = image.len;
 	add_to_fn_table(state, patch);
-	free(image.line_allocated);
+	free(image.line);
 
 	if (0 < patch->is_delete && patch->resultsize)
 		return error(_("removal patch leaves file contents"));
-- 
2.46.0.551.gc5ee8f2d1c.dirty

[PATCH v2 5/6] apply: rename members that track line count and allocation length

[Patrick Steinhardt]

The `struct image` has two members `nr` and `alloc` that track the
number of lines as well as how large its array is. It is somewhat easy
to confuse these members with `len` though, which tracks the length of
the `buf` member.

Rename these members to `line_nr` and `line_alloc` respectively to avoid
confusion. This is in line with how we typically name variables that
track an array in this way.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 apply.c | 91 ++++++++++++++++++++++++++++-----------------------------
 1 file changed, 45 insertions(+), 46 deletions(-)

diff --git a/apply.c b/apply.c
index 985564ac76..3340bb1fc0 100644
--- a/apply.c
+++ b/apply.c
@@ -279,9 +279,8 @@ struct line {
 struct image {
 	char *buf;
 	size_t len;
-	size_t nr;
-	size_t alloc;
 	struct line *line;
+	size_t line_nr, line_alloc;
 };
 #define IMAGE_INIT { 0 }
 
@@ -312,11 +311,11 @@ static uint32_t hash_line(const char *cp, size_t len)
 
 static void image_add_line(struct image *img, const char *bol, size_t len, unsigned flag)
 {
-	ALLOC_GROW(img->line, img->nr + 1, img->alloc);
-	img->line[img->nr].len = len;
-	img->line[img->nr].hash = hash_line(bol, len);
-	img->line[img->nr].flag = flag;
-	img->nr++;
+	ALLOC_GROW(img->line, img->line_nr + 1, img->line_alloc);
+	img->line[img->line_nr].len = len;
+	img->line[img->line_nr].hash = hash_line(bol, len);
+	img->line[img->line_nr].flag = flag;
+	img->line_nr++;
 }
 
 /*
@@ -353,14 +352,14 @@ static void image_remove_first_line(struct image *img)
 {
 	img->buf += img->line[0].len;
 	img->len -= img->line[0].len;
-	img->nr--;
-	if (img->nr)
-		MOVE_ARRAY(img->line, img->line + 1, img->nr);
+	img->line_nr--;
+	if (img->line_nr)
+		MOVE_ARRAY(img->line, img->line + 1, img->line_nr);
 }
 
 static void image_remove_last_line(struct image *img)
 {
-	img->len -= img->line[--img->nr].len;
+	img->len -= img->line[--img->line_nr].len;
 }
 
 /* fmt must contain _one_ %s and no other substitution */
@@ -2330,9 +2329,9 @@ static void update_pre_post_images(struct image *preimage,
 	 */
 	image_prepare(&fixed_preimage, buf, len, 1);
 	assert(postlen
-	       ? fixed_preimage.nr == preimage->nr
-	       : fixed_preimage.nr <= preimage->nr);
-	for (i = 0; i < fixed_preimage.nr; i++)
+	       ? fixed_preimage.line_nr == preimage->line_nr
+	       : fixed_preimage.line_nr <= preimage->line_nr);
+	for (i = 0; i < fixed_preimage.line_nr; i++)
 		fixed_preimage.line[i].flag = preimage->line[i].flag;
 	free(preimage->line);
 	*preimage = fixed_preimage;
@@ -2353,7 +2352,7 @@ static void update_pre_post_images(struct image *preimage,
 		new_buf = old_buf;
 	fixed = preimage->buf;
 
-	for (i = reduced = ctx = 0; i < postimage->nr; i++) {
+	for (i = reduced = ctx = 0; i < postimage->line_nr; i++) {
 		size_t l_len = postimage->line[i].len;
 		if (!(postimage->line[i].flag & LINE_COMMON)) {
 			/* an added line -- no counterparts in preimage */
@@ -2367,7 +2366,7 @@ static void update_pre_post_images(struct image *preimage,
 		old_buf += l_len;
 
 		/* and find the corresponding one in the fixed preimage */
-		while (ctx < preimage->nr &&
+		while (ctx < preimage->line_nr &&
 		       !(preimage->line[ctx].flag & LINE_COMMON)) {
 			fixed += preimage->line[ctx].len;
 			ctx++;
@@ -2377,7 +2376,7 @@ static void update_pre_post_images(struct image *preimage,
 		 * preimage is expected to run out, if the caller
 		 * fixed addition of trailing blank lines.
 		 */
-		if (preimage->nr <= ctx) {
+		if (preimage->line_nr <= ctx) {
 			reduced++;
 			continue;
 		}
@@ -2399,7 +2398,7 @@ static void update_pre_post_images(struct image *preimage,
 
 	/* Fix the length of the whole thing */
 	postimage->len = new_buf - postimage->buf;
-	postimage->nr -= reduced;
+	postimage->line_nr -= reduced;
 }
 
 /*
@@ -2482,7 +2481,7 @@ static int line_by_line_fuzzy_match(struct image *img,
 	 * we are removing blank lines at the end of the file.)
 	 */
 	buf = preimage_eof = preimage->buf + preoff;
-	for ( ; i < preimage->nr; i++)
+	for ( ; i < preimage->line_nr; i++)
 		preoff += preimage->line[i].len;
 	preimage_end = preimage->buf + preoff;
 	for ( ; buf < preimage_end; buf++)
@@ -2522,12 +2521,12 @@ static int match_fragment(struct apply_state *state,
 	int preimage_limit;
 	int ret;
 
-	if (preimage->nr + current_lno <= img->nr) {
+	if (preimage->line_nr + current_lno <= img->line_nr) {
 		/*
 		 * The hunk falls within the boundaries of img.
 		 */
-		preimage_limit = preimage->nr;
-		if (match_end && (preimage->nr + current_lno != img->nr)) {
+		preimage_limit = preimage->line_nr;
+		if (match_end && (preimage->line_nr + current_lno != img->line_nr)) {
 			ret = 0;
 			goto out;
 		}
@@ -2540,7 +2539,7 @@ static int match_fragment(struct apply_state *state,
 		 * match with img, and the remainder of the preimage
 		 * must be blank.
 		 */
-		preimage_limit = img->nr - current_lno;
+		preimage_limit = img->line_nr - current_lno;
 	} else {
 		/*
 		 * The hunk extends beyond the end of the img and
@@ -2565,7 +2564,7 @@ static int match_fragment(struct apply_state *state,
 		}
 	}
 
-	if (preimage_limit == preimage->nr) {
+	if (preimage_limit == preimage->line_nr) {
 		/*
 		 * Do we have an exact match?  If we were told to match
 		 * at the end, size must be exactly at current+fragsize,
@@ -2637,7 +2636,7 @@ static int match_fragment(struct apply_state *state,
 
 	/* First count added lines in postimage */
 	postlen = 0;
-	for (i = 0; i < postimage->nr; i++) {
+	for (i = 0; i < postimage->line_nr; i++) {
 		if (!(postimage->line[i].flag & LINE_COMMON))
 			postlen += postimage->line[i].len;
 	}
@@ -2699,7 +2698,7 @@ static int match_fragment(struct apply_state *state,
 	 * empty or only contain whitespace (if WS_BLANK_AT_EOL is
 	 * false).
 	 */
-	for ( ; i < preimage->nr; i++) {
+	for ( ; i < preimage->line_nr; i++) {
 		size_t fixstart = fixed.len; /* start of the fixed preimage */
 		size_t oldlen = preimage->line[i].len;
 		int j;
@@ -2754,7 +2753,7 @@ static int find_pos(struct apply_state *state,
 	 * than `match_beginning`.
 	 */
 	if (state->allow_overlap && match_beginning && match_end &&
-	    img->nr - preimage->nr != 0)
+	    img->line_nr - preimage->line_nr != 0)
 		match_beginning = 0;
 
 	/*
@@ -2765,15 +2764,15 @@ static int find_pos(struct apply_state *state,
 	if (match_beginning)
 		line = 0;
 	else if (match_end)
-		line = img->nr - preimage->nr;
+		line = img->line_nr - preimage->line_nr;
 
 	/*
 	 * Because the comparison is unsigned, the following test
 	 * will also take care of a negative line number that can
 	 * result when match_end and preimage is larger than the target.
 	 */
-	if ((size_t) line > img->nr)
-		line = img->nr;
+	if ((size_t) line > img->line_nr)
+		line = img->line_nr;
 
 	current = 0;
 	for (i = 0; i < line; i++)
@@ -2796,7 +2795,7 @@ static int find_pos(struct apply_state *state,
 			return current_lno;
 
 	again:
-		if (backwards_lno == 0 && forwards_lno == img->nr)
+		if (backwards_lno == 0 && forwards_lno == img->line_nr)
 			break;
 
 		if (i & 1) {
@@ -2809,7 +2808,7 @@ static int find_pos(struct apply_state *state,
 			current = backwards;
 			current_lno = backwards_lno;
 		} else {
-			if (forwards_lno == img->nr) {
+			if (forwards_lno == img->line_nr) {
 				i++;
 				goto again;
 			}
@@ -2852,9 +2851,9 @@ static void update_image(struct apply_state *state,
 	 * to the number of lines in the preimage that falls
 	 * within the boundaries.
 	 */
-	preimage_limit = preimage->nr;
-	if (preimage_limit > img->nr - applied_pos)
-		preimage_limit = img->nr - applied_pos;
+	preimage_limit = preimage->line_nr;
+	if (preimage_limit > img->line_nr - applied_pos)
+		preimage_limit = img->line_nr - applied_pos;
 
 	for (i = 0; i < applied_pos; i++)
 		applied_at += img->line[i].len;
@@ -2877,22 +2876,22 @@ static void update_image(struct apply_state *state,
 	result[img->len] = '\0';
 
 	/* Adjust the line table */
-	nr = img->nr + postimage->nr - preimage_limit;
-	if (preimage_limit < postimage->nr)
+	nr = img->line_nr + postimage->line_nr - preimage_limit;
+	if (preimage_limit < postimage->line_nr)
 		/*
 		 * NOTE: this knows that we never call image_remove_first_line()
 		 * on anything other than pre/post image.
 		 */
 		REALLOC_ARRAY(img->line, nr);
-	if (preimage_limit != postimage->nr)
-		MOVE_ARRAY(img->line + applied_pos + postimage->nr,
+	if (preimage_limit != postimage->line_nr)
+		MOVE_ARRAY(img->line + applied_pos + postimage->line_nr,
 			   img->line + applied_pos + preimage_limit,
-			   img->nr - (applied_pos + preimage_limit));
-	COPY_ARRAY(img->line + applied_pos, postimage->line, postimage->nr);
+			   img->line_nr - (applied_pos + preimage_limit));
+	COPY_ARRAY(img->line + applied_pos, postimage->line, postimage->line_nr);
 	if (!state->allow_overlap)
-		for (i = 0; i < postimage->nr; i++)
+		for (i = 0; i < postimage->line_nr; i++)
 			img->line[applied_pos + i].flag |= LINE_PATCHED;
-	img->nr = nr;
+	img->line_nr = nr;
 }
 
 /*
@@ -3024,8 +3023,8 @@ static int apply_one_fragment(struct apply_state *state,
 	    newlines.len > 0 && newlines.buf[newlines.len - 1] == '\n') {
 		old--;
 		strbuf_setlen(&newlines, newlines.len - 1);
-		preimage.line[preimage.nr - 1].len--;
-		postimage.line[postimage.nr - 1].len--;
+		preimage.line[preimage.line_nr - 1].len--;
+		postimage.line[postimage.line_nr - 1].len--;
 	}
 
 	leading = frag->leading;
@@ -3096,7 +3095,7 @@ static int apply_one_fragment(struct apply_state *state,
 
 	if (applied_pos >= 0) {
 		if (new_blank_lines_at_end &&
-		    preimage.nr + applied_pos >= img->nr &&
+		    preimage.line_nr + applied_pos >= img->line_nr &&
 		    (ws_rule & WS_BLANK_AT_EOF) &&
 		    state->ws_error_action != nowarn_ws_error) {
 			record_ws_error(state, WS_BLANK_AT_EOF, "+", 1,
-- 
2.46.0.551.gc5ee8f2d1c.dirty

[PATCH v2 6/6] apply: refactor `struct image` to use a `struct strbuf`

[Patrick Steinhardt]

The `struct image` uses a character array to track the pre- or postimage
of a patch operation. This has multiple downsides:

  - It is somewhat hard to track memory ownership. In fact, we have
    several memory leaks in git-apply(1) because we do not (and cannot
    easily) free the buffer in all situations.

  - We have to reinvent the wheel and manually implement a lot of
    functionality that would already be provided by `struct strbuf`.

  - We have to carefully track whether `update_pre_post_images()` can do
    an in-place update of the postimage or whether it has to allocate a
    new buffer for it.

This is all rather cumbersome, and especially `update_pre_post_images()`
is really hard to understand as a consequence even though what it is
doing is rather trivial.

Refactor the code to use a `struct strbuf` instead, addressing all of
the above. Like this we can easily perform in-place updates in all
situations, the logic to perform those updates becomes way simpler and
the lifetime of the buffer becomes a ton easier to track.

This refactoring also plugs some leaking buffers as a side effect.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 apply.c                            | 194 +++++++++++------------------
 t/t3436-rebase-more-options.sh     |   1 +
 t/t4107-apply-ignore-whitespace.sh |   4 +-
 t/t4124-apply-ws-rule.sh           |   1 +
 t/t4125-apply-ws-fuzz.sh           |   1 +
 t/t4138-apply-ws-expansion.sh      |   1 +
 6 files changed, 79 insertions(+), 123 deletions(-)

diff --git a/apply.c b/apply.c
index 3340bb1fc0..06a75ea034 100644
--- a/apply.c
+++ b/apply.c
@@ -277,12 +277,13 @@ struct line {
  * This represents a "file", which is an array of "lines".
  */
 struct image {
-	char *buf;
-	size_t len;
+	struct strbuf buf;
 	struct line *line;
 	size_t line_nr, line_alloc;
 };
-#define IMAGE_INIT { 0 }
+#define IMAGE_INIT { \
+	.buf = STRBUF_INIT, \
+}
 
 static void image_init(struct image *image)
 {
@@ -292,7 +293,7 @@ static void image_init(struct image *image)
 
 static void image_clear(struct image *image)
 {
-	free(image->buf);
+	strbuf_release(&image->buf);
 	free(image->line);
 	image_init(image);
 }
@@ -329,14 +330,13 @@ static void image_prepare(struct image *image, char *buf, size_t len,
 	const char *cp, *ep;
 
 	image_clear(image);
-	image->buf = buf;
-	image->len = len;
+	strbuf_attach(&image->buf, buf, len, len + 1);
 
 	if (!prepare_linetable)
 		return;
 
-	ep = image->buf + image->len;
-	cp = image->buf;
+	ep = image->buf.buf + image->buf.len;
+	cp = image->buf.buf;
 	while (cp < ep) {
 		const char *next;
 		for (next = cp; next < ep && *next != '\n'; next++)
@@ -350,8 +350,7 @@ static void image_prepare(struct image *image, char *buf, size_t len,
 
 static void image_remove_first_line(struct image *img)
 {
-	img->buf += img->line[0].len;
-	img->len -= img->line[0].len;
+	strbuf_remove(&img->buf, 0, img->line[0].len);
 	img->line_nr--;
 	if (img->line_nr)
 		MOVE_ARRAY(img->line, img->line + 1, img->line_nr);
@@ -359,7 +358,9 @@ static void image_remove_first_line(struct image *img)
 
 static void image_remove_last_line(struct image *img)
 {
-	img->len -= img->line[--img->line_nr].len;
+	size_t last_line_len = img->line[img->line_nr - 1].len;
+	strbuf_setlen(&img->buf, img->buf.len - last_line_len);
+	img->line_nr--;
 }
 
 /* fmt must contain _one_ %s and no other substitution */
@@ -2308,19 +2309,16 @@ static int read_old_data(struct stat *st, struct patch *patch,
 
 /*
  * Update the preimage, and the common lines in postimage,
- * from buffer buf of length len. If postlen is 0 the postimage
- * is updated in place, otherwise it's updated on a new buffer
- * of length postlen
+ * from buffer buf of length len.
  */
-
 static void update_pre_post_images(struct image *preimage,
 				   struct image *postimage,
-				   char *buf,
-				   size_t len, size_t postlen)
+				   char *buf, size_t len)
 {
-	int i, ctx, reduced;
-	char *new_buf, *old_buf, *fixed;
 	struct image fixed_preimage = IMAGE_INIT;
+	size_t insert_pos = 0;
+	int i, ctx, reduced;
+	const char *fixed;
 
 	/*
 	 * Update the preimage with whitespace fixes.  Note that we
@@ -2328,43 +2326,24 @@ static void update_pre_post_images(struct image *preimage,
 	 * free "oldlines".
 	 */
 	image_prepare(&fixed_preimage, buf, len, 1);
-	assert(postlen
-	       ? fixed_preimage.line_nr == preimage->line_nr
-	       : fixed_preimage.line_nr <= preimage->line_nr);
 	for (i = 0; i < fixed_preimage.line_nr; i++)
 		fixed_preimage.line[i].flag = preimage->line[i].flag;
-	free(preimage->line);
+	image_clear(preimage);
 	*preimage = fixed_preimage;
+	fixed = preimage->buf.buf;
 
 	/*
-	 * Adjust the common context lines in postimage. This can be
-	 * done in-place when we are shrinking it with whitespace
-	 * fixing, but needs a new buffer when ignoring whitespace or
-	 * expanding leading tabs to spaces.
-	 *
-	 * We trust the caller to tell us if the update can be done
-	 * in place (postlen==0) or not.
+	 * Adjust the common context lines in postimage.
 	 */
-	old_buf = postimage->buf;
-	if (postlen)
-		new_buf = postimage->buf = xmalloc(postlen);
-	else
-		new_buf = old_buf;
-	fixed = preimage->buf;
-
 	for (i = reduced = ctx = 0; i < postimage->line_nr; i++) {
 		size_t l_len = postimage->line[i].len;
+
 		if (!(postimage->line[i].flag & LINE_COMMON)) {
 			/* an added line -- no counterparts in preimage */
-			memmove(new_buf, old_buf, l_len);
-			old_buf += l_len;
-			new_buf += l_len;
+			insert_pos += l_len;
 			continue;
 		}
 
-		/* a common context -- skip it in the original postimage */
-		old_buf += l_len;
-
 		/* and find the corresponding one in the fixed preimage */
 		while (ctx < preimage->line_nr &&
 		       !(preimage->line[ctx].flag & LINE_COMMON)) {
@@ -2383,21 +2362,15 @@ static void update_pre_post_images(struct image *preimage,
 
 		/* and copy it in, while fixing the line length */
 		l_len = preimage->line[ctx].len;
-		memcpy(new_buf, fixed, l_len);
-		new_buf += l_len;
+		strbuf_splice(&postimage->buf, insert_pos, postimage->line[i].len,
+			      fixed, l_len);
+		insert_pos += l_len;
 		fixed += l_len;
 		postimage->line[i].len = l_len;
 		ctx++;
 	}
 
-	if (postlen
-	    ? postlen < new_buf - postimage->buf
-	    : postimage->len < new_buf - postimage->buf)
-		BUG("caller miscounted postlen: asked %d, orig = %d, used = %d",
-		    (int)postlen, (int) postimage->len, (int)(new_buf - postimage->buf));
-
 	/* Fix the length of the whole thing */
-	postimage->len = new_buf - postimage->buf;
 	postimage->line_nr -= reduced;
 }
 
@@ -2447,7 +2420,6 @@ static int line_by_line_fuzzy_match(struct image *img,
 	int i;
 	size_t imgoff = 0;
 	size_t preoff = 0;
-	size_t postlen = postimage->len;
 	size_t extra_chars;
 	char *buf;
 	char *preimage_eof;
@@ -2460,11 +2432,9 @@ static int line_by_line_fuzzy_match(struct image *img,
 		size_t prelen = preimage->line[i].len;
 		size_t imglen = img->line[current_lno+i].len;
 
-		if (!fuzzy_matchlines(img->buf + current + imgoff, imglen,
-				      preimage->buf + preoff, prelen))
+		if (!fuzzy_matchlines(img->buf.buf + current + imgoff, imglen,
+				      preimage->buf.buf + preoff, prelen))
 			return 0;
-		if (preimage->line[i].flag & LINE_COMMON)
-			postlen += imglen - prelen;
 		imgoff += imglen;
 		preoff += prelen;
 	}
@@ -2480,10 +2450,10 @@ static int line_by_line_fuzzy_match(struct image *img,
 	 * are whitespace characters. (This can only happen if
 	 * we are removing blank lines at the end of the file.)
 	 */
-	buf = preimage_eof = preimage->buf + preoff;
+	buf = preimage_eof = preimage->buf.buf + preoff;
 	for ( ; i < preimage->line_nr; i++)
 		preoff += preimage->line[i].len;
-	preimage_end = preimage->buf + preoff;
+	preimage_end = preimage->buf.buf + preoff;
 	for ( ; buf < preimage_end; buf++)
 		if (!isspace(*buf))
 			return 0;
@@ -2497,11 +2467,11 @@ static int line_by_line_fuzzy_match(struct image *img,
 	 */
 	extra_chars = preimage_end - preimage_eof;
 	strbuf_init(&fixed, imgoff + extra_chars);
-	strbuf_add(&fixed, img->buf + current, imgoff);
+	strbuf_add(&fixed, img->buf.buf + current, imgoff);
 	strbuf_add(&fixed, preimage_eof, extra_chars);
 	fixed_buf = strbuf_detach(&fixed, &fixed_len);
 	update_pre_post_images(preimage, postimage,
-			       fixed_buf, fixed_len, postlen);
+			       fixed_buf, fixed_len);
 	return 1;
 }
 
@@ -2517,7 +2487,8 @@ static int match_fragment(struct apply_state *state,
 	int i;
 	const char *orig, *target;
 	struct strbuf fixed = STRBUF_INIT;
-	size_t postlen;
+	char *fixed_buf;
+	size_t fixed_len;
 	int preimage_limit;
 	int ret;
 
@@ -2573,9 +2544,9 @@ static int match_fragment(struct apply_state *state,
 		 * exactly.
 		 */
 		if ((match_end
-		     ? (current + preimage->len == img->len)
-		     : (current + preimage->len <= img->len)) &&
-		    !memcmp(img->buf + current, preimage->buf, preimage->len)) {
+		     ? (current + preimage->buf.len == img->buf.len)
+		     : (current + preimage->buf.len <= img->buf.len)) &&
+		    !memcmp(img->buf.buf + current, preimage->buf.buf, preimage->buf.len)) {
 			ret = 1;
 			goto out;
 		}
@@ -2589,7 +2560,7 @@ static int match_fragment(struct apply_state *state,
 		 */
 		const char *buf, *buf_end;
 
-		buf = preimage->buf;
+		buf = preimage->buf.buf;
 		buf_end = buf;
 		for (i = 0; i < preimage_limit; i++)
 			buf_end += preimage->line[i].len;
@@ -2634,21 +2605,14 @@ static int match_fragment(struct apply_state *state,
 	 * fixed.
 	 */
 
-	/* First count added lines in postimage */
-	postlen = 0;
-	for (i = 0; i < postimage->line_nr; i++) {
-		if (!(postimage->line[i].flag & LINE_COMMON))
-			postlen += postimage->line[i].len;
-	}
-
 	/*
 	 * The preimage may extend beyond the end of the file,
 	 * but in this loop we will only handle the part of the
 	 * preimage that falls within the file.
 	 */
-	strbuf_grow(&fixed, preimage->len + 1);
-	orig = preimage->buf;
-	target = img->buf + current;
+	strbuf_grow(&fixed, preimage->buf.len + 1);
+	orig = preimage->buf.buf;
+	target = img->buf.buf + current;
 	for (i = 0; i < preimage_limit; i++) {
 		size_t oldlen = preimage->line[i].len;
 		size_t tgtlen = img->line[current_lno + i].len;
@@ -2677,10 +2641,6 @@ static int match_fragment(struct apply_state *state,
 			 !memcmp(tgtfix.buf, fixed.buf + fixstart,
 					     fixed.len - fixstart));
 
-		/* Add the length if this is common with the postimage */
-		if (preimage->line[i].flag & LINE_COMMON)
-			postlen += tgtfix.len;
-
 		strbuf_release(&tgtfix);
 		if (!match) {
 			ret = 0;
@@ -2722,10 +2682,9 @@ static int match_fragment(struct apply_state *state,
 	 * has whitespace breakages unfixed, and fixing them makes the
 	 * hunk match.  Update the context lines in the postimage.
 	 */
-	if (postlen < postimage->len)
-		postlen = 0;
+	fixed_buf = strbuf_detach(&fixed, &fixed_len);
 	update_pre_post_images(preimage, postimage,
-			       fixed.buf, fixed.len, postlen);
+			       fixed_buf, fixed_len);
 
 	ret = 1;
 
@@ -2839,6 +2798,7 @@ static void update_image(struct apply_state *state,
 	 */
 	int i, nr;
 	size_t remove_count, insert_count, applied_at = 0;
+	size_t result_alloc;
 	char *result;
 	int preimage_limit;
 
@@ -2861,19 +2821,18 @@ static void update_image(struct apply_state *state,
 	remove_count = 0;
 	for (i = 0; i < preimage_limit; i++)
 		remove_count += img->line[applied_pos + i].len;
-	insert_count = postimage->len;
+	insert_count = postimage->buf.len;
 
 	/* Adjust the contents */
-	result = xmalloc(st_add3(st_sub(img->len, remove_count), insert_count, 1));
-	memcpy(result, img->buf, applied_at);
-	memcpy(result + applied_at, postimage->buf, postimage->len);
-	memcpy(result + applied_at + postimage->len,
-	       img->buf + (applied_at + remove_count),
-	       img->len - (applied_at + remove_count));
-	free(img->buf);
-	img->buf = result;
-	img->len += insert_count - remove_count;
-	result[img->len] = '\0';
+	result_alloc = st_add3(st_sub(img->buf.len, remove_count), insert_count, 1);
+	result = xmalloc(result_alloc);
+	memcpy(result, img->buf.buf, applied_at);
+	memcpy(result + applied_at, postimage->buf.buf, postimage->buf.len);
+	memcpy(result + applied_at + postimage->buf.len,
+	       img->buf.buf + (applied_at + remove_count),
+	       img->buf.len - (applied_at + remove_count));
+	strbuf_attach(&img->buf, result, postimage->buf.len + img->buf.len - remove_count,
+		      result_alloc);
 
 	/* Adjust the line table */
 	nr = img->line_nr + postimage->line_nr - preimage_limit;
@@ -3054,10 +3013,8 @@ static int apply_one_fragment(struct apply_state *state,
 	match_end = !state->unidiff_zero && !trailing;
 
 	pos = frag->newpos ? (frag->newpos - 1) : 0;
-	preimage.buf = oldlines;
-	preimage.len = old - oldlines;
-	postimage.buf = newlines.buf;
-	postimage.len = newlines.len;
+	strbuf_add(&preimage.buf, oldlines, old - oldlines);
+	strbuf_swap(&postimage.buf, &newlines);
 
 	for (;;) {
 
@@ -3145,8 +3102,8 @@ static int apply_one_fragment(struct apply_state *state,
 out:
 	free(oldlines);
 	strbuf_release(&newlines);
-	free(preimage.line);
-	free(postimage.line);
+	image_clear(&preimage);
+	image_clear(&postimage);
 
 	return (applied_pos < 0);
 }
@@ -3176,18 +3133,16 @@ static int apply_binary_fragment(struct apply_state *state,
 	}
 	switch (fragment->binary_patch_method) {
 	case BINARY_DELTA_DEFLATED:
-		dst = patch_delta(img->buf, img->len, fragment->patch,
+		dst = patch_delta(img->buf.buf, img->buf.len, fragment->patch,
 				  fragment->size, &len);
 		if (!dst)
 			return -1;
 		image_clear(img);
-		img->buf = dst;
-		img->len = len;
+		strbuf_attach(&img->buf, dst, len, len + 1);
 		return 0;
 	case BINARY_LITERAL_DEFLATED:
 		image_clear(img);
-		img->len = fragment->size;
-		img->buf = xmemdupz(fragment->patch, img->len);
+		strbuf_add(&img->buf, fragment->patch, fragment->size);
 		return 0;
 	}
 	return -1;
@@ -3223,8 +3178,8 @@ static int apply_binary(struct apply_state *state,
 		 * See if the old one matches what the patch
 		 * applies to.
 		 */
-		hash_object_file(the_hash_algo, img->buf, img->len, OBJ_BLOB,
-				 &oid);
+		hash_object_file(the_hash_algo, img->buf.buf, img->buf.len,
+				 OBJ_BLOB, &oid);
 		if (strcmp(oid_to_hex(&oid), patch->old_oid_prefix))
 			return error(_("the patch applies to '%s' (%s), "
 				       "which does not match the "
@@ -3233,7 +3188,7 @@ static int apply_binary(struct apply_state *state,
 	}
 	else {
 		/* Otherwise, the old one must be empty. */
-		if (img->len)
+		if (img->buf.len)
 			return error(_("the patch applies to an empty "
 				       "'%s' but it is not empty"), name);
 	}
@@ -3257,8 +3212,7 @@ static int apply_binary(struct apply_state *state,
 				       "'%s' cannot be read"),
 				     patch->new_oid_prefix, name);
 		image_clear(img);
-		img->buf = result;
-		img->len = size;
+		strbuf_attach(&img->buf, result, size, size + 1);
 	} else {
 		/*
 		 * We have verified buf matches the preimage;
@@ -3270,7 +3224,7 @@ static int apply_binary(struct apply_state *state,
 				     name);
 
 		/* verify that the result matches */
-		hash_object_file(the_hash_algo, img->buf, img->len, OBJ_BLOB,
+		hash_object_file(the_hash_algo, img->buf.buf, img->buf.len, OBJ_BLOB,
 				 &oid);
 		if (strcmp(oid_to_hex(&oid), patch->new_oid_prefix))
 			return error(_("binary patch to '%s' creates incorrect result (expecting %s, got %s)"),
@@ -3540,14 +3494,14 @@ static int resolve_to(struct image *image, const struct object_id *result_id)
 {
 	unsigned long size;
 	enum object_type type;
+	char *data;
 
 	image_clear(image);
 
-	image->buf = repo_read_object_file(the_repository, result_id, &type,
-					   &size);
-	if (!image->buf || type != OBJ_BLOB)
+	data = repo_read_object_file(the_repository, result_id, &type, &size);
+	if (!data || type != OBJ_BLOB)
 		die("unable to read blob object %s", oid_to_hex(result_id));
-	image->len = size;
+	strbuf_attach(&image->buf, data, size, size + 1);
 
 	return 0;
 }
@@ -3589,8 +3543,7 @@ static int three_way_merge(struct apply_state *state,
 		return -1;
 	}
 	image_clear(image);
-	image->buf = result.ptr;
-	image->len = result.size;
+	strbuf_attach(&image->buf, result.ptr, result.size, result.size);
 
 	return status;
 }
@@ -3677,7 +3630,7 @@ static int try_threeway(struct apply_state *state,
 		return -1;
 	}
 	/* post_oid is theirs */
-	write_object_file(tmp_image.buf, tmp_image.len, OBJ_BLOB, &post_oid);
+	write_object_file(tmp_image.buf.buf, tmp_image.buf.len, OBJ_BLOB, &post_oid);
 	image_clear(&tmp_image);
 
 	/* our_oid is ours */
@@ -3690,7 +3643,7 @@ static int try_threeway(struct apply_state *state,
 			return error(_("cannot read the current contents of '%s'"),
 				     patch->old_name);
 	}
-	write_object_file(tmp_image.buf, tmp_image.len, OBJ_BLOB, &our_oid);
+	write_object_file(tmp_image.buf.buf, tmp_image.buf.len, OBJ_BLOB, &our_oid);
 	image_clear(&tmp_image);
 
 	/* in-core three-way merge between post and our using pre as base */
@@ -3743,8 +3696,7 @@ static int apply_data(struct apply_state *state, struct patch *patch,
 			return -1;
 		}
 	}
-	patch->result = image.buf;
-	patch->resultsize = image.len;
+	patch->result = strbuf_detach(&image.buf, &patch->resultsize);
 	add_to_fn_table(state, patch);
 	free(image.line);
 
diff --git a/t/t3436-rebase-more-options.sh b/t/t3436-rebase-more-options.sh
index 94671d3c46..4d9744e5fc 100755
--- a/t/t3436-rebase-more-options.sh
+++ b/t/t3436-rebase-more-options.sh
@@ -5,6 +5,7 @@
 
 test_description='tests to ensure compatibility between am and interactive backends'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 . "$TEST_DIRECTORY"/lib-rebase.sh
diff --git a/t/t4107-apply-ignore-whitespace.sh b/t/t4107-apply-ignore-whitespace.sh
index ac72eeaf27..5e6e203aa5 100755
--- a/t/t4107-apply-ignore-whitespace.sh
+++ b/t/t4107-apply-ignore-whitespace.sh
@@ -3,9 +3,9 @@
 # Copyright (c) 2009 Giuseppe Bilotta
 #
 
-test_description='git-apply --ignore-whitespace.
+test_description='git-apply --ignore-whitespace.'
 
-'
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 # This primes main.c file that indents without using HT at all.
diff --git a/t/t4124-apply-ws-rule.sh b/t/t4124-apply-ws-rule.sh
index 485c7d2d12..cdffee0273 100755
--- a/t/t4124-apply-ws-rule.sh
+++ b/t/t4124-apply-ws-rule.sh
@@ -2,6 +2,7 @@
 
 test_description='core.whitespace rules and git apply'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 prepare_test_file () {
diff --git a/t/t4125-apply-ws-fuzz.sh b/t/t4125-apply-ws-fuzz.sh
index 090987c89b..f248cc2a00 100755
--- a/t/t4125-apply-ws-fuzz.sh
+++ b/t/t4125-apply-ws-fuzz.sh
@@ -2,6 +2,7 @@
 
 test_description='applying patch that has broken whitespaces in context'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success setup '
diff --git a/t/t4138-apply-ws-expansion.sh b/t/t4138-apply-ws-expansion.sh
index 8bbf8260fa..7981931b4e 100755
--- a/t/t4138-apply-ws-expansion.sh
+++ b/t/t4138-apply-ws-expansion.sh
@@ -5,6 +5,7 @@
 
 test_description='git apply test patches with whitespace expansion.'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success setup '
-- 
2.46.0.551.gc5ee8f2d1c.dirty

Re: [PATCH v2 0/6] apply: fix leaking buffer of `struct image`

[Patrick Steinhardt]

On Tue, Sep 17, 2024 at 12:07:48PM +0200, Patrick Steinhardt wrote:
> Hi,
> 
> this is the second version of my patch series that refactors lifecycle
> management of `struct image` in "apply.c" to plug a bunch of memory
> leaks.
> 
> Changes compared to v1:
>  
>   - Fix two typos.
> 
>   - Correct the statement that we don't loop around
>     `image_remove_first_line()`. No idea how I missed that loop.
> 
>   - Split up an overly complex line into multiple lines.
> 
> Thanks!

Ugh, sorry. I meant to Cc Junio, not Taylor.

Patrick

Re: [PATCH v2 0/6] apply: fix leaking buffer of `struct image`

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> this is the second version of my patch series that refactors lifecycle
> management of `struct image` in "apply.c" to plug a bunch of memory
> leaks.
>
> Changes compared to v1:
>  
>   - Fix two typos.
>
>   - Correct the statement that we don't loop around
>     `image_remove_first_line()`. No idea how I missed that loop.
>
>   - Split up an overly complex line into multiple lines.

Looking good.  Will queue.  Thanks.