From f22cc52e9df3c08a15c082abf41595516e160294 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Wed, 30 Sep 2026 19:44:02 -0400 Subject: [PATCH 1/7] xdiff: clean up read_mmfile() allocations on error 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 Signed-off-by: Junio C Hamano --- 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); } From a28d2bfe19586dd1dd066c30f93581b48bbdcbc8 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Wed, 30 Sep 2026 19:44:06 -0400 Subject: [PATCH 2/7] xdiff: replace mmbuffer_t with mmfile_t 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 Signed-off-by: Junio C Hamano --- 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); From 3c7a31204ff383acf47c4a088ce711676bd7252f Mon Sep 17 00:00:00 2001 From: Jeff King Date: Wed, 30 Sep 2026 19:44:10 -0400 Subject: [PATCH 3/7] xdiff: use size_t for buffer sizes 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 Signed-off-by: Junio C Hamano --- 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 { From 25b334d8dbcae2683e60484b026a84eda4aa8c22 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Wed, 30 Sep 2026 19:44:13 -0400 Subject: [PATCH 4/7] xdiff: NUL-terminate buffers read by read_mmfile() 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 Signed-off-by: Junio C Hamano --- 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); From c9a589810288c4ca9716fda54497df49484a9c81 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Wed, 30 Sep 2026 19:44:16 -0400 Subject: [PATCH 5/7] merge-ll: use read_mmfile() to read external merge results 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 Signed-off-by: Junio C Hamano --- 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); From 3c00e11eabd0dbbd9e9c78e23acfe83f5f065e2f Mon Sep 17 00:00:00 2001 From: Jeff King Date: Wed, 30 Sep 2026 19:44:18 -0400 Subject: [PATCH 6/7] merge-ll: handle external driver status before reading result 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 Signed-off-by: Junio C Hamano --- 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,13 +240,6 @@ 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) @@ -254,6 +247,13 @@ static enum ll_merge_result ll_ext_merge(const struct ll_merge_driver *fn, 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; } From 5b0456361d23bf6ffe9539dd6f323a02c69c14b4 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Wed, 30 Sep 2026 19:44:20 -0400 Subject: [PATCH 7/7] 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 Signed-off-by: Junio C Hamano --- 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