[PATCH 0/5] use size_t for xdiff mmfile_t

[PATCH 0/5] use size_t for xdiff mmfile_t

[Jeff King]

An earlier series tried to simplify ll_ext_merge()'s code to read back
the merge result from a temporary file, but Elijah pointed out some
subtle integer overflow confusion:

  https://lore.kernel.org/git/CABPp-BG9Hkc7i_JxAbYfyzu+b4Mc_pZUr0jJF=vY0jHSARpHzw@mail.gmail.com/

I dug a little bit and found that similar problems exist elsewhere. So
here's an attempt to make things at least incrementally better. And
patch 4 is the original cleanup I set out to do. ;)

There are a few textual conflicts with the v3 of
jk/merge-ll-tempfile-cleanup that I just sent out. They should be
easy-ish to resolve, but I'm happy to just base this on that topic if
it's easier.

  [1/5]: xdiff: clean up read_mmfile() allocations on error
  [2/5]: xdiff: replace mmbuffer_t with mmfile_t
  [3/5]: xdiff: use size_t for buffer sizes
  [4/5]: merge-ll: use read_mmfile() to read external merge results
  [5/5]: xdiff: NUL-terminate buffers read by read_mmfile()

 Documentation/technical/api-merge.adoc |  7 +++---
 apply.c                                |  2 +-
 builtin/checkout.c                     |  2 +-
 builtin/merge-file.c                   |  2 +-
 builtin/merge-tree.c                   |  2 +-
 builtin/rerere.c                       |  8 +++++--
 diff.c                                 |  2 +-
 merge-blobs.c                          |  2 +-
 merge-ll.c                             | 33 +++++++++-----------------
 merge-ll.h                             |  4 ++--
 merge-ort.c                            |  4 ++--
 notes-merge.c                          |  2 +-
 rerere.c                               | 11 ++++-----
 xdiff-interface.c                      |  5 ++--
 xdiff/xdiff.h                          | 11 +++------
 xdiff/xmerge.c                         |  4 ++--
 xdiff/xutils.c                         |  4 ++--
 17 files changed, 46 insertions(+), 59 deletions(-)

-Peff

[PATCH 1/5] xdiff: clean up read_mmfile() allocations on error

[Jeff King]

When read_mmfile() returns an error, it may or may not have allocated a
buffer in the passed-in mmfile_t. So callers must initialize the pointer
to NULL and free it even on error.

Most callers do this already, but rerere's diff_two() does not, and
would leak the buffer after a read error. We could fix it directly, but
let's instead try to make the interface less error-prone by freeing the
memory when returning failure from read_mmfile().

This fixes (part of) the leak in diff_two(). In theory it also lets us
simplify other callers to skip initializing the mmfile. But in practice
most still need zero-initialization because they may jump to free()
before even calling read_mmfile (e.g., in try_merge()). But we can at
least simplify rerere_forget_one_path() a bit.

I said "part of" earlier. There's a related leak in diff_two(): if
reading the first file succeeds but reading the second fails, we return
early and leak the first buffer. We can fix that by checking each
individually.

Signed-off-by: Jeff King <peff@peff.net>
---
I found this while reading the code, but never actually triggered it in
practice. It would require some way of having fopen() succeed and
fread() fail.

 builtin/rerere.c  | 6 +++++-
 rerere.c          | 3 +--
 xdiff-interface.c | 1 +
 3 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/builtin/rerere.c b/builtin/rerere.c
index a056cb791b..d39c6e8445 100644
--- a/builtin/rerere.c
+++ b/builtin/rerere.c
@@ -34,8 +34,12 @@ static int diff_two(const char *file1, const char *label1,
 	mmfile_t minus, plus;
 	int ret;
 
-	if (read_mmfile(&minus, file1) || read_mmfile(&plus, file2))
+	if (read_mmfile(&minus, file1))
 		return -1;
+	if (read_mmfile(&plus, file2)) {
+		free(minus.ptr);
+		return -1;
+	}
 
 	printf("--- a/%s\n+++ b/%s\n", label1, label2);
 	fflush(stdout);
diff --git a/rerere.c b/rerere.c
index 1c3745d9e3..856347c9ae 100644
--- a/rerere.c
+++ b/rerere.c
@@ -1039,7 +1039,7 @@ static int rerere_forget_one_path(struct index_state *istate,
 	for (id->variant = 0;
 	     id->variant < id->collection->status_nr;
 	     id->variant++) {
-		mmfile_t cur = { NULL, 0 };
+		mmfile_t cur;
 		mmbuffer_t result = {NULL, 0};
 		int cleanly_resolved;
 
@@ -1048,7 +1048,6 @@ static int rerere_forget_one_path(struct index_state *istate,
 
 		handle_cache(istate, path, hash, rerere_path(&buf, id, "thisimage"));
 		if (read_mmfile(&cur, rerere_path(&buf, id, "thisimage"))) {
-			free(cur.ptr);
 			error(_("failed to update conflicted state in '%s'"), path);
 			goto fail_exit;
 		}
diff --git a/xdiff-interface.c b/xdiff-interface.c
index db6938689f..e3dd2184ae 100644
--- a/xdiff-interface.c
+++ b/xdiff-interface.c
@@ -168,6 +168,7 @@ int read_mmfile(mmfile_t *ptr, const char *filename)
 	sz = xsize_t(st.st_size);
 	ptr->ptr = xmalloc(sz ? sz : 1);
 	if (sz && fread(ptr->ptr, sz, 1, f) != 1) {
+		FREE_AND_NULL(ptr->ptr);
 		fclose(f);
 		return error("Could not read %s", filename);
 	}
-- 
2.56.0.325.g545d7e68bc

Re: [PATCH 1/5] xdiff: clean up read_mmfile() allocations on error

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> When read_mmfile() returns an error, it may or may not have allocated a
> buffer in the passed-in mmfile_t. So callers must initialize the pointer
> to NULL and free it even on error.
>
> Most callers do this already, but rerere's diff_two() does not, and
> would leak the buffer after a read error. We could fix it directly, but
> let's instead try to make the interface less error-prone by freeing the
> memory when returning failure from read_mmfile().
>
> This fixes (part of) the leak in diff_two(). In theory it also lets us
> simplify other callers to skip initializing the mmfile. But in practice
> most still need zero-initialization because they may jump to free()
> before even calling read_mmfile (e.g., in try_merge()). But we can at
> least simplify rerere_forget_one_path() a bit.
>
> I said "part of" earlier. There's a related leak in diff_two(): if
> reading the first file succeeds but reading the second fails, we return
> early and leak the first buffer. We can fix that by checking each
> individually.

Nice.  Thanks for plugging my leaks.

[PATCH 2/5] xdiff: replace mmbuffer_t with mmfile_t

[Jeff King]

Our import of xdiff has two identical buffer structures: mmfile_t and
mmbuffer_t. In upstream xdiff these were actually different, but the
import in 3443546f6e (Use a *real* built-in diff generator, 2006-03-24)
simplified mmfile_t to a simple buffer.

In xdiff we usually use mmfile_t for input and mmbuffer_t for output,
but they are really both just a ptr/len pair. I don't think that having
different types is buying us anything in terms of type safety or
semantics, and having two makes it awkward to use the same helpers for
both. In particular, an external merge driver's output is read from a
file, but we can't easily use read_mmfile(), since we want the result in
an mmbuffer_t.

Let's use mmfile_t for both cases and drop mmbuffer_t. The latter is
probably a more descriptive name, but we have many more uses of
mmfile_t (and helpers like read_mmfile). So let's consolidate using that
name; we can always change it to something more sensible later.

There should be no behavior change here; this is just consolidating the
types.

Signed-off-by: Jeff King <peff@peff.net>
---
I guess this step might be controversial, but I hope not. I think the
ship has long sailed on trying to pull "upstream" changes from xdiff
(there haven't been any, and we've hacked it up quite a bit already).

 Documentation/technical/api-merge.adoc |  7 +++----
 apply.c                                |  2 +-
 builtin/checkout.c                     |  2 +-
 builtin/merge-file.c                   |  2 +-
 builtin/merge-tree.c                   |  2 +-
 builtin/rerere.c                       |  2 +-
 merge-blobs.c                          |  2 +-
 merge-ll.c                             | 12 ++++++------
 merge-ll.h                             |  4 ++--
 merge-ort.c                            |  4 ++--
 notes-merge.c                          |  2 +-
 rerere.c                               |  8 ++++----
 xdiff-interface.c                      |  2 +-
 xdiff/xdiff.h                          |  9 ++-------
 xdiff/xmerge.c                         |  4 ++--
 xdiff/xutils.c                         |  4 ++--
 16 files changed, 31 insertions(+), 37 deletions(-)

diff --git a/Documentation/technical/api-merge.adoc b/Documentation/technical/api-merge.adoc
index c2ba01828c..b691599393 100644
--- a/Documentation/technical/api-merge.adoc
+++ b/Documentation/technical/api-merge.adoc
@@ -20,11 +20,10 @@ responsible for a few things.
 Data structures
 ---------------
 
-* `mmbuffer_t`, `mmfile_t`
+* `mmfile_t`
 
-These store data usable for use by the xdiff backend, for writing and
-for reading, respectively.  See `xdiff/xdiff.h` for the definitions
-and `diff.c` for examples.
+This stores a buffer and its size for input to or output from the xdiff
+backend. See `xdiff/xdiff.h` for the definition and `diff.c` for examples.
 
 * `struct ll_merge_options`
 
diff --git a/apply.c b/apply.c
index f00b7ba4d3..faf3c1dba0 100644
--- a/apply.c
+++ b/apply.c
@@ -3646,7 +3646,7 @@ static int three_way_merge(struct apply_state *state,
 {
 	mmfile_t base_file, our_file, their_file;
 	struct ll_merge_options merge_opts = LL_MERGE_OPTIONS_INIT;
-	mmbuffer_t result = { NULL };
+	mmfile_t result = { NULL };
 	enum ll_merge_result status;
 
 	/* resolve trivial cases first */
diff --git a/builtin/checkout.c b/builtin/checkout.c
index c0f0d2c700..2d575a4f57 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -320,7 +320,7 @@ static int checkout_merged(int pos, const struct checkout *state,
 	enum ll_merge_result merge_status;
 	int status;
 	struct object_id oid;
-	mmbuffer_t result_buf;
+	mmfile_t result_buf;
 	struct object_id threeway[3];
 	unsigned mode = 0;
 	struct ll_merge_options ll_opts = LL_MERGE_OPTIONS_INIT;
diff --git a/builtin/merge-file.c b/builtin/merge-file.c
index 8fa5765239..ddca408c46 100644
--- a/builtin/merge-file.c
+++ b/builtin/merge-file.c
@@ -64,7 +64,7 @@ int cmd_merge_file(int argc,
 {
 	const char *names[3] = { 0 };
 	mmfile_t mmfs[3] = { 0 };
-	mmbuffer_t result = { 0 };
+	mmfile_t result = { 0 };
 	xmparam_t xmp = { 0 };
 	int ret = 0, i = 0, to_stdout = 0, object_id = 0;
 	int quiet = 0;
diff --git a/builtin/merge-tree.c b/builtin/merge-tree.c
index 49f41e520f..552c2ad736 100644
--- a/builtin/merge-tree.c
+++ b/builtin/merge-tree.c
@@ -109,7 +109,7 @@ static void *origin(struct merge_list *entry, size_t *size)
 	return NULL;
 }
 
-static int show_outf(void *priv UNUSED, mmbuffer_t *mb, int nbuf)
+static int show_outf(void *priv UNUSED, mmfile_t *mb, int nbuf)
 {
 	int i;
 	for (i = 0; i < nbuf; i++)
diff --git a/builtin/rerere.c b/builtin/rerere.c
index d39c6e8445..ef03b79f5b 100644
--- a/builtin/rerere.c
+++ b/builtin/rerere.c
@@ -16,7 +16,7 @@ static const char * const rerere_usage[] = {
 	NULL,
 };
 
-static int outf(void *dummy UNUSED, mmbuffer_t *ptr, int nbuf)
+static int outf(void *dummy UNUSED, mmfile_t *ptr, int nbuf)
 {
 	int i;
 	for (i = 0; i < nbuf; i++)
diff --git a/merge-blobs.c b/merge-blobs.c
index 16a75bd1e3..49dbec9529 100644
--- a/merge-blobs.c
+++ b/merge-blobs.c
@@ -38,7 +38,7 @@ static void *three_way_filemerge(struct index_state *istate,
 				 size_t *size)
 {
 	enum ll_merge_result merge_status;
-	mmbuffer_t res;
+	mmfile_t res;
 
 	/*
 	 * This function is only used by cmd_merge_tree, which
diff --git a/merge-ll.c b/merge-ll.c
index ef5287dee8..dfed6411a8 100644
--- a/merge-ll.c
+++ b/merge-ll.c
@@ -21,7 +21,7 @@
 struct ll_merge_driver;
 
 typedef enum ll_merge_result (*ll_merge_fn)(const struct ll_merge_driver *,
-			   mmbuffer_t *result,
+			   mmfile_t *result,
 			   const char *path,
 			   mmfile_t *orig, const char *orig_name,
 			   mmfile_t *src1, const char *name1,
@@ -56,7 +56,7 @@ void reset_merge_attributes(void)
  * Built-in low-levels
  */
 static enum ll_merge_result ll_binary_merge(const struct ll_merge_driver *drv UNUSED,
-			   mmbuffer_t *result,
+			   mmfile_t *result,
 			   const char *path UNUSED,
 			   mmfile_t *orig, const char *orig_name UNUSED,
 			   mmfile_t *src1, const char *name1 UNUSED,
@@ -101,7 +101,7 @@ static enum ll_merge_result ll_binary_merge(const struct ll_merge_driver *drv UN
 }
 
 static enum ll_merge_result ll_xdl_merge(const struct ll_merge_driver *drv_unused,
-			mmbuffer_t *result,
+			mmfile_t *result,
 			const char *path,
 			mmfile_t *orig, const char *orig_name,
 			mmfile_t *src1, const char *name1,
@@ -147,7 +147,7 @@ static enum ll_merge_result ll_xdl_merge(const struct ll_merge_driver *drv_unuse
 }
 
 static enum ll_merge_result ll_union_merge(const struct ll_merge_driver *drv_unused,
-			  mmbuffer_t *result,
+			  mmfile_t *result,
 			  const char *path,
 			  mmfile_t *orig, const char *orig_name,
 			  mmfile_t *src1, const char *name1,
@@ -189,7 +189,7 @@ static void create_temp(mmfile_t *src, char *path, size_t len)
  * User defined low-level merge driver support.
  */
 static enum ll_merge_result ll_ext_merge(const struct ll_merge_driver *fn,
-			mmbuffer_t *result,
+			mmfile_t *result,
 			const char *path,
 			mmfile_t *orig, const char *orig_name,
 			mmfile_t *src1, const char *name1,
@@ -403,7 +403,7 @@ static void normalize_file(mmfile_t *mm, const char *path, struct index_state *i
 	}
 }
 
-enum ll_merge_result ll_merge(mmbuffer_t *result_buf,
+enum ll_merge_result ll_merge(mmfile_t *result_buf,
 	     const char *path,
 	     mmfile_t *ancestor, const char *ancestor_label,
 	     mmfile_t *ours, const char *our_label,
diff --git a/merge-ll.h b/merge-ll.h
index f26aef238d..f95332c682 100644
--- a/merge-ll.h
+++ b/merge-ll.h
@@ -16,7 +16,7 @@
  *   If you have no special requests, skip this and pass `NULL`
  *   as the `opts` parameter to use the default options.
  *
- * - Allocate an mmbuffer_t variable for the result.
+ * - Allocate an mmfile_t variable for the result.
  *
  * - Allocate and fill variables with the file's original content
  *   and two modified versions (using `read_mmfile`, for example).
@@ -100,7 +100,7 @@ enum ll_merge_result {
  * `.gitattributes` or `.git/info/attributes` into account.
  * Returns 0 for a clean merge.
  */
-enum ll_merge_result ll_merge(mmbuffer_t *result_buf,
+enum ll_merge_result ll_merge(mmfile_t *result_buf,
 	     const char *path,
 	     mmfile_t *ancestor, const char *ancestor_label,
 	     mmfile_t *ours, const char *our_label,
diff --git a/merge-ort.c b/merge-ort.c
index c410a5d353..1d3d193d35 100644
--- a/merge-ort.c
+++ b/merge-ort.c
@@ -2111,7 +2111,7 @@ static int merge_3way(struct merge_options *opt,
 		      const struct object_id *b,
 		      const char *pathnames[3],
 		      const int extra_marker_size,
-		      mmbuffer_t *result_buf)
+		      mmfile_t *result_buf)
 {
 	mmfile_t orig, src1, src2;
 	struct ll_merge_options ll_opts = LL_MERGE_OPTIONS_INIT;
@@ -2247,7 +2247,7 @@ static int handle_content_merge(struct merge_options *opt,
 
 	/* Remaining rules depend on file vs. submodule vs. symlink. */
 	else if (S_ISREG(a->mode)) {
-		mmbuffer_t result_buf;
+		mmfile_t result_buf;
 		int ret = 0, merge_status;
 		int two_way;
 
diff --git a/notes-merge.c b/notes-merge.c
index 118cad2518..d361e70a48 100644
--- a/notes-merge.c
+++ b/notes-merge.c
@@ -355,7 +355,7 @@ static void write_note_to_worktree(const struct object_id *obj,
 static int ll_merge_in_worktree(struct notes_merge_options *o,
 				struct notes_merge_pair *p)
 {
-	mmbuffer_t result_buf;
+	mmfile_t result_buf;
 	mmfile_t base, local, remote;
 	enum ll_merge_result status;
 
diff --git a/rerere.c b/rerere.c
index 856347c9ae..8696f8e7b7 100644
--- a/rerere.c
+++ b/rerere.c
@@ -597,7 +597,7 @@ int rerere_remaining(struct repository *r, struct string_list *merge_rr)
  */
 static int try_merge(struct index_state *istate,
 		     const struct rerere_id *id, const char *path,
-		     mmfile_t *cur, mmbuffer_t *result)
+		     mmfile_t *cur, mmfile_t *result)
 {
 	enum ll_merge_result ret;
 	mmfile_t base = {NULL, 0}, other = {NULL, 0};
@@ -638,7 +638,7 @@ static int merge(struct index_state *istate, const struct rerere_id *id, const c
 	int ret;
 	struct strbuf buf = STRBUF_INIT;
 	mmfile_t cur = {NULL, 0};
-	mmbuffer_t result = {NULL, 0};
+	mmfile_t result = {NULL, 0};
 
 	/*
 	 * Normalize the conflicts in path and write it out to
@@ -947,7 +947,7 @@ static int handle_cache(struct index_state *istate,
 			const char *path, unsigned char *hash, const char *output)
 {
 	mmfile_t mmfile[3] = {{NULL}};
-	mmbuffer_t result = {NULL, 0};
+	mmfile_t result = {NULL, 0};
 	const struct cache_entry *ce;
 	int pos, len, i, has_conflicts;
 	struct rerere_io_mem io;
@@ -1040,7 +1040,7 @@ static int rerere_forget_one_path(struct index_state *istate,
 	     id->variant < id->collection->status_nr;
 	     id->variant++) {
 		mmfile_t cur;
-		mmbuffer_t result = {NULL, 0};
+		mmfile_t result = {NULL, 0};
 		int cleanly_resolved;
 
 		if (!has_rerere_resolution(id))
diff --git a/xdiff-interface.c b/xdiff-interface.c
index e3dd2184ae..bc340d5a8a 100644
--- a/xdiff-interface.c
+++ b/xdiff-interface.c
@@ -53,7 +53,7 @@ static int consume_one(void *priv_, char *s, unsigned long size)
 	return 0;
 }
 
-static int xdiff_outf(void *priv_, mmbuffer_t *mb, int nbuf)
+static int xdiff_outf(void *priv_, mmfile_t *mb, int nbuf)
 {
 	struct xdiff_emit_state *priv = priv_;
 	int i;
diff --git a/xdiff/xdiff.h b/xdiff/xdiff.h
index dc370712e9..334eb436f6 100644
--- a/xdiff/xdiff.h
+++ b/xdiff/xdiff.h
@@ -73,11 +73,6 @@ typedef struct s_mmfile {
 	long size;
 } mmfile_t;
 
-typedef struct s_mmbuffer {
-	char *ptr;
-	long size;
-} mmbuffer_t;
-
 typedef struct s_xpparam {
 	unsigned long flags;
 
@@ -96,7 +91,7 @@ typedef struct s_xdemitcb {
 			long old_begin, long old_nr,
 			long new_begin, long new_nr,
 			const char *func, long funclen);
-	int (*out_line)(void *, mmbuffer_t *, int);
+	int (*out_line)(void *, mmfile_t *, int);
 } xdemitcb_t;
 
 typedef long (*find_func_t)(const char *line, long line_len, char *buffer, long buffer_size, void *priv);
@@ -144,7 +139,7 @@ typedef struct s_xmparam {
 #define DEFAULT_CONFLICT_MARKER_SIZE 7
 
 int xdl_merge(mmfile_t *orig, mmfile_t *mf1, mmfile_t *mf2,
-		xmparam_t const *xmp, mmbuffer_t *result);
+		xmparam_t const *xmp, mmfile_t *result);
 
 #ifdef __cplusplus
 }
diff --git a/xdiff/xmerge.c b/xdiff/xmerge.c
index 659ad4ec97..7b37968d25 100644
--- a/xdiff/xmerge.c
+++ b/xdiff/xmerge.c
@@ -504,7 +504,7 @@ static int xdl_simplify_non_conflicts(xdfenv_t *xe1, xdmerge_t *m,
  */
 static int xdl_do_merge(xdfenv_t *xe1, xdchange_t *xscr1,
 		xdfenv_t *xe2, xdchange_t *xscr2,
-		xmparam_t const *xmp, mmbuffer_t *result)
+		xmparam_t const *xmp, mmfile_t *result)
 {
 	xdmerge_t *changes, *c;
 	xpparam_t const *xpp = &xmp->xpp;
@@ -682,7 +682,7 @@ static int xdl_do_merge(xdfenv_t *xe1, xdchange_t *xscr1,
 }
 
 int xdl_merge(mmfile_t *orig, mmfile_t *mf1, mmfile_t *mf2,
-		xmparam_t const *xmp, mmbuffer_t *result)
+		xmparam_t const *xmp, mmfile_t *result)
 {
 	xdchange_t *xscr1 = NULL, *xscr2 = NULL;
 	xdfenv_t xe1, xe2;
diff --git a/xdiff/xutils.c b/xdiff/xutils.c
index 9a999acdc0..4215f646c5 100644
--- a/xdiff/xutils.c
+++ b/xdiff/xutils.c
@@ -39,7 +39,7 @@ uint64_t xdl_bogosqrt(uint64_t n) {
 int xdl_emit_diffrec(char const *rec, long size, char const *pre, long psize,
 		     xdemitcb_t *ecb) {
 	int i = 2;
-	mmbuffer_t mb[3];
+	mmfile_t mb[3];
 
 	mb[0].ptr = (char *) pre;
 	mb[0].size = psize;
@@ -392,7 +392,7 @@ static int xdl_format_hunk_hdr(long s1, long c1, long s2, long c2,
 			       const char *func, long funclen,
 			       xdemitcb_t *ecb) {
 	int nb = 0;
-	mmbuffer_t mb;
+	mmfile_t mb;
 	char buf[128];
 
 	memcpy(buf, "@@ -", 4);
-- 
2.56.0.325.g545d7e68bc

Re: [PATCH 2/5] xdiff: replace mmbuffer_t with mmfile_t

[D. Ben Knoble]

On Tue, Sep 29, 2026 at 2:57 AM Jeff King <peff@peff.net> wrote:
>
> Our import of xdiff has two identical buffer structures: mmfile_t and
> mmbuffer_t. In upstream xdiff these were actually different, but the
> import in 3443546f6e (Use a *real* built-in diff generator, 2006-03-24)
> simplified mmfile_t to a simple buffer.

[snip]

> I guess this step might be controversial, but I hope not. I think the
> ship has long sailed on trying to pull "upstream" changes from xdiff
> (there haven't been any, and we've hacked it up quite a bit already).

I think for a while Vim has also pulled in xdiff from Git (since our
copy is actively maintained), but I might have that wrong. I think
they decided to stop doing so after the Rust bits merged?

At any rate, I don't think that should be a strong (or even weak)
objection to consolidating our code. Thanks.

Re: [PATCH 2/5] xdiff: replace mmbuffer_t with mmfile_t

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Our import of xdiff has two identical buffer structures: mmfile_t and
> mmbuffer_t. In upstream xdiff these were actually different, but the
> import in 3443546f6e (Use a *real* built-in diff generator, 2006-03-24)
> simplified mmfile_t to a simple buffer.
>
> In xdiff we usually use mmfile_t for input and mmbuffer_t for output,
> but they are really both just a ptr/len pair. I don't think that having
> different types is buying us anything in terms of type safety or
> semantics, and having two makes it awkward to use the same helpers for
> both. In particular, an external merge driver's output is read from a
> file, but we can't easily use read_mmfile(), since we want the result in
> an mmbuffer_t.
>
> Let's use mmfile_t for both cases and drop mmbuffer_t. The latter is
> probably a more descriptive name, but we have many more uses of
> mmfile_t (and helpers like read_mmfile). So let's consolidate using that
> name; we can always change it to something more sensible later.
>
> There should be no behavior change here; this is just consolidating the
> types.

Obviously good.

>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> I guess this step might be controversial, but I hope not. I think the
> ship has long sailed on trying to pull "upstream" changes from xdiff
> (there haven't been any, and we've hacked it up quite a bit already).

I share your prediction that we will not be "synchronizing" with the
upstream.

Re: [PATCH 2/5] xdiff: replace mmbuffer_t with mmfile_t

[Patrick Steinhardt]

On Tue, Sep 29, 2026 at 02:52:39AM -0400, Jeff King wrote:
> Our import of xdiff has two identical buffer structures: mmfile_t and
> mmbuffer_t. In upstream xdiff these were actually different, but the
> import in 3443546f6e (Use a *real* built-in diff generator, 2006-03-24)
> simplified mmfile_t to a simple buffer.
> 
> In xdiff we usually use mmfile_t for input and mmbuffer_t for output,
> but they are really both just a ptr/len pair. I don't think that having
> different types is buying us anything in terms of type safety or
> semantics, and having two makes it awkward to use the same helpers for
> both. In particular, an external merge driver's output is read from a
> file, but we can't easily use read_mmfile(), since we want the result in
> an mmbuffer_t.
> 
> Let's use mmfile_t for both cases and drop mmbuffer_t. The latter is
> probably a more descriptive name, but we have many more uses of
> mmfile_t (and helpers like read_mmfile). So let's consolidate using that
> name; we can always change it to something more sensible later.

Yeah, that was my initial reaction, too. `mmbuffer_t` is indeed a better
name as `mmfile_t` indicates that it's coming from... well, a file. And
that's not necessarily true.

I do wonder whether we should just aim for gradual improvement and use
`mmbuffer_t` regardless or even shoot for something altogether different
like `struct xdiff_buf` and then simply not mind the fact that we're
being inconsistent. That would at least be an initial step into a better
direction in my opinion, and we can then touch up things over some time.

But I won't insist on any change like that, I'm okay with keeping
`mmfile_t`.

Patrick

Re: [PATCH 2/5] xdiff: replace mmbuffer_t with mmfile_t

[Jeff King]

On Wed, Sep 30, 2026 at 05:32:49PM +0200, Patrick Steinhardt wrote:

> > Let's use mmfile_t for both cases and drop mmbuffer_t. The latter is
> > probably a more descriptive name, but we have many more uses of
> > mmfile_t (and helpers like read_mmfile). So let's consolidate using that
> > name; we can always change it to something more sensible later.
> 
> Yeah, that was my initial reaction, too. `mmbuffer_t` is indeed a better
> name as `mmfile_t` indicates that it's coming from... well, a file. And
> that's not necessarily true.
> 
> I do wonder whether we should just aim for gradual improvement and use
> `mmbuffer_t` regardless or even shoot for something altogether different
> like `struct xdiff_buf` and then simply not mind the fact that we're
> being inconsistent. That would at least be an initial step into a better
> direction in my opinion, and we can then touch up things over some time.
> 
> But I won't insist on any change like that, I'm okay with keeping
> `mmfile_t`.

I'd really prefer to punt on it for now, just because the diff would be
_so_ big, and has so many extra rabbit holes (e.g., should "mmfile_t
*mf" get a new variable name?).

-Peff

Re: [PATCH 2/5] xdiff: replace mmbuffer_t with mmfile_t

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Wed, Sep 30, 2026 at 05:32:49PM +0200, Patrick Steinhardt wrote:
>
>> > Let's use mmfile_t for both cases and drop mmbuffer_t. The latter is
>> > probably a more descriptive name, but we have many more uses of
>> > mmfile_t (and helpers like read_mmfile). So let's consolidate using that
>> > name; we can always change it to something more sensible later.
>> 
>> Yeah, that was my initial reaction, too. `mmbuffer_t` is indeed a better
>> name as `mmfile_t` indicates that it's coming from... well, a file. And
>> that's not necessarily true.
>> 
>> I do wonder whether we should just aim for gradual improvement and use
>> `mmbuffer_t` regardless or even shoot for something altogether different
>> like `struct xdiff_buf` and then simply not mind the fact that we're
>> being inconsistent. That would at least be an initial step into a better
>> direction in my opinion, and we can then touch up things over some time.
>> 
>> But I won't insist on any change like that, I'm okay with keeping
>> `mmfile_t`.
>
> I'd really prefer to punt on it for now, just because the diff would be
> _so_ big, and has so many extra rabbit holes (e.g., should "mmfile_t
> *mf" get a new variable name?).

I am happy enough with the fact that mmfile is shorter than mmbuffer ;-)

After all xdiff is about comparing two files, and if you do not have
files to compare, you create mmfile out of what you have (which may
not be a file) and pass it to xdiff, pretending it were a file.  You
tell the API that that mmfile has contents from what path etc., so
at that point, the argument that says mmbuffer_t is more generic and
can represent any non-file sources does not really matter, I would
have to say.

[PATCH 3/5] xdiff: use size_t for buffer sizes

[Jeff King]

An mmfile_t stores its size as a signed long, but the more natural type
for a buffer size is size_t. This not only limits the size of entry we
can hold, but also creates some possible integer overflow issues.

For example, read_mmfile() checks that the file size fits in a size_t
before allocating, but then assigns it to a long. Likewise,
read_mmblob() and fill_mmfile() copy sizes from other types without
checking that they fit.

On LP64 systems like Linux, this is mostly academic. You could wrap to a
negative long value, but you'd need an object that's 2^63 bytes, which
is impractical.

But on an LLP64 system like Windows, a 2^31+1-byte blob could perhaps
cause mischief. We do prevent large values from entering the xdiff code
due to MAX_XDIFF_SIZE (which is itself marked as unsigned, so we'd
convert any negative "long" back to a large unsigned value). But if you
ask for binary diffs, that negative long value could instead be
converted to a huge 64-bit size_t when passed to memcmp(), diff_delta(),
etc. So probably there are paths that can cause an out-of-bounds read,
given the right set of options, but I didn't really dig for them.

On a 32-bit system things are less clear. Because "long" and "size_t"
have the same width, any time we implicitly convert to size_t, we should
get back the original size (even if the intermediate "long" is itself
negative). Probably iterating using a long could be a problem, but most
of that happens inside xdiff, which is protected by MAX_XDIFF_SIZE
(which, again, compares in the unsigned space).

Let's just use the obvious size_t type for counting the bytes. I suspect
you could still find truncation problems on LLP64 systems due to the use
of "unsigned long" throughout the code, but that's a larger problem.
This should at least nudge us in the right direction.

Note that we have to update the printf format in emit_binary_diff_body()
to accommodate the new type. Curiously it was using "%lu", even though
the type was signed (I guess compiler printf-linting is happy enough if
just the width of the format and the type match).

Signed-off-by: Jeff King <peff@peff.net>
---
 diff.c        | 2 +-
 xdiff/xdiff.h | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/diff.c b/diff.c
index 414532d09f..b4ac17f8ef 100644
--- a/diff.c
+++ b/diff.c
@@ -3646,7 +3646,7 @@ static void emit_binary_diff_body(struct diff_options *o,
 		data = delta;
 		data_size = delta_size;
 	} else {
-		char *s = xstrfmt("%lu", two->size);
+		char *s = xstrfmt("%"PRIuMAX, (uintmax_t)two->size);
 		emit_diff_symbol(o, DIFF_SYMBOL_BINARY_DIFF_HEADER_LITERAL,
 				 s, strlen(s), 0);
 		free(s);
diff --git a/xdiff/xdiff.h b/xdiff/xdiff.h
index 334eb436f6..8fa513fc4e 100644
--- a/xdiff/xdiff.h
+++ b/xdiff/xdiff.h
@@ -70,7 +70,7 @@ extern "C" {
 
 typedef struct s_mmfile {
 	char *ptr;
-	long size;
+	size_t size;
 } mmfile_t;
 
 typedef struct s_xpparam {
-- 
2.56.0.325.g545d7e68bc

[PATCH 4/5] merge-ll: use read_mmfile() to read external merge results

[Jeff King]

After running an external merge driver, ll_ext_merge() reads the result
back from a temporary file. We can do the same thing with much less code
by using read_mmfile().

As a bonus, note that read_mmfile() correctly uses xsize_t() to detect
the case when we'd truncate the result.

Signed-off-by: Jeff King <peff@peff.net>
---
 merge-ll.c | 21 +++++----------------
 1 file changed, 5 insertions(+), 16 deletions(-)

diff --git a/merge-ll.c b/merge-ll.c
index dfed6411a8..7fab7c5438 100644
--- a/merge-ll.c
+++ b/merge-ll.c
@@ -201,8 +201,7 @@ static enum ll_merge_result ll_ext_merge(const struct ll_merge_driver *fn,
 	struct strbuf cmd = STRBUF_INIT;
 	const char *format = fn->cmdline;
 	struct child_process child = CHILD_PROCESS_INIT;
-	int status, fd, i;
-	struct stat st;
+	int status, i;
 	enum ll_merge_result ret;
 	assert(opts);
 
@@ -241,20 +240,10 @@ static enum ll_merge_result ll_ext_merge(const struct ll_merge_driver *fn,
 	child.use_shell = 1;
 	strvec_push(&child.args, cmd.buf);
 	status = run_command(&child);
-	fd = open(temp[1], O_RDONLY);
-	if (fd < 0)
-		goto bad;
-	if (fstat(fd, &st))
-		goto close_bad;
-	result->size = st.st_size;
-	result->ptr = xmallocz(result->size);
-	if (read_in_full(fd, result->ptr, result->size) != result->size) {
-		FREE_AND_NULL(result->ptr);
-		result->size = 0;
-	}
- close_bad:
-	close(fd);
- bad:
+
+	/* We can ignore errors; result is left NULL/0 in that case. */
+	read_mmfile(result, temp[1]);
+
 	for (i = 0; i < 3; i++)
 		unlink_or_warn(temp[i]);
 	strbuf_release(&cmd);
-- 
2.56.0.325.g545d7e68bc

Re: [PATCH 4/5] merge-ll: use read_mmfile() to read external merge results

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> After running an external merge driver, ll_ext_merge() reads the result
> back from a temporary file. We can do the same thing with much less code
> by using read_mmfile().
>
> As a bonus, note that read_mmfile() correctly uses xsize_t() to detect
> the case when we'd truncate the result.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  merge-ll.c | 21 +++++----------------
>  1 file changed, 5 insertions(+), 16 deletions(-)
>
> diff --git a/merge-ll.c b/merge-ll.c
> index dfed6411a8..7fab7c5438 100644
> --- a/merge-ll.c
> +++ b/merge-ll.c
> @@ -201,8 +201,7 @@ static enum ll_merge_result ll_ext_merge(const struct ll_merge_driver *fn,
>  	struct strbuf cmd = STRBUF_INIT;
>  	const char *format = fn->cmdline;
>  	struct child_process child = CHILD_PROCESS_INIT;
> -	int status, fd, i;
> -	struct stat st;
> +	int status, i;
>  	enum ll_merge_result ret;
>  	assert(opts);
>  
> @@ -241,20 +240,10 @@ static enum ll_merge_result ll_ext_merge(const struct ll_merge_driver *fn,
>  	child.use_shell = 1;
>  	strvec_push(&child.args, cmd.buf);
>  	status = run_command(&child);
> -	fd = open(temp[1], O_RDONLY);
> -	if (fd < 0)
> -		goto bad;
> -	if (fstat(fd, &st))
> -		goto close_bad;
> -	result->size = st.st_size;
> -	result->ptr = xmallocz(result->size);
> -	if (read_in_full(fd, result->ptr, result->size) != result->size) {
> -		FREE_AND_NULL(result->ptr);
> -		result->size = 0;
> -	}
> - close_bad:
> -	close(fd);
> - bad:
> +
> +	/* We can ignore errors; result is left NULL/0 in that case. */
> +	read_mmfile(result, temp[1]);
> +
>  	for (i = 0; i < 3; i++)
>  		unlink_or_warn(temp[i]);
>  	strbuf_release(&cmd);

Lets see if I understand why we can safely ignore errors.

If the external driver claims that it successfully merged (i.e.,
status = run_command(&child) returns 0), and yet read_mmfile() fails
(e.g., perhaps the driver unlinks "%A"), read_mmfile() will leave
result->ptr and result->size as initialized, and we return
LL_MERGE_OK from this function.  The result is eventually relayed to
the caller of ll_merge(), like merge-ort.c:merge_3way(), or
apply.c:three_way_merge().  Both have something like

	status = ll_merge(&result, path,
			  &base_file, "base",
			  &our_file, "ours",
			  &their_file, "theirs",
			  state->repo->index,
			  &merge_opts);
	if (status == LL_MERGE_BINARY_CONFLICT)
		warning("Cannot merge binary files: %s (%s vs. %s)",
			path, "ours", "theirs");
	free(base_file.ptr);
	free(our_file.ptr);
	free(their_file.ptr);
	if (status < 0 || !result.ptr) {
		free(result.ptr);
		return -1;
	}

to treat that result.ptr==NULL is just as bad as any error from
ll_merge() (i.e., status < 0).

merge-blobs.c:merge_blobs() does not check the !result.ptr
condition, and its sole caller builtin/merge-tree.c:result() passes
the NULL to show_diff(), which uses a <NULL, 0> mmfile_t as one side
of xdi_diff(), which the callee is prepared to handle, so this is OK.

rerere.c:try_merge() does not check the !result.ptr condition, and
its caller rerere.c:merge() ends up calling

	fwrite(NULL, (size_t)0, 1, f)

which may happen to work on most systems, but is not exactly kosher.

Perhaps something like this on top might make it safer?  Not even
compile tested and I haven't thought through the ramifications to
rerere.c:merge() code path, that used to take such a bogus merge
result as successful merge and relied on the fwrite(NULL) becoming
a no-op to produce an empty file.

 merge-ll.c | 16 +++++++++++++---
 merge-ll.h |  4 ++++
 2 files changed, 17 insertions(+), 3 deletions(-)

diff --git c/merge-ll.c w/merge-ll.c
index 7fab7c5438..518c05636f 100644
--- c/merge-ll.c
+++ w/merge-ll.c
@@ -405,6 +405,7 @@ enum ll_merge_result ll_merge(mmfile_t *result_buf,
 	const char *ll_driver_name = NULL;
 	int marker_size = DEFAULT_CONFLICT_MARKER_SIZE;
 	const struct ll_merge_driver *driver;
+	enum ll_merge_result result;
 
 	if (!opts)
 		opts = &default_opts;
@@ -434,9 +435,18 @@ enum ll_merge_result ll_merge(mmfile_t *result_buf,
 	if (opts->extra_marker_size) {
 		marker_size += opts->extra_marker_size;
 	}
-	return driver->fn(driver, result_buf, path, ancestor, ancestor_label,
-			  ours, our_label, theirs, their_label,
-			  opts, marker_size);
+	result = driver->fn(driver, result_buf, path, ancestor, ancestor_label,
+			    ours, our_label, theirs, their_label,
+			    opts, marker_size);
+	if (!result_buf.ptr && result == LL_MERGE_OK) {
+		/*
+		 * Forbid the driver from giving bogus result and claim
+		 * that the merge succeeded.
+		 */
+		result = LL_MERGE_ERROR;
+		result_buf.size = 0;
+	}
+	return result;
 }
 
 int ll_merge_marker_size(struct index_state *istate, const char *path)
diff --git c/merge-ll.h w/merge-ll.h
index f95332c682..c5e11f397e 100644
--- c/merge-ll.h
+++ w/merge-ll.h
@@ -23,6 +23,10 @@
  *
  * - Call `ll_merge()`.
  *
+ * - Notice a merge error by checking return value from ll_merge().  If the
+ *   .ptr member of the result is NULL, that may indicate that we failed to
+ *   read the merge results from an external merge driver.
+ *
  * - Read the merged content from `result_buf.ptr` and `result_buf.size`.
  *
  * - Release buffers when finished.  A simple

Re: [PATCH 4/5] merge-ll: use read_mmfile() to read external merge results

[Jeff King]

On Tue, Sep 29, 2026 at 12:22:39PM -0700, Junio C Hamano wrote:

> > +	/* We can ignore errors; result is left NULL/0 in that case. */
> > +	read_mmfile(result, temp[1]);
> > +
> >  	for (i = 0; i < 3; i++)
> >  		unlink_or_warn(temp[i]);
> >  	strbuf_release(&cmd);
> 
> Lets see if I understand why we can safely ignore errors.
> 
> If the external driver claims that it successfully merged (i.e.,
> status = run_command(&child) returns 0), and yet read_mmfile() fails
> (e.g., perhaps the driver unlinks "%A"), read_mmfile() will leave
> result->ptr and result->size as initialized, and we return
> LL_MERGE_OK from this function.  The result is eventually relayed to
> the caller of ll_merge(), like merge-ort.c:merge_3way(), or
> apply.c:three_way_merge().  Both have something like
> 
> 	status = ll_merge(&result, path,
> 			  &base_file, "base",
> 			  &our_file, "ours",
> 			  &their_file, "theirs",
> 			  state->repo->index,
> 			  &merge_opts);
> 	if (status == LL_MERGE_BINARY_CONFLICT)
> 		warning("Cannot merge binary files: %s (%s vs. %s)",
> 			path, "ours", "theirs");
> 	free(base_file.ptr);
> 	free(our_file.ptr);
> 	free(their_file.ptr);
> 	if (status < 0 || !result.ptr) {
> 		free(result.ptr);
> 		return -1;
> 	}
> 
> to treat that result.ptr==NULL is just as bad as any error from
> ll_merge() (i.e., status < 0).

Yeah, exactly. This confused me quite a bit at first, and I thought I'd
found another bug. It feels like we should return LL_MERGE_ERROR for
this case (it is not the external merge driver's error, but rather ours,
but from the caller's perspective does it matter?).

But then I saw that the callers did check for NULL already (which is
what the existing code reliably returned on error). So there's no bug,
but I agree it's subtle. For the purposes of this refactor I tried to
draw the line at retaining the same visible behavior from
ll_ext_merge(), just to keep scope creep to a minimum.

But I'm definitely not opposed to refactoring further on top, and I
think you may have actually found a bug below.

> merge-blobs.c:merge_blobs() does not check the !result.ptr
> condition, and its sole caller builtin/merge-tree.c:result() passes
> the NULL to show_diff(), which uses a <NULL, 0> mmfile_t as one side
> of xdi_diff(), which the callee is prepared to handle, so this is OK.
> 
> rerere.c:try_merge() does not check the !result.ptr condition, and
> its caller rerere.c:merge() ends up calling
> 
> 	fwrite(NULL, (size_t)0, 1, f)
> 
> which may happen to work on most systems, but is not exactly kosher.

Even if it works and sends an empty output, I think it is the wrong
behavior. It's possible the driver actually returned a real output, but
we failed to read it in. And now we're propagating a bogus empty value
instead.

It's hard to test, though, because the easiest way to trigger a read
failure is for the driver to actually _not_ return an output (i.e., to
delete the %A file). And in that case it happens to coincide with the
correct behavior. ;)

I guess a more interesting one is one where the driver changes the mode
on %A so that it cannot be read.

We can trigger that case like this:

-- >8 --
git init

echo base >file
git add file
git commit -am base

git checkout -b one
echo one >file
git commit -am one

git checkout -b two HEAD^
echo two >file
git commit -am two

git config merge.foo.driver 'echo result >%A; chmod 0 %A'
echo 'file merge=foo' >.gitattributes

git merge one
-- 8< --

But I'm not sure how to convince rerere to work on it. The merge command
produces output like:

  error: Could not open /home/peff/tmp/repo/.merge_file_ma1Kcq: Permission denied
  error: failed to execute internal merge for file
  Merge with strategy ort failed.

which is reasonable (probably mentioning the external driver would be
better still, but at least we notice the problem).

I guess to confuse rerere we probably have to do a regular merge, record
the result, and then configure our broken driver, and then try to merge
to run rerere on the result.

So if we amend the end of that script to:

-- >8 --
# merge that records resolution (we abort here, but it
# could just be that we create the same merge elsewhere)
git -c rerere.enabled=true merge one
echo result >file
git rerere
git reset --hard

# now we merge in a way that creates the conflict again
git -c rerere.enabled=false merge one

# but then in the middle we start using the broken driver
git config merge.foo.driver 'echo result >%A; chmod 0 %A'
echo 'file merge=foo' >.gitattributes

# and now rerere gets confused; we claim to use the recorded
# resolution, but it's incorrectly empty
git rerere
-- 8< --

That sequence is quite fishy (changing the attributes mid-merge!?) but
in theory it could trigger racily due to a system error, fread()
failing, and so on.

> Perhaps something like this on top might make it safer?  Not even
> compile tested and I haven't thought through the ramifications to
> rerere.c:merge() code path, that used to take such a bogus merge
> result as successful merge and relied on the fwrite(NULL) becoming
> a no-op to produce an empty file.

This does fix the case above (modulo some s/./->/ in your patch). We end
up with the unresolved contents in "file".

> +	if (!result_buf.ptr && result == LL_MERGE_OK) {
> +		/*
> +		 * Forbid the driver from giving bogus result and claim
> +		 * that the merge succeeded.
> +		 */
> +		result = LL_MERGE_ERROR;
> +		result_buf.size = 0;
> +	}

I had imagined just fixing this in ll_ext_merge(), like:

diff --git a/merge-ll.c b/merge-ll.c
index 7fab7c5438..0e56e303fa 100644
--- a/merge-ll.c
+++ b/merge-ll.c
@@ -241,8 +241,13 @@ static enum ll_merge_result ll_ext_merge(const struct ll_merge_driver *fn,
 	strvec_push(&child.args, cmd.buf);
 	status = run_command(&child);
 
-	/* We can ignore errors; result is left NULL/0 in that case. */
-	read_mmfile(result, temp[1]);
+	/*
+	 * fake a driver error when we can't read the result; a slightly more
+	 * elegant solution is to hoist the status-to-ret conversion from
+	 * below, and then we can directly assign ret = LL_MERGE_ERROR.
+	 */
+	if (read_mmfile(result, temp[1]) < 0)
+		status = 129;
 
 	for (i = 0; i < 3; i++)
 		unlink_or_warn(temp[i]);

which reduces the weirdness coming out of that function. But it wouldn't
help with other drivers (which may or may not have similar problems? I'd
guess not, since they are all operating internally).

-Peff

Re: [PATCH 4/5] merge-ll: use read_mmfile() to read external merge results

[Jeff King]

On Tue, Sep 29, 2026 at 04:11:34PM -0400, Jeff King wrote:

> I had imagined just fixing this in ll_ext_merge(), like:
> [...]
> which reduces the weirdness coming out of that function. But it wouldn't
> help with other drivers (which may or may not have similar problems? I'd
> guess not, since they are all operating internally).

So here are patches to do that, including a cleaned-up version of the
reproduction I posted.

I think ll_ext_merge() is the only driver that has this weird error
case, so it should be sufficient. Your patch would protect a potential
future driver, but I'd be surprised if we had one that introduced the
same NULL-but-not-an-error behavior.

  [6/5]: merge-ll: handle external driver status before reading result
  [7/5]: merge-ll: report an error when reading external merge results fails

 merge-ll.c        | 14 ++++++-------
 t/t4200-rerere.sh | 51 +++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 58 insertions(+), 7 deletions(-)

-Peff

[PATCH 6/5] merge-ll: handle external driver status before reading result

[Jeff King]

After running an external merge driver, ll_ext_merge() reads its output
and cleans up the temporary files before converting the exit status to
an ll_merge_result.

Move that conversion immediately after run_command(). This will let us
override the result if reading the output fails, without having to fake
an exit status. No behavior change yet.

It is tempting to only call read_mmfile() when we have LL_MERGE_OK, but
callers do care about the result even with LL_MERGE_CONFLICT (e.g., the
output may contain a partial). I think we could safely skip it for
LL_MERGE_ERROR, but that's a rare case and not worth complicating the
code for.

Signed-off-by: Jeff King <peff@peff.net>
---
 merge-ll.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/merge-ll.c b/merge-ll.c
index 7fab7c5438..4d82836bc5 100644
--- a/merge-ll.c
+++ b/merge-ll.c
@@ -240,20 +240,20 @@ static enum ll_merge_result ll_ext_merge(const struct ll_merge_driver *fn,
 	child.use_shell = 1;
 	strvec_push(&child.args, cmd.buf);
 	status = run_command(&child);
-
-	/* We can ignore errors; result is left NULL/0 in that case. */
-	read_mmfile(result, temp[1]);
-
-	for (i = 0; i < 3; i++)
-		unlink_or_warn(temp[i]);
-	strbuf_release(&cmd);
 	if (!status)
 		ret = LL_MERGE_OK;
 	else if (status <= 128)
 		ret = LL_MERGE_CONFLICT;
 	else
 		/* died due to a signal: WTERMSIG(status) + 128 */
 		ret = LL_MERGE_ERROR;
+
+	/* We can ignore errors; result is left NULL/0 in that case. */
+	read_mmfile(result, temp[1]);
+
+	for (i = 0; i < 3; i++)
+		unlink_or_warn(temp[i]);
+	strbuf_release(&cmd);
 	return ret;
 }
 
-- 
2.56.0.325.g545d7e68bc

[PATCH 7/5] merge-ll: report an error when reading external merge results fails

[Jeff King]

If we can't read an external merge driver's output, ll_ext_merge()
leaves the result buffer as NULL but returns a status based only on the
driver's exit code. So a driver which exits successfully can cause us to
return LL_MERGE_OK without a result.

Most callers of ll_merge() check for a NULL buffer in addition to an
error return, so they're fine. But rerere's merge() checks only the
return value, and may write out the (incorrect) empty result as the
recorded resolution.

Let's return LL_MERGE_ERROR when read_mmfile() fails, regardless of the
driver's exit status, to make it clear that the returned value is not
valid.

Our test is a little funny; the bad case happens when reading back the
file happens to fail. That can happen randomly due to system errors, but
of course we want it to be deterministic. We can make that happen by
breaking the permissions on the result file. But if we configure a
driver that always does that, we'd never record a rerere result in the
first place! So we instead create a driver that "breaks" the read only
when we instruct it do so, simulating a flaky system.

Signed-off-by: Jeff King <peff@peff.net>
---
 merge-ll.c        |  4 ++--
 t/t4200-rerere.sh | 51 +++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 53 insertions(+), 2 deletions(-)

diff --git a/merge-ll.c b/merge-ll.c
index 4d82836bc5..3b5327e7df 100644
--- a/merge-ll.c
+++ b/merge-ll.c
@@ -248,8 +248,8 @@ static enum ll_merge_result ll_ext_merge(const struct ll_merge_driver *fn,
 		/* died due to a signal: WTERMSIG(status) + 128 */
 		ret = LL_MERGE_ERROR;
 
-	/* We can ignore errors; result is left NULL/0 in that case. */
-	read_mmfile(result, temp[1]);
+	if (read_mmfile(result, temp[1]) < 0)
+		ret = LL_MERGE_ERROR;
 
 	for (i = 0; i < 3; i++)
 		unlink_or_warn(temp[i]);
diff --git a/t/t4200-rerere.sh b/t/t4200-rerere.sh
index 7bb601e117..74945a4e3c 100755
--- a/t/t4200-rerere.sh
+++ b/t/t4200-rerere.sh
@@ -734,4 +734,55 @@ test_expect_success 'rerere does not crash with unmatched conflict marker' '
 	test_must_fail git rebase --continue
 '
 
+test_expect_success SANITY 'rerere preserves conflicts when driver output is unreadable' '
+	test_create_repo unreadable-output &&
+	(
+		cd unreadable-output &&
+		git config rerere.enabled true &&
+		git config rerere.autoupdate true &&
+		write_script merge-driver <<-\EOF &&
+		git merge-file "$@"
+		status=$?
+		if test -f fail-read
+		then
+			chmod 0 "$1" || exit 1
+		fi
+		exit "$status"
+		EOF
+		git config merge.unreadable.driver "./merge-driver %A %O %B" &&
+		echo "file merge=unreadable" >.gitattributes &&
+		test_commit base file base &&
+		git checkout -b one &&
+		test_commit --no-tag one file one &&
+		git checkout -b two base &&
+		test_commit --no-tag two file two &&
+
+		# Teach rerere a resolution while the driver works normally.
+		test_must_fail git merge one &&
+		echo resolved >file &&
+		git rerere &&
+		git merge --abort &&
+
+		# Recreate the conflict without replaying the resolution yet.
+		test_must_fail git -c rerere.enabled=false merge one &&
+
+		# We will expect the same conflicted content after rerere fails
+		# below.
+		cp file expect &&
+		git ls-files -u >expect-index &&
+		test_file_not_empty expect-index &&
+
+		# Now we try rerere again, but the merge driver will cause the
+		# read to fail.
+		>fail-read &&
+		git rerere 2>err &&
+		test_grep "Could not open" err &&
+
+		# And we expect the conflicted state.
+		test_cmp expect file &&
+		git ls-files -u >actual-index &&
+		test_cmp expect-index actual-index
+	)
+'
+
 test_done
-- 
2.56.0.325.g545d7e68bc

Re: [PATCH 7/5] merge-ll: report an error when reading external merge results fails

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> +test_expect_success SANITY 'rerere preserves conflicts when driver output is unreadable' '
> +	test_create_repo unreadable-output &&
> +	(

> +		cd unreadable-output &&
> +		git config rerere.enabled true &&
> +		git config rerere.autoupdate true &&
> +		write_script merge-driver <<-\EOF &&
> +		git merge-file "$@"
> +		status=$?
> +		if test -f fail-read
> +		then
> +			chmod 0 "$1" || exit 1
> +		fi

Can we lose SANITY by "rm $1" instead of "chmod 0"?

> +		exit "$status"
> +		EOF
> +		git config merge.unreadable.driver "./merge-driver %A %O %B" &&
> +		echo "file merge=unreadable" >.gitattributes &&
> +		test_commit base file base &&
> +		git checkout -b one &&
> +		test_commit --no-tag one file one &&
> +		git checkout -b two base &&
> +		test_commit --no-tag two file two &&
> +
> +		# Teach rerere a resolution while the driver works normally.
> +		test_must_fail git merge one &&
> +		echo resolved >file &&
> +		git rerere &&
> +		git merge --abort &&
> +
> +		# Recreate the conflict without replaying the resolution yet.
> +		test_must_fail git -c rerere.enabled=false merge one &&
> +
> +		# We will expect the same conflicted content after rerere fails
> +		# below.
> +		cp file expect &&
> +		git ls-files -u >expect-index &&
> +		test_file_not_empty expect-index &&
> +
> +		# Now we try rerere again, but the merge driver will cause the
> +		# read to fail.
> +		>fail-read &&
> +		git rerere 2>err &&
> +		test_grep "Could not open" err &&
> +
> +		# And we expect the conflicted state.
> +		test_cmp expect file &&
> +		git ls-files -u >actual-index &&
> +		test_cmp expect-index actual-index
> +	)
> +'
> +
>  test_done

Re: [PATCH 7/5] merge-ll: report an error when reading external merge results fails

[Jeff King]

On Tue, Sep 29, 2026 at 02:19:26PM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > +test_expect_success SANITY 'rerere preserves conflicts when driver output is unreadable' '
> > +	test_create_repo unreadable-output &&
> > +	(
> 
> > +		cd unreadable-output &&
> > +		git config rerere.enabled true &&
> > +		git config rerere.autoupdate true &&
> > +		write_script merge-driver <<-\EOF &&
> > +		git merge-file "$@"
> > +		status=$?
> > +		if test -f fail-read
> > +		then
> > +			chmod 0 "$1" || exit 1
> > +		fi
> 
> Can we lose SANITY by "rm $1" instead of "chmod 0"?

Hmm, I guess we can. I was thinking for some reason that we need to fail
later in read_mmfile(). But I think I was just confusing that with the
earlier leak fix. With a stat failure, read_mmfile() will leave the
mmfile_t untouched, but we initialize it in ll_ext_merge() to NULL/0. So
the outcome should be the same from the caller's perspective.

And that's actually a more realistic example, I think. Instead of
simulating a racy read failure, we are considering a driver that
sometimes accidentally deletes the file while returning 0. Buggy, but a
plausible bug. ;)

Here's a resend of that final patch (not just a squash, because the
commit message mentioned the chmod).

-- >8 --
Subject: merge-ll: report an error when reading external merge results fails

If we can't read an external merge driver's output, ll_ext_merge()
leaves the result buffer as NULL but returns a status based only on the
driver's exit code. So a driver which exits successfully can cause us to
return LL_MERGE_OK without a result.

Most callers of ll_merge() check for a NULL buffer in addition to an
error return, so they're fine. But rerere's merge() checks only the
return value, and may write out the (incorrect) empty result as the
recorded resolution.

Let's return LL_MERGE_ERROR when read_mmfile() fails, regardless of the
driver's exit status, to make it clear that the returned value is not
valid.

Our test is a little funny; the bad case happens when reading back the
file happens to fail. That can happen due to system errors, but of
course we want it to be deterministic. We can make that happen by
removing the result file. But if we configure a driver that always does
that, we'd never record a rerere result in the first place! So we
instead create a driver that "breaks" the read only when we instruct it
to do so.

Signed-off-by: Jeff King <peff@peff.net>
---
 merge-ll.c        |  4 ++--
 t/t4200-rerere.sh | 51 +++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 53 insertions(+), 2 deletions(-)

diff --git a/merge-ll.c b/merge-ll.c
index 4d82836bc5..3b5327e7df 100644
--- a/merge-ll.c
+++ b/merge-ll.c
@@ -248,8 +248,8 @@ static enum ll_merge_result ll_ext_merge(const struct ll_merge_driver *fn,
 		/* died due to a signal: WTERMSIG(status) + 128 */
 		ret = LL_MERGE_ERROR;
 
-	/* We can ignore errors; result is left NULL/0 in that case. */
-	read_mmfile(result, temp[1]);
+	if (read_mmfile(result, temp[1]) < 0)
+		ret = LL_MERGE_ERROR;
 
 	for (i = 0; i < 3; i++)
 		unlink_or_warn(temp[i]);
diff --git a/t/t4200-rerere.sh b/t/t4200-rerere.sh
index 7bb601e117..5be3f056f5 100755
--- a/t/t4200-rerere.sh
+++ b/t/t4200-rerere.sh
@@ -734,4 +734,55 @@ test_expect_success 'rerere does not crash with unmatched conflict marker' '
 	test_must_fail git rebase --continue
 '
 
+test_expect_success 'rerere preserves conflicts when driver output is unreadable' '
+	test_create_repo unreadable-output &&
+	(
+		cd unreadable-output &&
+		git config rerere.enabled true &&
+		git config rerere.autoupdate true &&
+		write_script merge-driver <<-\EOF &&
+		git merge-file "$@"
+		status=$?
+		if test -f fail-read
+		then
+			rm "$1" || exit 1
+		fi
+		exit "$status"
+		EOF
+		git config merge.unreadable.driver "./merge-driver %A %O %B" &&
+		echo "file merge=unreadable" >.gitattributes &&
+		test_commit base file base &&
+		git checkout -b one &&
+		test_commit --no-tag one file one &&
+		git checkout -b two base &&
+		test_commit --no-tag two file two &&
+
+		# Teach rerere a resolution while the driver works normally.
+		test_must_fail git merge one &&
+		echo resolved >file &&
+		git rerere &&
+		git merge --abort &&
+
+		# Recreate the conflict without replaying the resolution yet.
+		test_must_fail git -c rerere.enabled=false merge one &&
+
+		# We will expect the same conflicted content after rerere fails
+		# below.
+		cp file expect &&
+		git ls-files -u >expect-index &&
+		test_file_not_empty expect-index &&
+
+		# Now we try rerere again, but the merge driver will cause the
+		# read to fail.
+		>fail-read &&
+		git rerere 2>err &&
+		test_grep "Could not stat" err &&
+
+		# And we expect the conflicted state.
+		test_cmp expect file &&
+		git ls-files -u >actual-index &&
+		test_cmp expect-index actual-index
+	)
+'
+
 test_done
-- 
2.56.0.325.g545d7e68bc

Re: [PATCH 7/5] merge-ll: report an error when reading external merge results fails

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Here's a resend of that final patch (not just a squash, because the
> commit message mentioned the chmod).

Makes sense.

These 6/5 and 7/5 are probably better squashed into 5/5 than left as
"oops that was bad, so here is a preliminary clean-up to make the
fix easier (6/5), and here is the fix of the fifth step (7/5)", no?

Re: [PATCH 7/5] merge-ll: report an error when reading external merge results fails

[Jeff King]

On Wed, Sep 30, 2026 at 11:01:28AM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > Here's a resend of that final patch (not just a squash, because the
> > commit message mentioned the chmod).
> 
> Makes sense.
> 
> These 6/5 and 7/5 are probably better squashed into 5/5 than left as
> "oops that was bad, so here is a preliminary clean-up to make the
> fix easier (6/5), and here is the fix of the fifth step (7/5)", no?

I don't think it is the fault of 5/5 at all (which carefully tried to
maintain the NULL behavior). The problem fixed by 7/5 existed before my
series.

In theory that fix _could_ come earlier in the series, but it's actually
much easier to fix after 5/5, because we have a single spot to error
check.

-Peff

Re: [PATCH 7/5] merge-ll: report an error when reading external merge results fails

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Wed, Sep 30, 2026 at 11:01:28AM -0700, Junio C Hamano wrote:
>
>> Jeff King <peff@peff.net> writes:
>> 
>> > Here's a resend of that final patch (not just a squash, because the
>> > commit message mentioned the chmod).
>> 
>> Makes sense.
>> 
>> These 6/5 and 7/5 are probably better squashed into 5/5 than left as
>> "oops that was bad, so here is a preliminary clean-up to make the
>> fix easier (6/5), and here is the fix of the fifth step (7/5)", no?
>
> I don't think it is the fault of 5/5 at all (which carefully tried to
> maintain the NULL behavior). The problem fixed by 7/5 existed before my
> series.

Ah, OK, rereading the code before 5/5 is applied, I notice that we
are not declaring the result is bad when we jump to "bad:" label
after noticing an I/O error.  The code only paid attention to the
status returned by run_command().

> In theory that fix _could_ come earlier in the series, but it's actually
> much easier to fix after 5/5, because we have a single spot to error
> check.

True.  Thanks.

Re: [PATCH 4/5] merge-ll: use read_mmfile() to read external merge results

[Patrick Steinhardt]

On Tue, Sep 29, 2026 at 02:54:42AM -0400, Jeff King wrote:
> diff --git a/merge-ll.c b/merge-ll.c
> index dfed6411a8..7fab7c5438 100644
> --- a/merge-ll.c
> +++ b/merge-ll.c
> @@ -241,20 +240,10 @@ static enum ll_merge_result ll_ext_merge(const struct ll_merge_driver *fn,
>  	child.use_shell = 1;
>  	strvec_push(&child.args, cmd.buf);
>  	status = run_command(&child);
> -	fd = open(temp[1], O_RDONLY);
> -	if (fd < 0)
> -		goto bad;
> -	if (fstat(fd, &st))
> -		goto close_bad;
> -	result->size = st.st_size;
> -	result->ptr = xmallocz(result->size);

So we do lose the NUL-termination that `xmallocz()` gave us, as
`read_mmfile()` doesn't do that. You reinstate that in the last patch
though, which makes me lean more into the direction of having that last
optional patch. If so though, we may want to reorder it to come first.

> -	if (read_in_full(fd, result->ptr, result->size) != result->size) {
> -		FREE_AND_NULL(result->ptr);
> -		result->size = 0;
> -	}
> - close_bad:
> -	close(fd);
> - bad:
> +
> +	/* We can ignore errors; result is left NULL/0 in that case. */
> +	read_mmfile(result, temp[1]);

One change in behaviour that wasn't called out is that this will now
make us write an error message in case we failed reading the file. That
could be a good change, but that's hard to say.

Patrick

Re: [PATCH 4/5] merge-ll: use read_mmfile() to read external merge results

[Jeff King]

On Wed, Sep 30, 2026 at 05:33:01PM +0200, Patrick Steinhardt wrote:

> So we do lose the NUL-termination that `xmallocz()` gave us, as
> `read_mmfile()` doesn't do that. You reinstate that in the last patch
> though, which makes me lean more into the direction of having that last
> optional patch. If so though, we may want to reorder it to come first.

Yeah, I'll do that re-order.

> > -	if (read_in_full(fd, result->ptr, result->size) != result->size) {
> > -		FREE_AND_NULL(result->ptr);
> > -		result->size = 0;
> > -	}
> > - close_bad:
> > -	close(fd);
> > - bad:
> > +
> > +	/* We can ignore errors; result is left NULL/0 in that case. */
> > +	read_mmfile(result, temp[1]);
> 
> One change in behaviour that wasn't called out is that this will now
> make us write an error message in case we failed reading the file. That
> could be a good change, but that's hard to say.

True, I hadn't even thought about that. It seems like a strict
improvement to me, but I'll mention it in the commit message.

-Peff

[PATCH 5/5] xdiff: NUL-terminate buffers read by read_mmfile()

[Jeff King]

Since an mmfile_t is a ptr/len pair, our read_mmfile() allocates exactly
the number of bytes we claim to store. But in many other places in Git,
we add an extra NUL "just in case", which can help avoid read overruns
due to off-by-ones or the use of string functions.

I don't know of any path that would benefit from this, but I noticed it
while converting ll_ext_merge() to use read_mmfile(), since its original
code did add a NUL byte (even though I cannot find any case where it
would have mattered). Let's teach read_mmfile() to add this defensive
NUL; it probably doesn't help anything, but nor should it hurt.

Note that the matching read_mmblob() doesn't need the same treatment.
Its buffers already have a NUL from the object-reading code (which uses
the same defensive trick).

As a bonus, we can get rid of the hack in read_mmfile() to handle empty
files by allocating a single byte.

Signed-off-by: Jeff King <peff@peff.net>
---
This one is obviously optional, which is why I put it last.

 xdiff-interface.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xdiff-interface.c b/xdiff-interface.c
index bc340d5a8a..b3e9f1952b 100644
--- a/xdiff-interface.c
+++ b/xdiff-interface.c
@@ -166,7 +166,7 @@ int read_mmfile(mmfile_t *ptr, const char *filename)
 	if (!(f = fopen(filename, "rb")))
 		return error_errno("Could not open %s", filename);
 	sz = xsize_t(st.st_size);
-	ptr->ptr = xmalloc(sz ? sz : 1);
+	ptr->ptr = xmallocz(sz);
 	if (sz && fread(ptr->ptr, sz, 1, f) != 1) {
 		FREE_AND_NULL(ptr->ptr);
 		fclose(f);
-- 
2.56.0.325.g545d7e68bc

Re: [PATCH 5/5] xdiff: NUL-terminate buffers read by read_mmfile()

[Patrick Steinhardt]

On Tue, Sep 29, 2026 at 02:55:04AM -0400, Jeff King wrote:
> Since an mmfile_t is a ptr/len pair, our read_mmfile() allocates exactly
> the number of bytes we claim to store. But in many other places in Git,
> we add an extra NUL "just in case", which can help avoid read overruns
> due to off-by-ones or the use of string functions.
> 
> I don't know of any path that would benefit from this, but I noticed it
> while converting ll_ext_merge() to use read_mmfile(), since its original
> code did add a NUL byte (even though I cannot find any case where it
> would have mattered). Let's teach read_mmfile() to add this defensive
> NUL; it probably doesn't help anything, but nor should it hurt.
> 
> Note that the matching read_mmblob() doesn't need the same treatment.
> Its buffers already have a NUL from the object-reading code (which uses
> the same defensive trick).
> 
> As a bonus, we can get rid of the hack in read_mmfile() to handle empty
> files by allocating a single byte.
> 
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> This one is obviously optional, which is why I put it last.

Hm, I'm somewhat indifferent here. It always feels a bit weird to be
this defensive because "programming errors", as the next question then
is "but what about all the other errors where we're not defensive?" But
the xdiff code is complex enough with a bunch of pointer arithmetics, so
maybe it's not even that bad of an idea.

That being said, I feel like a better course of action could be to use a
fuzzer for this code, because as far as I'm aware we have none yet, and
that would potentially shake out a bunch of bugs. But that still doesn't
really help us to catch platform-specific bugs due to different integer
sizes.

The counterargument is that before your 3/5 we used to use xmallocz, so
you're essentially just reinstating the previous safety guards.

> diff --git a/xdiff-interface.c b/xdiff-interface.c
> index bc340d5a8a..b3e9f1952b 100644
> --- a/xdiff-interface.c
> +++ b/xdiff-interface.c
> @@ -166,7 +166,7 @@ int read_mmfile(mmfile_t *ptr, const char *filename)
>  	if (!(f = fopen(filename, "rb")))
>  		return error_errno("Could not open %s", filename);
>  	sz = xsize_t(st.st_size);
> -	ptr->ptr = xmalloc(sz ? sz : 1);
> +	ptr->ptr = xmallocz(sz);

I was staring at this code a while before I noticed the added `z` at the
end of this function.

Patrick

Re: [PATCH 5/5] xdiff: NUL-terminate buffers read by read_mmfile()

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> On Tue, Sep 29, 2026 at 02:55:04AM -0400, Jeff King wrote:
>> Since an mmfile_t is a ptr/len pair, our read_mmfile() allocates exactly
>> the number of bytes we claim to store. But in many other places in Git,
>> we add an extra NUL "just in case", which can help avoid read overruns
>> due to off-by-ones or the use of string functions.
>> 
>> I don't know of any path that would benefit from this, but I noticed it
>> while converting ll_ext_merge() to use read_mmfile(), since its original
>> code did add a NUL byte (even though I cannot find any case where it
>> would have mattered). Let's teach read_mmfile() to add this defensive
>> NUL; it probably doesn't help anything, but nor should it hurt.
>> 
>> Note that the matching read_mmblob() doesn't need the same treatment.
>> Its buffers already have a NUL from the object-reading code (which uses
>> the same defensive trick).
>> 
>> As a bonus, we can get rid of the hack in read_mmfile() to handle empty
>> files by allocating a single byte.
>> 
>> Signed-off-by: Jeff King <peff@peff.net>
>> ---
>> This one is obviously optional, which is why I put it last.
>> ...
>> -	ptr->ptr = xmalloc(sz ? sz : 1);
>> +	ptr->ptr = xmallocz(sz);
>
> I was staring at this code a while before I noticed the added `z` at the
> end of this function.

I had the same reaction yesterday.  The proposed log message never
said how it added the extra NUL (or for that matter, it wasn't clear
if it actually did the adding).  The usual imperative "Add the same
'just in case' NUL by using xmallocz()." would have helped a lot.

Re: [PATCH 5/5] xdiff: NUL-terminate buffers read by read_mmfile()

[Jeff King]

On Wed, Sep 30, 2026 at 05:32:55PM +0200, Patrick Steinhardt wrote:

> > This one is obviously optional, which is why I put it last.
> 
> Hm, I'm somewhat indifferent here. It always feels a bit weird to be
> this defensive because "programming errors", as the next question then
> is "but what about all the other errors where we're not defensive?" But
> the xdiff code is complex enough with a bunch of pointer arithmetics, so
> maybe it's not even that bad of an idea.
> 
> That being said, I feel like a better course of action could be to use a
> fuzzer for this code, because as far as I'm aware we have none yet, and
> that would potentially shake out a bunch of bugs. But that still doesn't
> really help us to catch platform-specific bugs due to different integer
> sizes.

I look at it as: why not do both?

Mostly the lack of extra NUL surprised me, as we routinely add one in
most other places (and it has prevented some memory bugs in the past).

> The counterargument is that before your 3/5 we used to use xmallocz, so
> you're essentially just reinstating the previous safety guards.

Yes, though I did confirm that those guards were doing nothing. This is
less about protecting the new ll_ext_merge() caller and more about all
of the _other_ callers of read_mmfile().

But yeah, it is obviously a lot easier to explain if this patch comes
first. I just wasn't sure if we'd want to drop it or not (though yeah,
we probably should explain in the earlier patch that the lack of NUL
termination is OK).

I'll re-roll with this patch earlier in the series.

> > diff --git a/xdiff-interface.c b/xdiff-interface.c
> > index bc340d5a8a..b3e9f1952b 100644
> > --- a/xdiff-interface.c
> > +++ b/xdiff-interface.c
> > @@ -166,7 +166,7 @@ int read_mmfile(mmfile_t *ptr, const char *filename)
> >  	if (!(f = fopen(filename, "rb")))
> >  		return error_errno("Could not open %s", filename);
> >  	sz = xsize_t(st.st_size);
> > -	ptr->ptr = xmalloc(sz ? sz : 1);
> > +	ptr->ptr = xmallocz(sz);
> 
> I was staring at this code a while before I noticed the added `z` at the
> end of this function.

Heh, fair. I'll say something more explicit in the commit message when
re-rolling.

-Peff

[PATCH v2 0/7] use size_t for xdiff mmfile_t

[Jeff King]

On Tue, Sep 29, 2026 at 02:49:36AM -0400, Jeff King wrote:

> An earlier series tried to simplify ll_ext_merge()'s code to read back
> the merge result from a temporary file, but Elijah pointed out some
> subtle integer overflow confusion:
> 
>   https://lore.kernel.org/git/CABPp-BG9Hkc7i_JxAbYfyzu+b4Mc_pZUr0jJF=vY0jHSARpHzw@mail.gmail.com/
> 
> I dug a little bit and found that similar problems exist elsewhere. So
> here's an attempt to make things at least incrementally better. And
> patch 4 is the original cleanup I set out to do. ;)

Here's a v2 that addresses review so far. The end state is the same
(plus the two bonus patches sent earlier), but it moves the xmallocz()
patch earlier, and fills in a few bits in the commit messages.

Range-diff is below.

  [1/7]: xdiff: clean up read_mmfile() allocations on error
  [2/7]: xdiff: replace mmbuffer_t with mmfile_t
  [3/7]: xdiff: use size_t for buffer sizes
  [4/7]: xdiff: NUL-terminate buffers read by read_mmfile()
  [5/7]: merge-ll: use read_mmfile() to read external merge results
  [6/7]: merge-ll: handle external driver status before reading result
  [7/7]: merge-ll: report an error when reading external merge results fails

 Documentation/technical/api-merge.adoc |  7 ++--
 apply.c                                |  2 +-
 builtin/checkout.c                     |  2 +-
 builtin/merge-file.c                   |  2 +-
 builtin/merge-tree.c                   |  2 +-
 builtin/rerere.c                       |  8 +++-
 diff.c                                 |  2 +-
 merge-blobs.c                          |  2 +-
 merge-ll.c                             | 39 +++++++-------------
 merge-ll.h                             |  4 +-
 merge-ort.c                            |  4 +-
 notes-merge.c                          |  2 +-
 rerere.c                               | 11 +++---
 t/t4200-rerere.sh                      | 51 ++++++++++++++++++++++++++
 xdiff-interface.c                      |  5 ++-
 xdiff/xdiff.h                          | 11 ++----
 xdiff/xmerge.c                         |  4 +-
 xdiff/xutils.c                         |  4 +-
 18 files changed, 100 insertions(+), 62 deletions(-)

1:  985905950f = 1:  985905950f xdiff: clean up read_mmfile() allocations on error
2:  ddcae336eb = 2:  ddcae336eb xdiff: replace mmbuffer_t with mmfile_t
3:  36d932e0ee = 3:  36d932e0ee xdiff: use size_t for buffer sizes
5:  f25902e825 ! 4:  c9cd3c7c3c xdiff: NUL-terminate buffers read by read_mmfile()
    @@ Commit message
         I don't know of any path that would benefit from this, but I noticed it
         while converting ll_ext_merge() to use read_mmfile(), since its original
         code did add a NUL byte (even though I cannot find any case where it
    -    would have mattered). Let's teach read_mmfile() to add this defensive
    -    NUL; it probably doesn't help anything, but nor should it hurt.
    +    would have mattered). Let's add the same defensive NUL in read_mmfile()
    +    by using xmallocz() instead of xmalloc().
     
         Note that the matching read_mmblob() doesn't need the same treatment.
         Its buffers already have a NUL from the object-reading code (which uses
4:  6ac0d54bda ! 5:  86fa283e00 merge-ll: use read_mmfile() to read external merge results
    @@ Commit message
         back from a temporary file. We can do the same thing with much less code
         by using read_mmfile().
     
    -    As a bonus, note that read_mmfile() correctly uses xsize_t() to detect
    -    the case when we'd truncate the result.
    +    There are also two behavior improvements.
    +
    +    One, read_mmfile() correctly uses xsize_t() to detect the case when we'd
    +    truncate the result.
    +
    +    And two, read_mmfile() will report errors to stderr if it can't read the
    +    file (whereas the existing code silently returned NULL). I think most
    +    callers would have said _something_ in this case like "failed to execute
    +    merge" (from merge-ort), but more specifics are probably helpful (e.g.,
    +    to distinguish a random system error from a badly configured merge
    +    driver).
     
         Signed-off-by: Jeff King <peff@peff.net>
     
6:  3e5f090284 = 6:  8ad0b774bf merge-ll: handle external driver status before reading result
7:  30357e6e9a = 7:  896031317b merge-ll: report an error when reading external merge results fails

[PATCH v2 1/7] xdiff: clean up read_mmfile() allocations on error

[Jeff King]

When read_mmfile() returns an error, it may or may not have allocated a
buffer in the passed-in mmfile_t. So callers must initialize the pointer
to NULL and free it even on error.

Most callers do this already, but rerere's diff_two() does not, and
would leak the buffer after a read error. We could fix it directly, but
let's instead try to make the interface less error-prone by freeing the
memory when returning failure from read_mmfile().

This fixes (part of) the leak in diff_two(). In theory it also lets us
simplify other callers to skip initializing the mmfile. But in practice
most still need zero-initialization because they may jump to free()
before even calling read_mmfile (e.g., in try_merge()). But we can at
least simplify rerere_forget_one_path() a bit.

I said "part of" earlier. There's a related leak in diff_two(): if
reading the first file succeeds but reading the second fails, we return
early and leak the first buffer. We can fix that by checking each
individually.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/rerere.c  | 6 +++++-
 rerere.c          | 3 +--
 xdiff-interface.c | 1 +
 3 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/builtin/rerere.c b/builtin/rerere.c
index a056cb791b..d39c6e8445 100644
--- a/builtin/rerere.c
+++ b/builtin/rerere.c
@@ -34,8 +34,12 @@ static int diff_two(const char *file1, const char *label1,
 	mmfile_t minus, plus;
 	int ret;
 
-	if (read_mmfile(&minus, file1) || read_mmfile(&plus, file2))
+	if (read_mmfile(&minus, file1))
 		return -1;
+	if (read_mmfile(&plus, file2)) {
+		free(minus.ptr);
+		return -1;
+	}
 
 	printf("--- a/%s\n+++ b/%s\n", label1, label2);
 	fflush(stdout);
diff --git a/rerere.c b/rerere.c
index 1c3745d9e3..856347c9ae 100644
--- a/rerere.c
+++ b/rerere.c
@@ -1039,7 +1039,7 @@ static int rerere_forget_one_path(struct index_state *istate,
 	for (id->variant = 0;
 	     id->variant < id->collection->status_nr;
 	     id->variant++) {
-		mmfile_t cur = { NULL, 0 };
+		mmfile_t cur;
 		mmbuffer_t result = {NULL, 0};
 		int cleanly_resolved;
 
@@ -1048,7 +1048,6 @@ static int rerere_forget_one_path(struct index_state *istate,
 
 		handle_cache(istate, path, hash, rerere_path(&buf, id, "thisimage"));
 		if (read_mmfile(&cur, rerere_path(&buf, id, "thisimage"))) {
-			free(cur.ptr);
 			error(_("failed to update conflicted state in '%s'"), path);
 			goto fail_exit;
 		}
diff --git a/xdiff-interface.c b/xdiff-interface.c
index db6938689f..e3dd2184ae 100644
--- a/xdiff-interface.c
+++ b/xdiff-interface.c
@@ -168,6 +168,7 @@ int read_mmfile(mmfile_t *ptr, const char *filename)
 	sz = xsize_t(st.st_size);
 	ptr->ptr = xmalloc(sz ? sz : 1);
 	if (sz && fread(ptr->ptr, sz, 1, f) != 1) {
+		FREE_AND_NULL(ptr->ptr);
 		fclose(f);
 		return error("Could not read %s", filename);
 	}
-- 
2.56.0.354.gb6b32d5be5

[PATCH v2 2/7] xdiff: replace mmbuffer_t with mmfile_t

[Jeff King]

Our import of xdiff has two identical buffer structures: mmfile_t and
mmbuffer_t. In upstream xdiff these were actually different, but the
import in 3443546f6e (Use a *real* built-in diff generator, 2006-03-24)
simplified mmfile_t to a simple buffer.

In xdiff we usually use mmfile_t for input and mmbuffer_t for output,
but they are really both just a ptr/len pair. I don't think that having
different types is buying us anything in terms of type safety or
semantics, and having two makes it awkward to use the same helpers for
both. In particular, an external merge driver's output is read from a
file, but we can't easily use read_mmfile(), since we want the result in
an mmbuffer_t.

Let's use mmfile_t for both cases and drop mmbuffer_t. The latter is
probably a more descriptive name, but we have many more uses of
mmfile_t (and helpers like read_mmfile). So let's consolidate using that
name; we can always change it to something more sensible later.

There should be no behavior change here; this is just consolidating the
types.

Signed-off-by: Jeff King <peff@peff.net>
---
 Documentation/technical/api-merge.adoc |  7 +++----
 apply.c                                |  2 +-
 builtin/checkout.c                     |  2 +-
 builtin/merge-file.c                   |  2 +-
 builtin/merge-tree.c                   |  2 +-
 builtin/rerere.c                       |  2 +-
 merge-blobs.c                          |  2 +-
 merge-ll.c                             | 12 ++++++------
 merge-ll.h                             |  4 ++--
 merge-ort.c                            |  4 ++--
 notes-merge.c                          |  2 +-
 rerere.c                               |  8 ++++----
 xdiff-interface.c                      |  2 +-
 xdiff/xdiff.h                          |  9 ++-------
 xdiff/xmerge.c                         |  4 ++--
 xdiff/xutils.c                         |  4 ++--
 16 files changed, 31 insertions(+), 37 deletions(-)

diff --git a/Documentation/technical/api-merge.adoc b/Documentation/technical/api-merge.adoc
index c2ba01828c..b691599393 100644
--- a/Documentation/technical/api-merge.adoc
+++ b/Documentation/technical/api-merge.adoc
@@ -20,11 +20,10 @@ responsible for a few things.
 Data structures
 ---------------
 
-* `mmbuffer_t`, `mmfile_t`
+* `mmfile_t`
 
-These store data usable for use by the xdiff backend, for writing and
-for reading, respectively.  See `xdiff/xdiff.h` for the definitions
-and `diff.c` for examples.
+This stores a buffer and its size for input to or output from the xdiff
+backend. See `xdiff/xdiff.h` for the definition and `diff.c` for examples.
 
 * `struct ll_merge_options`
 
diff --git a/apply.c b/apply.c
index f00b7ba4d3..faf3c1dba0 100644
--- a/apply.c
+++ b/apply.c
@@ -3646,7 +3646,7 @@ static int three_way_merge(struct apply_state *state,
 {
 	mmfile_t base_file, our_file, their_file;
 	struct ll_merge_options merge_opts = LL_MERGE_OPTIONS_INIT;
-	mmbuffer_t result = { NULL };
+	mmfile_t result = { NULL };
 	enum ll_merge_result status;
 
 	/* resolve trivial cases first */
diff --git a/builtin/checkout.c b/builtin/checkout.c
index c0f0d2c700..2d575a4f57 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -320,7 +320,7 @@ static int checkout_merged(int pos, const struct checkout *state,
 	enum ll_merge_result merge_status;
 	int status;
 	struct object_id oid;
-	mmbuffer_t result_buf;
+	mmfile_t result_buf;
 	struct object_id threeway[3];
 	unsigned mode = 0;
 	struct ll_merge_options ll_opts = LL_MERGE_OPTIONS_INIT;
diff --git a/builtin/merge-file.c b/builtin/merge-file.c
index 8fa5765239..ddca408c46 100644
--- a/builtin/merge-file.c
+++ b/builtin/merge-file.c
@@ -64,7 +64,7 @@ int cmd_merge_file(int argc,
 {
 	const char *names[3] = { 0 };
 	mmfile_t mmfs[3] = { 0 };
-	mmbuffer_t result = { 0 };
+	mmfile_t result = { 0 };
 	xmparam_t xmp = { 0 };
 	int ret = 0, i = 0, to_stdout = 0, object_id = 0;
 	int quiet = 0;
diff --git a/builtin/merge-tree.c b/builtin/merge-tree.c
index 49f41e520f..552c2ad736 100644
--- a/builtin/merge-tree.c
+++ b/builtin/merge-tree.c
@@ -109,7 +109,7 @@ static void *origin(struct merge_list *entry, size_t *size)
 	return NULL;
 }
 
-static int show_outf(void *priv UNUSED, mmbuffer_t *mb, int nbuf)
+static int show_outf(void *priv UNUSED, mmfile_t *mb, int nbuf)
 {
 	int i;
 	for (i = 0; i < nbuf; i++)
diff --git a/builtin/rerere.c b/builtin/rerere.c
index d39c6e8445..ef03b79f5b 100644
--- a/builtin/rerere.c
+++ b/builtin/rerere.c
@@ -16,7 +16,7 @@ static const char * const rerere_usage[] = {
 	NULL,
 };
 
-static int outf(void *dummy UNUSED, mmbuffer_t *ptr, int nbuf)
+static int outf(void *dummy UNUSED, mmfile_t *ptr, int nbuf)
 {
 	int i;
 	for (i = 0; i < nbuf; i++)
diff --git a/merge-blobs.c b/merge-blobs.c
index 16a75bd1e3..49dbec9529 100644
--- a/merge-blobs.c
+++ b/merge-blobs.c
@@ -38,7 +38,7 @@ static void *three_way_filemerge(struct index_state *istate,
 				 size_t *size)
 {
 	enum ll_merge_result merge_status;
-	mmbuffer_t res;
+	mmfile_t res;
 
 	/*
 	 * This function is only used by cmd_merge_tree, which
diff --git a/merge-ll.c b/merge-ll.c
index ef5287dee8..dfed6411a8 100644
--- a/merge-ll.c
+++ b/merge-ll.c
@@ -21,7 +21,7 @@
 struct ll_merge_driver;
 
 typedef enum ll_merge_result (*ll_merge_fn)(const struct ll_merge_driver *,
-			   mmbuffer_t *result,
+			   mmfile_t *result,
 			   const char *path,
 			   mmfile_t *orig, const char *orig_name,
 			   mmfile_t *src1, const char *name1,
@@ -56,7 +56,7 @@ void reset_merge_attributes(void)
  * Built-in low-levels
  */
 static enum ll_merge_result ll_binary_merge(const struct ll_merge_driver *drv UNUSED,
-			   mmbuffer_t *result,
+			   mmfile_t *result,
 			   const char *path UNUSED,
 			   mmfile_t *orig, const char *orig_name UNUSED,
 			   mmfile_t *src1, const char *name1 UNUSED,
@@ -101,7 +101,7 @@ static enum ll_merge_result ll_binary_merge(const struct ll_merge_driver *drv UN
 }
 
 static enum ll_merge_result ll_xdl_merge(const struct ll_merge_driver *drv_unused,
-			mmbuffer_t *result,
+			mmfile_t *result,
 			const char *path,
 			mmfile_t *orig, const char *orig_name,
 			mmfile_t *src1, const char *name1,
@@ -147,7 +147,7 @@ static enum ll_merge_result ll_xdl_merge(const struct ll_merge_driver *drv_unuse
 }
 
 static enum ll_merge_result ll_union_merge(const struct ll_merge_driver *drv_unused,
-			  mmbuffer_t *result,
+			  mmfile_t *result,
 			  const char *path,
 			  mmfile_t *orig, const char *orig_name,
 			  mmfile_t *src1, const char *name1,
@@ -189,7 +189,7 @@ static void create_temp(mmfile_t *src, char *path, size_t len)
  * User defined low-level merge driver support.
  */
 static enum ll_merge_result ll_ext_merge(const struct ll_merge_driver *fn,
-			mmbuffer_t *result,
+			mmfile_t *result,
 			const char *path,
 			mmfile_t *orig, const char *orig_name,
 			mmfile_t *src1, const char *name1,
@@ -403,7 +403,7 @@ static void normalize_file(mmfile_t *mm, const char *path, struct index_state *i
 	}
 }
 
-enum ll_merge_result ll_merge(mmbuffer_t *result_buf,
+enum ll_merge_result ll_merge(mmfile_t *result_buf,
 	     const char *path,
 	     mmfile_t *ancestor, const char *ancestor_label,
 	     mmfile_t *ours, const char *our_label,
diff --git a/merge-ll.h b/merge-ll.h
index f26aef238d..f95332c682 100644
--- a/merge-ll.h
+++ b/merge-ll.h
@@ -16,7 +16,7 @@
  *   If you have no special requests, skip this and pass `NULL`
  *   as the `opts` parameter to use the default options.
  *
- * - Allocate an mmbuffer_t variable for the result.
+ * - Allocate an mmfile_t variable for the result.
  *
  * - Allocate and fill variables with the file's original content
  *   and two modified versions (using `read_mmfile`, for example).
@@ -100,7 +100,7 @@ enum ll_merge_result {
  * `.gitattributes` or `.git/info/attributes` into account.
  * Returns 0 for a clean merge.
  */
-enum ll_merge_result ll_merge(mmbuffer_t *result_buf,
+enum ll_merge_result ll_merge(mmfile_t *result_buf,
 	     const char *path,
 	     mmfile_t *ancestor, const char *ancestor_label,
 	     mmfile_t *ours, const char *our_label,
diff --git a/merge-ort.c b/merge-ort.c
index c410a5d353..1d3d193d35 100644
--- a/merge-ort.c
+++ b/merge-ort.c
@@ -2111,7 +2111,7 @@ static int merge_3way(struct merge_options *opt,
 		      const struct object_id *b,
 		      const char *pathnames[3],
 		      const int extra_marker_size,
-		      mmbuffer_t *result_buf)
+		      mmfile_t *result_buf)
 {
 	mmfile_t orig, src1, src2;
 	struct ll_merge_options ll_opts = LL_MERGE_OPTIONS_INIT;
@@ -2247,7 +2247,7 @@ static int handle_content_merge(struct merge_options *opt,
 
 	/* Remaining rules depend on file vs. submodule vs. symlink. */
 	else if (S_ISREG(a->mode)) {
-		mmbuffer_t result_buf;
+		mmfile_t result_buf;
 		int ret = 0, merge_status;
 		int two_way;
 
diff --git a/notes-merge.c b/notes-merge.c
index 118cad2518..d361e70a48 100644
--- a/notes-merge.c
+++ b/notes-merge.c
@@ -355,7 +355,7 @@ static void write_note_to_worktree(const struct object_id *obj,
 static int ll_merge_in_worktree(struct notes_merge_options *o,
 				struct notes_merge_pair *p)
 {
-	mmbuffer_t result_buf;
+	mmfile_t result_buf;
 	mmfile_t base, local, remote;
 	enum ll_merge_result status;
 
diff --git a/rerere.c b/rerere.c
index 856347c9ae..8696f8e7b7 100644
--- a/rerere.c
+++ b/rerere.c
@@ -597,7 +597,7 @@ int rerere_remaining(struct repository *r, struct string_list *merge_rr)
  */
 static int try_merge(struct index_state *istate,
 		     const struct rerere_id *id, const char *path,
-		     mmfile_t *cur, mmbuffer_t *result)
+		     mmfile_t *cur, mmfile_t *result)
 {
 	enum ll_merge_result ret;
 	mmfile_t base = {NULL, 0}, other = {NULL, 0};
@@ -638,7 +638,7 @@ static int merge(struct index_state *istate, const struct rerere_id *id, const c
 	int ret;
 	struct strbuf buf = STRBUF_INIT;
 	mmfile_t cur = {NULL, 0};
-	mmbuffer_t result = {NULL, 0};
+	mmfile_t result = {NULL, 0};
 
 	/*
 	 * Normalize the conflicts in path and write it out to
@@ -947,7 +947,7 @@ static int handle_cache(struct index_state *istate,
 			const char *path, unsigned char *hash, const char *output)
 {
 	mmfile_t mmfile[3] = {{NULL}};
-	mmbuffer_t result = {NULL, 0};
+	mmfile_t result = {NULL, 0};
 	const struct cache_entry *ce;
 	int pos, len, i, has_conflicts;
 	struct rerere_io_mem io;
@@ -1040,7 +1040,7 @@ static int rerere_forget_one_path(struct index_state *istate,
 	     id->variant < id->collection->status_nr;
 	     id->variant++) {
 		mmfile_t cur;
-		mmbuffer_t result = {NULL, 0};
+		mmfile_t result = {NULL, 0};
 		int cleanly_resolved;
 
 		if (!has_rerere_resolution(id))
diff --git a/xdiff-interface.c b/xdiff-interface.c
index e3dd2184ae..bc340d5a8a 100644
--- a/xdiff-interface.c
+++ b/xdiff-interface.c
@@ -53,7 +53,7 @@ static int consume_one(void *priv_, char *s, unsigned long size)
 	return 0;
 }
 
-static int xdiff_outf(void *priv_, mmbuffer_t *mb, int nbuf)
+static int xdiff_outf(void *priv_, mmfile_t *mb, int nbuf)
 {
 	struct xdiff_emit_state *priv = priv_;
 	int i;
diff --git a/xdiff/xdiff.h b/xdiff/xdiff.h
index dc370712e9..334eb436f6 100644
--- a/xdiff/xdiff.h
+++ b/xdiff/xdiff.h
@@ -73,11 +73,6 @@ typedef struct s_mmfile {
 	long size;
 } mmfile_t;
 
-typedef struct s_mmbuffer {
-	char *ptr;
-	long size;
-} mmbuffer_t;
-
 typedef struct s_xpparam {
 	unsigned long flags;
 
@@ -96,7 +91,7 @@ typedef struct s_xdemitcb {
 			long old_begin, long old_nr,
 			long new_begin, long new_nr,
 			const char *func, long funclen);
-	int (*out_line)(void *, mmbuffer_t *, int);
+	int (*out_line)(void *, mmfile_t *, int);
 } xdemitcb_t;
 
 typedef long (*find_func_t)(const char *line, long line_len, char *buffer, long buffer_size, void *priv);
@@ -144,7 +139,7 @@ typedef struct s_xmparam {
 #define DEFAULT_CONFLICT_MARKER_SIZE 7
 
 int xdl_merge(mmfile_t *orig, mmfile_t *mf1, mmfile_t *mf2,
-		xmparam_t const *xmp, mmbuffer_t *result);
+		xmparam_t const *xmp, mmfile_t *result);
 
 #ifdef __cplusplus
 }
diff --git a/xdiff/xmerge.c b/xdiff/xmerge.c
index 659ad4ec97..7b37968d25 100644
--- a/xdiff/xmerge.c
+++ b/xdiff/xmerge.c
@@ -504,7 +504,7 @@ static int xdl_simplify_non_conflicts(xdfenv_t *xe1, xdmerge_t *m,
  */
 static int xdl_do_merge(xdfenv_t *xe1, xdchange_t *xscr1,
 		xdfenv_t *xe2, xdchange_t *xscr2,
-		xmparam_t const *xmp, mmbuffer_t *result)
+		xmparam_t const *xmp, mmfile_t *result)
 {
 	xdmerge_t *changes, *c;
 	xpparam_t const *xpp = &xmp->xpp;
@@ -682,7 +682,7 @@ static int xdl_do_merge(xdfenv_t *xe1, xdchange_t *xscr1,
 }
 
 int xdl_merge(mmfile_t *orig, mmfile_t *mf1, mmfile_t *mf2,
-		xmparam_t const *xmp, mmbuffer_t *result)
+		xmparam_t const *xmp, mmfile_t *result)
 {
 	xdchange_t *xscr1 = NULL, *xscr2 = NULL;
 	xdfenv_t xe1, xe2;
diff --git a/xdiff/xutils.c b/xdiff/xutils.c
index 9a999acdc0..4215f646c5 100644
--- a/xdiff/xutils.c
+++ b/xdiff/xutils.c
@@ -39,7 +39,7 @@ uint64_t xdl_bogosqrt(uint64_t n) {
 int xdl_emit_diffrec(char const *rec, long size, char const *pre, long psize,
 		     xdemitcb_t *ecb) {
 	int i = 2;
-	mmbuffer_t mb[3];
+	mmfile_t mb[3];
 
 	mb[0].ptr = (char *) pre;
 	mb[0].size = psize;
@@ -392,7 +392,7 @@ static int xdl_format_hunk_hdr(long s1, long c1, long s2, long c2,
 			       const char *func, long funclen,
 			       xdemitcb_t *ecb) {
 	int nb = 0;
-	mmbuffer_t mb;
+	mmfile_t mb;
 	char buf[128];
 
 	memcpy(buf, "@@ -", 4);
-- 
2.56.0.354.gb6b32d5be5

[PATCH v2 3/7] xdiff: use size_t for buffer sizes

[Jeff King]

An mmfile_t stores its size as a signed long, but the more natural type
for a buffer size is size_t. This not only limits the size of entry we
can hold, but also creates some possible integer overflow issues.

For example, read_mmfile() checks that the file size fits in a size_t
before allocating, but then assigns it to a long. Likewise,
read_mmblob() and fill_mmfile() copy sizes from other types without
checking that they fit.

On LP64 systems like Linux, this is mostly academic. You could wrap to a
negative long value, but you'd need an object that's 2^63 bytes, which
is impractical.

But on an LLP64 system like Windows, a 2^31+1-byte blob could perhaps
cause mischief. We do prevent large values from entering the xdiff code
due to MAX_XDIFF_SIZE (which is itself marked as unsigned, so we'd
convert any negative "long" back to a large unsigned value). But if you
ask for binary diffs, that negative long value could instead be
converted to a huge 64-bit size_t when passed to memcmp(), diff_delta(),
etc. So probably there are paths that can cause an out-of-bounds read,
given the right set of options, but I didn't really dig for them.

On a 32-bit system things are less clear. Because "long" and "size_t"
have the same width, any time we implicitly convert to size_t, we should
get back the original size (even if the intermediate "long" is itself
negative). Probably iterating using a long could be a problem, but most
of that happens inside xdiff, which is protected by MAX_XDIFF_SIZE
(which, again, compares in the unsigned space).

Let's just use the obvious size_t type for counting the bytes. I suspect
you could still find truncation problems on LLP64 systems due to the use
of "unsigned long" throughout the code, but that's a larger problem.
This should at least nudge us in the right direction.

Note that we have to update the printf format in emit_binary_diff_body()
to accommodate the new type. Curiously it was using "%lu", even though
the type was signed (I guess compiler printf-linting is happy enough if
just the width of the format and the type match).

Signed-off-by: Jeff King <peff@peff.net>
---
 diff.c        | 2 +-
 xdiff/xdiff.h | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/diff.c b/diff.c
index 414532d09f..b4ac17f8ef 100644
--- a/diff.c
+++ b/diff.c
@@ -3646,7 +3646,7 @@ static void emit_binary_diff_body(struct diff_options *o,
 		data = delta;
 		data_size = delta_size;
 	} else {
-		char *s = xstrfmt("%lu", two->size);
+		char *s = xstrfmt("%"PRIuMAX, (uintmax_t)two->size);
 		emit_diff_symbol(o, DIFF_SYMBOL_BINARY_DIFF_HEADER_LITERAL,
 				 s, strlen(s), 0);
 		free(s);
diff --git a/xdiff/xdiff.h b/xdiff/xdiff.h
index 334eb436f6..8fa513fc4e 100644
--- a/xdiff/xdiff.h
+++ b/xdiff/xdiff.h
@@ -70,7 +70,7 @@ extern "C" {
 
 typedef struct s_mmfile {
 	char *ptr;
-	long size;
+	size_t size;
 } mmfile_t;
 
 typedef struct s_xpparam {
-- 
2.56.0.354.gb6b32d5be5

[PATCH v2 4/7] xdiff: NUL-terminate buffers read by read_mmfile()

[Jeff King]

Since an mmfile_t is a ptr/len pair, our read_mmfile() allocates exactly
the number of bytes we claim to store. But in many other places in Git,
we add an extra NUL "just in case", which can help avoid read overruns
due to off-by-ones or the use of string functions.

I don't know of any path that would benefit from this, but I noticed it
while converting ll_ext_merge() to use read_mmfile(), since its original
code did add a NUL byte (even though I cannot find any case where it
would have mattered). Let's add the same defensive NUL in read_mmfile()
by using xmallocz() instead of xmalloc().

Note that the matching read_mmblob() doesn't need the same treatment.
Its buffers already have a NUL from the object-reading code (which uses
the same defensive trick).

As a bonus, we can get rid of the hack in read_mmfile() to handle empty
files by allocating a single byte.

Signed-off-by: Jeff King <peff@peff.net>
---
 xdiff-interface.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xdiff-interface.c b/xdiff-interface.c
index bc340d5a8a..b3e9f1952b 100644
--- a/xdiff-interface.c
+++ b/xdiff-interface.c
@@ -166,7 +166,7 @@ int read_mmfile(mmfile_t *ptr, const char *filename)
 	if (!(f = fopen(filename, "rb")))
 		return error_errno("Could not open %s", filename);
 	sz = xsize_t(st.st_size);
-	ptr->ptr = xmalloc(sz ? sz : 1);
+	ptr->ptr = xmallocz(sz);
 	if (sz && fread(ptr->ptr, sz, 1, f) != 1) {
 		FREE_AND_NULL(ptr->ptr);
 		fclose(f);
-- 
2.56.0.354.gb6b32d5be5

Re: [PATCH v2 4/7] xdiff: NUL-terminate buffers read by read_mmfile()

[Patrick Steinhardt]

On Wed, Sep 30, 2026 at 07:44:13PM -0400, Jeff King wrote:
> Since an mmfile_t is a ptr/len pair, our read_mmfile() allocates exactly
> the number of bytes we claim to store. But in many other places in Git,
> we add an extra NUL "just in case", which can help avoid read overruns
> due to off-by-ones or the use of string functions.
> 
> I don't know of any path that would benefit from this, but I noticed it
> while converting ll_ext_merge() to use read_mmfile(), since its original
> code did add a NUL byte (even though I cannot find any case where it
> would have mattered). Let's add the same defensive NUL in read_mmfile()
> by using xmallocz() instead of xmalloc().

Nit: I guess this is an artifact from the reorder, but this sounds as if
`ll_ext_merge()` wouldn't append the NUL byte anymore. But at this step
it still does, as the change to `read_mmfile()` now happens before the
change to `ll_ext_merge()`.

Patrick

[PATCH v2 5/7] merge-ll: use read_mmfile() to read external merge results

[Jeff King]

After running an external merge driver, ll_ext_merge() reads the result
back from a temporary file. We can do the same thing with much less code
by using read_mmfile().

There are also two behavior improvements.

One, read_mmfile() correctly uses xsize_t() to detect the case when we'd
truncate the result.

And two, read_mmfile() will report errors to stderr if it can't read the
file (whereas the existing code silently returned NULL). I think most
callers would have said _something_ in this case like "failed to execute
merge" (from merge-ort), but more specifics are probably helpful (e.g.,
to distinguish a random system error from a badly configured merge
driver).

Signed-off-by: Jeff King <peff@peff.net>
---
 merge-ll.c | 21 +++++----------------
 1 file changed, 5 insertions(+), 16 deletions(-)

diff --git a/merge-ll.c b/merge-ll.c
index dfed6411a8..7fab7c5438 100644
--- a/merge-ll.c
+++ b/merge-ll.c
@@ -201,8 +201,7 @@ static enum ll_merge_result ll_ext_merge(const struct ll_merge_driver *fn,
 	struct strbuf cmd = STRBUF_INIT;
 	const char *format = fn->cmdline;
 	struct child_process child = CHILD_PROCESS_INIT;
-	int status, fd, i;
-	struct stat st;
+	int status, i;
 	enum ll_merge_result ret;
 	assert(opts);
 
@@ -241,20 +240,10 @@ static enum ll_merge_result ll_ext_merge(const struct ll_merge_driver *fn,
 	child.use_shell = 1;
 	strvec_push(&child.args, cmd.buf);
 	status = run_command(&child);
-	fd = open(temp[1], O_RDONLY);
-	if (fd < 0)
-		goto bad;
-	if (fstat(fd, &st))
-		goto close_bad;
-	result->size = st.st_size;
-	result->ptr = xmallocz(result->size);
-	if (read_in_full(fd, result->ptr, result->size) != result->size) {
-		FREE_AND_NULL(result->ptr);
-		result->size = 0;
-	}
- close_bad:
-	close(fd);
- bad:
+
+	/* We can ignore errors; result is left NULL/0 in that case. */
+	read_mmfile(result, temp[1]);
+
 	for (i = 0; i < 3; i++)
 		unlink_or_warn(temp[i]);
 	strbuf_release(&cmd);
-- 
2.56.0.354.gb6b32d5be5

Re: [PATCH v2 5/7] merge-ll: use read_mmfile() to read external merge results

[Patrick Steinhardt]

On Wed, Sep 30, 2026 at 07:44:16PM -0400, Jeff King wrote:
> After running an external merge driver, ll_ext_merge() reads the result
> back from a temporary file. We can do the same thing with much less code
> by using read_mmfile().
> 
> There are also two behavior improvements.
> 
> One, read_mmfile() correctly uses xsize_t() to detect the case when we'd
> truncate the result.
> 
> And two, read_mmfile() will report errors to stderr if it can't read the
> file (whereas the existing code silently returned NULL). I think most
> callers would have said _something_ in this case like "failed to execute
> merge" (from merge-ort), but more specifics are probably helpful (e.g.,
> to distinguish a random system error from a badly configured merge
> driver).

Okay. Those code paths would now print two error messages, but that's
probably fine.

Patrick

[PATCH v2 6/7] merge-ll: handle external driver status before reading result

[Jeff King]

After running an external merge driver, ll_ext_merge() reads its output
and cleans up the temporary files before converting the exit status to
an ll_merge_result.

Move that conversion immediately after run_command(). This will let us
override the result if reading the output fails, without having to fake
an exit status. No behavior change yet.

It is tempting to only call read_mmfile() when we have LL_MERGE_OK, but
callers do care about the result even with LL_MERGE_CONFLICT (e.g., the
output may contain a partial). I think we could safely skip it for
LL_MERGE_ERROR, but that's a rare case and not worth complicating the
code for.

Signed-off-by: Jeff King <peff@peff.net>
---
 merge-ll.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/merge-ll.c b/merge-ll.c
index 7fab7c5438..4d82836bc5 100644
--- a/merge-ll.c
+++ b/merge-ll.c
@@ -240,20 +240,20 @@ static enum ll_merge_result ll_ext_merge(const struct ll_merge_driver *fn,
 	child.use_shell = 1;
 	strvec_push(&child.args, cmd.buf);
 	status = run_command(&child);
-
-	/* We can ignore errors; result is left NULL/0 in that case. */
-	read_mmfile(result, temp[1]);
-
-	for (i = 0; i < 3; i++)
-		unlink_or_warn(temp[i]);
-	strbuf_release(&cmd);
 	if (!status)
 		ret = LL_MERGE_OK;
 	else if (status <= 128)
 		ret = LL_MERGE_CONFLICT;
 	else
 		/* died due to a signal: WTERMSIG(status) + 128 */
 		ret = LL_MERGE_ERROR;
+
+	/* We can ignore errors; result is left NULL/0 in that case. */
+	read_mmfile(result, temp[1]);
+
+	for (i = 0; i < 3; i++)
+		unlink_or_warn(temp[i]);
+	strbuf_release(&cmd);
 	return ret;
 }
 
-- 
2.56.0.354.gb6b32d5be5

[PATCH v2 7/7] merge-ll: report an error when reading external merge results fails

[Jeff King]

If we can't read an external merge driver's output, ll_ext_merge()
leaves the result buffer as NULL but returns a status based only on the
driver's exit code. So a driver which exits successfully can cause us to
return LL_MERGE_OK without a result.

Most callers of ll_merge() check for a NULL buffer in addition to an
error return, so they're fine. But rerere's merge() checks only the
return value, and may write out the (incorrect) empty result as the
recorded resolution.

Let's return LL_MERGE_ERROR when read_mmfile() fails, regardless of the
driver's exit status, to make it clear that the returned value is not
valid.

Our test is a little funny; the bad case happens when reading back the
file happens to fail. That can happen due to system errors, but of
course we want it to be deterministic. We can make that happen by
removing the result file. But if we configure a driver that always does
that, we'd never record a rerere result in the first place! So we
instead create a driver that "breaks" the read only when we instruct it
to do so.

Signed-off-by: Jeff King <peff@peff.net>
---
 merge-ll.c        |  4 ++--
 t/t4200-rerere.sh | 51 +++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 53 insertions(+), 2 deletions(-)

diff --git a/merge-ll.c b/merge-ll.c
index 4d82836bc5..3b5327e7df 100644
--- a/merge-ll.c
+++ b/merge-ll.c
@@ -248,8 +248,8 @@ static enum ll_merge_result ll_ext_merge(const struct ll_merge_driver *fn,
 		/* died due to a signal: WTERMSIG(status) + 128 */
 		ret = LL_MERGE_ERROR;
 
-	/* We can ignore errors; result is left NULL/0 in that case. */
-	read_mmfile(result, temp[1]);
+	if (read_mmfile(result, temp[1]) < 0)
+		ret = LL_MERGE_ERROR;
 
 	for (i = 0; i < 3; i++)
 		unlink_or_warn(temp[i]);
diff --git a/t/t4200-rerere.sh b/t/t4200-rerere.sh
index 7bb601e117..5be3f056f5 100755
--- a/t/t4200-rerere.sh
+++ b/t/t4200-rerere.sh
@@ -734,4 +734,55 @@ test_expect_success 'rerere does not crash with unmatched conflict marker' '
 	test_must_fail git rebase --continue
 '
 
+test_expect_success 'rerere preserves conflicts when driver output is unreadable' '
+	test_create_repo unreadable-output &&
+	(
+		cd unreadable-output &&
+		git config rerere.enabled true &&
+		git config rerere.autoupdate true &&
+		write_script merge-driver <<-\EOF &&
+		git merge-file "$@"
+		status=$?
+		if test -f fail-read
+		then
+			rm "$1" || exit 1
+		fi
+		exit "$status"
+		EOF
+		git config merge.unreadable.driver "./merge-driver %A %O %B" &&
+		echo "file merge=unreadable" >.gitattributes &&
+		test_commit base file base &&
+		git checkout -b one &&
+		test_commit --no-tag one file one &&
+		git checkout -b two base &&
+		test_commit --no-tag two file two &&
+
+		# Teach rerere a resolution while the driver works normally.
+		test_must_fail git merge one &&
+		echo resolved >file &&
+		git rerere &&
+		git merge --abort &&
+
+		# Recreate the conflict without replaying the resolution yet.
+		test_must_fail git -c rerere.enabled=false merge one &&
+
+		# We will expect the same conflicted content after rerere fails
+		# below.
+		cp file expect &&
+		git ls-files -u >expect-index &&
+		test_file_not_empty expect-index &&
+
+		# Now we try rerere again, but the merge driver will cause the
+		# read to fail.
+		>fail-read &&
+		git rerere 2>err &&
+		test_grep "Could not stat" err &&
+
+		# And we expect the conflicted state.
+		test_cmp expect file &&
+		git ls-files -u >actual-index &&
+		test_cmp expect-index actual-index
+	)
+'
+
 test_done
-- 
2.56.0.354.gb6b32d5be5