{"thread":{"id":"66416","subject":"[PATCH 0/5] use size_t for xdiff mmfile_t","startedAt":"2026-09-29T06:49:37Z","lastAt":"2026-10-01T15:41:01Z","messageCount":37,"participants":["Jeff King","D. Ben Knoble","Junio C Hamano","Patrick Steinhardt"],"isPatch":true,"patchVersion":1,"patchTotal":5},"messages":[{"id":"553546","messageId":"20260929064935.GA1276867@coredump.intra.peff.net","threadId":"66416","inReplyTo":null,"subject":"[PATCH 0/5] use size_t for xdiff mmfile_t","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-09-29T06:49:35Z","receivedAt":"2026-09-29T06:49:37Z","isPatch":true,"body":"An earlier series tried to simplify ll_ext_merge()'s code to read back\nthe merge result from a temporary file, but Elijah pointed out some\nsubtle integer overflow confusion:\n\n https://lore.kernel.org/git/CABPp-BG9Hkc7i_JxAbYfyzu+b4Mc_pZUr0jJF=vY0jHSARpHzw@mail.gmail.com/\n\nI dug a little bit and found that similar problems exist elsewhere. So\nhere's an attempt to make things at least incrementally better. And\npatch 4 is the original cleanup I set out to do. ;)\n\nThere are a few textual conflicts with the v3 of\njk/merge-ll-tempfile-cleanup that I just sent out. They should be\neasy-ish to resolve, but I'm happy to just base this on that topic if\nit's easier.\n\n [1/5]: xdiff: clean up read_mmfile() allocations on error\n [2/5]: xdiff: replace mmbuffer_t with mmfile_t\n [3/5]: xdiff: use size_t for buffer sizes\n [4/5]: merge-ll: use read_mmfile() to read external merge results\n [5/5]: xdiff: NUL-terminate buffers read by read_mmfile()\n\n Documentation/technical/api-merge.adoc | 7 +++---\n apply.c | 2 +-\n builtin/checkout.c | 2 +-\n builtin/merge-file.c | 2 +-\n builtin/merge-tree.c | 2 +-\n builtin/rerere.c | 8 +++++--\n diff.c | 2 +-\n merge-blobs.c | 2 +-\n merge-ll.c | 33 +++++++++-----------------\n merge-ll.h | 4 ++--\n merge-ort.c | 4 ++--\n notes-merge.c | 2 +-\n rerere.c | 11 ++++-----\n xdiff-interface.c | 5 ++--\n xdiff/xdiff.h | 11 +++------\n xdiff/xmerge.c | 4 ++--\n xdiff/xutils.c | 4 ++--\n 17 files changed, 46 insertions(+), 59 deletions(-)\n\n-Peff\n"},{"id":"553547","messageId":"20260929065131.GA1697497@coredump.intra.peff.net","threadId":"66416","inReplyTo":"20260929064935.GA1276867@coredump.intra.peff.net","subject":"[PATCH 1/5] xdiff: clean up read_mmfile() allocations on error","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-09-29T06:51:31Z","receivedAt":"2026-09-29T06:51:32Z","isPatch":true,"body":"When read_mmfile() returns an error, it may or may not have allocated a\nbuffer in the passed-in mmfile_t. So callers must initialize the pointer\nto NULL and free it even on error.\n\nMost callers do this already, but rerere's diff_two() does not, and\nwould leak the buffer after a read error. We could fix it directly, but\nlet's instead try to make the interface less error-prone by freeing the\nmemory when returning failure from read_mmfile().\n\nThis fixes (part of) the leak in diff_two(). In theory it also lets us\nsimplify other callers to skip initializing the mmfile. But in practice\nmost still need zero-initialization because they may jump to free()\nbefore even calling read_mmfile (e.g., in try_merge()). But we can at\nleast simplify rerere_forget_one_path() a bit.\n\nI said \"part of\" earlier. There's a related leak in diff_two(): if\nreading the first file succeeds but reading the second fails, we return\nearly and leak the first buffer. We can fix that by checking each\nindividually.\n\nSigned-off-by: Jeff King \n---\nI found this while reading the code, but never actually triggered it in\npractice. It would require some way of having fopen() succeed and\nfread() fail.\n\n builtin/rerere.c | 6 +++++-\n rerere.c | 3 +--\n xdiff-interface.c | 1 +\n 3 files changed, 7 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/rerere.c b/builtin/rerere.c\nindex a056cb791b..d39c6e8445 100644\n--- a/builtin/rerere.c\n+++ b/builtin/rerere.c\n@@ -34,8 +34,12 @@ static int diff_two(const char *file1, const char *label1,\n \tmmfile_t minus, plus;\n \tint ret;\n \n-\tif (read_mmfile(&minus, file1) || read_mmfile(&plus, file2))\n+\tif (read_mmfile(&minus, file1))\n \t\treturn -1;\n+\tif (read_mmfile(&plus, file2)) {\n+\t\tfree(minus.ptr);\n+\t\treturn -1;\n+\t}\n \n \tprintf(\"--- a/%s\\n+++ b/%s\\n\", label1, label2);\n \tfflush(stdout);\ndiff --git a/rerere.c b/rerere.c\nindex 1c3745d9e3..856347c9ae 100644\n--- a/rerere.c\n+++ b/rerere.c\n@@ -1039,7 +1039,7 @@ static int rerere_forget_one_path(struct index_state *istate,\n \tfor (id->variant = 0;\n \t id->variant < id->collection->status_nr;\n \t id->variant++) {\n-\t\tmmfile_t cur = { NULL, 0 };\n+\t\tmmfile_t cur;\n \t\tmmbuffer_t result = {NULL, 0};\n \t\tint cleanly_resolved;\n \n@@ -1048,7 +1048,6 @@ static int rerere_forget_one_path(struct index_state *istate,\n \n \t\thandle_cache(istate, path, hash, rerere_path(&buf, id, \"thisimage\"));\n \t\tif (read_mmfile(&cur, rerere_path(&buf, id, \"thisimage\"))) {\n-\t\t\tfree(cur.ptr);\n \t\t\terror(_(\"failed to update conflicted state in '%s'\"), path);\n \t\t\tgoto fail_exit;\n \t\t}\ndiff --git a/xdiff-interface.c b/xdiff-interface.c\nindex db6938689f..e3dd2184ae 100644\n--- a/xdiff-interface.c\n+++ b/xdiff-interface.c\n@@ -168,6 +168,7 @@ int read_mmfile(mmfile_t *ptr, const char *filename)\n \tsz = xsize_t(st.st_size);\n \tptr->ptr = xmalloc(sz ? sz : 1);\n \tif (sz && fread(ptr->ptr, sz, 1, f) != 1) {\n+\t\tFREE_AND_NULL(ptr->ptr);\n \t\tfclose(f);\n \t\treturn error(\"Could not read %s\", filename);\n \t}\n-- \n2.56.0.325.g545d7e68bc\n\n"},{"id":"553548","messageId":"20260929065239.GB1697497@coredump.intra.peff.net","threadId":"66416","inReplyTo":"20260929064935.GA1276867@coredump.intra.peff.net","subject":"[PATCH 2/5] xdiff: replace mmbuffer_t with mmfile_t","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-09-29T06:52:39Z","receivedAt":"2026-09-29T06:52:41Z","isPatch":true,"body":"Our import of xdiff has two identical buffer structures: mmfile_t and\nmmbuffer_t. In upstream xdiff these were actually different, but the\nimport in 3443546f6e (Use a *real* built-in diff generator, 2006-03-24)\nsimplified mmfile_t to a simple buffer.\n\nIn xdiff we usually use mmfile_t for input and mmbuffer_t for output,\nbut they are really both just a ptr/len pair. I don't think that having\ndifferent types is buying us anything in terms of type safety or\nsemantics, and having two makes it awkward to use the same helpers for\nboth. In particular, an external merge driver's output is read from a\nfile, but we can't easily use read_mmfile(), since we want the result in\nan mmbuffer_t.\n\nLet's use mmfile_t for both cases and drop mmbuffer_t. The latter is\nprobably a more descriptive name, but we have many more uses of\nmmfile_t (and helpers like read_mmfile). So let's consolidate using that\nname; we can always change it to something more sensible later.\n\nThere should be no behavior change here; this is just consolidating the\ntypes.\n\nSigned-off-by: Jeff King \n---\nI guess this step might be controversial, but I hope not. I think the\nship has long sailed on trying to pull \"upstream\" changes from xdiff\n(there haven't been any, and we've hacked it up quite a bit already).\n\n Documentation/technical/api-merge.adoc | 7 +++----\n apply.c | 2 +-\n builtin/checkout.c | 2 +-\n builtin/merge-file.c | 2 +-\n builtin/merge-tree.c | 2 +-\n builtin/rerere.c | 2 +-\n merge-blobs.c | 2 +-\n merge-ll.c | 12 ++++++------\n merge-ll.h | 4 ++--\n merge-ort.c | 4 ++--\n notes-merge.c | 2 +-\n rerere.c | 8 ++++----\n xdiff-interface.c | 2 +-\n xdiff/xdiff.h | 9 ++-------\n xdiff/xmerge.c | 4 ++--\n xdiff/xutils.c | 4 ++--\n 16 files changed, 31 insertions(+), 37 deletions(-)\n\ndiff --git a/Documentation/technical/api-merge.adoc b/Documentation/technical/api-merge.adoc\nindex c2ba01828c..b691599393 100644\n--- a/Documentation/technical/api-merge.adoc\n+++ b/Documentation/technical/api-merge.adoc\n@@ -20,11 +20,10 @@ responsible for a few things.\n Data structures\n ---------------\n \n-* `mmbuffer_t`, `mmfile_t`\n+* `mmfile_t`\n \n-These store data usable for use by the xdiff backend, for writing and\n-for reading, respectively. See `xdiff/xdiff.h` for the definitions\n-and `diff.c` for examples.\n+This stores a buffer and its size for input to or output from the xdiff\n+backend. See `xdiff/xdiff.h` for the definition and `diff.c` for examples.\n \n * `struct ll_merge_options`\n \ndiff --git a/apply.c b/apply.c\nindex f00b7ba4d3..faf3c1dba0 100644\n--- a/apply.c\n+++ b/apply.c\n@@ -3646,7 +3646,7 @@ static int three_way_merge(struct apply_state *state,\n {\n \tmmfile_t base_file, our_file, their_file;\n \tstruct ll_merge_options merge_opts = LL_MERGE_OPTIONS_INIT;\n-\tmmbuffer_t result = { NULL };\n+\tmmfile_t result = { NULL };\n \tenum ll_merge_result status;\n \n \t/* resolve trivial cases first */\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex c0f0d2c700..2d575a4f57 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -320,7 +320,7 @@ static int checkout_merged(int pos, const struct checkout *state,\n \tenum ll_merge_result merge_status;\n \tint status;\n \tstruct object_id oid;\n-\tmmbuffer_t result_buf;\n+\tmmfile_t result_buf;\n \tstruct object_id threeway[3];\n \tunsigned mode = 0;\n \tstruct ll_merge_options ll_opts = LL_MERGE_OPTIONS_INIT;\ndiff --git a/builtin/merge-file.c b/builtin/merge-file.c\nindex 8fa5765239..ddca408c46 100644\n--- a/builtin/merge-file.c\n+++ b/builtin/merge-file.c\n@@ -64,7 +64,7 @@ int cmd_merge_file(int argc,\n {\n \tconst char *names[3] = { 0 };\n \tmmfile_t mmfs[3] = { 0 };\n-\tmmbuffer_t result = { 0 };\n+\tmmfile_t result = { 0 };\n \txmparam_t xmp = { 0 };\n \tint ret = 0, i = 0, to_stdout = 0, object_id = 0;\n \tint quiet = 0;\ndiff --git a/builtin/merge-tree.c b/builtin/merge-tree.c\nindex 49f41e520f..552c2ad736 100644\n--- a/builtin/merge-tree.c\n+++ b/builtin/merge-tree.c\n@@ -109,7 +109,7 @@ static void *origin(struct merge_list *entry, size_t *size)\n \treturn NULL;\n }\n \n-static int show_outf(void *priv UNUSED, mmbuffer_t *mb, int nbuf)\n+static int show_outf(void *priv UNUSED, mmfile_t *mb, int nbuf)\n {\n \tint i;\n \tfor (i = 0; i < nbuf; i++)\ndiff --git a/builtin/rerere.c b/builtin/rerere.c\nindex d39c6e8445..ef03b79f5b 100644\n--- a/builtin/rerere.c\n+++ b/builtin/rerere.c\n@@ -16,7 +16,7 @@ static const char * const rerere_usage[] = {\n \tNULL,\n };\n \n-static int outf(void *dummy UNUSED, mmbuffer_t *ptr, int nbuf)\n+static int outf(void *dummy UNUSED, mmfile_t *ptr, int nbuf)\n {\n \tint i;\n \tfor (i = 0; i < nbuf; i++)\ndiff --git a/merge-blobs.c b/merge-blobs.c\nindex 16a75bd1e3..49dbec9529 100644\n--- a/merge-blobs.c\n+++ b/merge-blobs.c\n@@ -38,7 +38,7 @@ static void *three_way_filemerge(struct index_state *istate,\n \t\t\t\t size_t *size)\n {\n \tenum ll_merge_result merge_status;\n-\tmmbuffer_t res;\n+\tmmfile_t res;\n \n \t/*\n \t * This function is only used by cmd_merge_tree, which\ndiff --git a/merge-ll.c b/merge-ll.c\nindex ef5287dee8..dfed6411a8 100644\n--- a/merge-ll.c\n+++ b/merge-ll.c\n@@ -21,7 +21,7 @@\n struct ll_merge_driver;\n \n typedef enum ll_merge_result (*ll_merge_fn)(const struct ll_merge_driver *,\n-\t\t\t mmbuffer_t *result,\n+\t\t\t mmfile_t *result,\n \t\t\t const char *path,\n \t\t\t mmfile_t *orig, const char *orig_name,\n \t\t\t mmfile_t *src1, const char *name1,\n@@ -56,7 +56,7 @@ void reset_merge_attributes(void)\n * Built-in low-levels\n */\n static enum ll_merge_result ll_binary_merge(const struct ll_merge_driver *drv UNUSED,\n-\t\t\t mmbuffer_t *result,\n+\t\t\t mmfile_t *result,\n \t\t\t const char *path UNUSED,\n \t\t\t mmfile_t *orig, const char *orig_name UNUSED,\n \t\t\t mmfile_t *src1, const char *name1 UNUSED,\n@@ -101,7 +101,7 @@ static enum ll_merge_result ll_binary_merge(const struct ll_merge_driver *drv UN\n }\n \n static enum ll_merge_result ll_xdl_merge(const struct ll_merge_driver *drv_unused,\n-\t\t\tmmbuffer_t *result,\n+\t\t\tmmfile_t *result,\n \t\t\tconst char *path,\n \t\t\tmmfile_t *orig, const char *orig_name,\n \t\t\tmmfile_t *src1, const char *name1,\n@@ -147,7 +147,7 @@ static enum ll_merge_result ll_xdl_merge(const struct ll_merge_driver *drv_unuse\n }\n \n static enum ll_merge_result ll_union_merge(const struct ll_merge_driver *drv_unused,\n-\t\t\t mmbuffer_t *result,\n+\t\t\t mmfile_t *result,\n \t\t\t const char *path,\n \t\t\t mmfile_t *orig, const char *orig_name,\n \t\t\t mmfile_t *src1, const char *name1,\n@@ -189,7 +189,7 @@ static void create_temp(mmfile_t *src, char *path, size_t len)\n * User defined low-level merge driver support.\n */\n static enum ll_merge_result ll_ext_merge(const struct ll_merge_driver *fn,\n-\t\t\tmmbuffer_t *result,\n+\t\t\tmmfile_t *result,\n \t\t\tconst char *path,\n \t\t\tmmfile_t *orig, const char *orig_name,\n \t\t\tmmfile_t *src1, const char *name1,\n@@ -403,7 +403,7 @@ static void normalize_file(mmfile_t *mm, const char *path, struct index_state *i\n \t}\n }\n \n-enum ll_merge_result ll_merge(mmbuffer_t *result_buf,\n+enum ll_merge_result ll_merge(mmfile_t *result_buf,\n \t const char *path,\n \t mmfile_t *ancestor, const char *ancestor_label,\n \t mmfile_t *ours, const char *our_label,\ndiff --git a/merge-ll.h b/merge-ll.h\nindex f26aef238d..f95332c682 100644\n--- a/merge-ll.h\n+++ b/merge-ll.h\n@@ -16,7 +16,7 @@\n * If you have no special requests, skip this and pass `NULL`\n * as the `opts` parameter to use the default options.\n *\n- * - Allocate an mmbuffer_t variable for the result.\n+ * - Allocate an mmfile_t variable for the result.\n *\n * - Allocate and fill variables with the file's original content\n * and two modified versions (using `read_mmfile`, for example).\n@@ -100,7 +100,7 @@ enum ll_merge_result {\n * `.gitattributes` or `.git/info/attributes` into account.\n * Returns 0 for a clean merge.\n */\n-enum ll_merge_result ll_merge(mmbuffer_t *result_buf,\n+enum ll_merge_result ll_merge(mmfile_t *result_buf,\n \t const char *path,\n \t mmfile_t *ancestor, const char *ancestor_label,\n \t mmfile_t *ours, const char *our_label,\ndiff --git a/merge-ort.c b/merge-ort.c\nindex c410a5d353..1d3d193d35 100644\n--- a/merge-ort.c\n+++ b/merge-ort.c\n@@ -2111,7 +2111,7 @@ static int merge_3way(struct merge_options *opt,\n \t\t const struct object_id *b,\n \t\t const char *pathnames[3],\n \t\t const int extra_marker_size,\n-\t\t mmbuffer_t *result_buf)\n+\t\t mmfile_t *result_buf)\n {\n \tmmfile_t orig, src1, src2;\n \tstruct ll_merge_options ll_opts = LL_MERGE_OPTIONS_INIT;\n@@ -2247,7 +2247,7 @@ static int handle_content_merge(struct merge_options *opt,\n \n \t/* Remaining rules depend on file vs. submodule vs. symlink. */\n \telse if (S_ISREG(a->mode)) {\n-\t\tmmbuffer_t result_buf;\n+\t\tmmfile_t result_buf;\n \t\tint ret = 0, merge_status;\n \t\tint two_way;\n \ndiff --git a/notes-merge.c b/notes-merge.c\nindex 118cad2518..d361e70a48 100644\n--- a/notes-merge.c\n+++ b/notes-merge.c\n@@ -355,7 +355,7 @@ static void write_note_to_worktree(const struct object_id *obj,\n static int ll_merge_in_worktree(struct notes_merge_options *o,\n \t\t\t\tstruct notes_merge_pair *p)\n {\n-\tmmbuffer_t result_buf;\n+\tmmfile_t result_buf;\n \tmmfile_t base, local, remote;\n \tenum ll_merge_result status;\n \ndiff --git a/rerere.c b/rerere.c\nindex 856347c9ae..8696f8e7b7 100644\n--- a/rerere.c\n+++ b/rerere.c\n@@ -597,7 +597,7 @@ int rerere_remaining(struct repository *r, struct string_list *merge_rr)\n */\n static int try_merge(struct index_state *istate,\n \t\t const struct rerere_id *id, const char *path,\n-\t\t mmfile_t *cur, mmbuffer_t *result)\n+\t\t mmfile_t *cur, mmfile_t *result)\n {\n \tenum ll_merge_result ret;\n \tmmfile_t base = {NULL, 0}, other = {NULL, 0};\n@@ -638,7 +638,7 @@ static int merge(struct index_state *istate, const struct rerere_id *id, const c\n \tint ret;\n \tstruct strbuf buf = STRBUF_INIT;\n \tmmfile_t cur = {NULL, 0};\n-\tmmbuffer_t result = {NULL, 0};\n+\tmmfile_t result = {NULL, 0};\n \n \t/*\n \t * Normalize the conflicts in path and write it out to\n@@ -947,7 +947,7 @@ static int handle_cache(struct index_state *istate,\n \t\t\tconst char *path, unsigned char *hash, const char *output)\n {\n \tmmfile_t mmfile[3] = {{NULL}};\n-\tmmbuffer_t result = {NULL, 0};\n+\tmmfile_t result = {NULL, 0};\n \tconst struct cache_entry *ce;\n \tint pos, len, i, has_conflicts;\n \tstruct rerere_io_mem io;\n@@ -1040,7 +1040,7 @@ static int rerere_forget_one_path(struct index_state *istate,\n \t id->variant < id->collection->status_nr;\n \t id->variant++) {\n \t\tmmfile_t cur;\n-\t\tmmbuffer_t result = {NULL, 0};\n+\t\tmmfile_t result = {NULL, 0};\n \t\tint cleanly_resolved;\n \n \t\tif (!has_rerere_resolution(id))\ndiff --git a/xdiff-interface.c b/xdiff-interface.c\nindex e3dd2184ae..bc340d5a8a 100644\n--- a/xdiff-interface.c\n+++ b/xdiff-interface.c\n@@ -53,7 +53,7 @@ static int consume_one(void *priv_, char *s, unsigned long size)\n \treturn 0;\n }\n \n-static int xdiff_outf(void *priv_, mmbuffer_t *mb, int nbuf)\n+static int xdiff_outf(void *priv_, mmfile_t *mb, int nbuf)\n {\n \tstruct xdiff_emit_state *priv = priv_;\n \tint i;\ndiff --git a/xdiff/xdiff.h b/xdiff/xdiff.h\nindex dc370712e9..334eb436f6 100644\n--- a/xdiff/xdiff.h\n+++ b/xdiff/xdiff.h\n@@ -73,11 +73,6 @@ typedef struct s_mmfile {\n \tlong size;\n } mmfile_t;\n \n-typedef struct s_mmbuffer {\n-\tchar *ptr;\n-\tlong size;\n-} mmbuffer_t;\n-\n typedef struct s_xpparam {\n \tunsigned long flags;\n \n@@ -96,7 +91,7 @@ typedef struct s_xdemitcb {\n \t\t\tlong old_begin, long old_nr,\n \t\t\tlong new_begin, long new_nr,\n \t\t\tconst char *func, long funclen);\n-\tint (*out_line)(void *, mmbuffer_t *, int);\n+\tint (*out_line)(void *, mmfile_t *, int);\n } xdemitcb_t;\n \n typedef long (*find_func_t)(const char *line, long line_len, char *buffer, long buffer_size, void *priv);\n@@ -144,7 +139,7 @@ typedef struct s_xmparam {\n #define DEFAULT_CONFLICT_MARKER_SIZE 7\n \n int xdl_merge(mmfile_t *orig, mmfile_t *mf1, mmfile_t *mf2,\n-\t\txmparam_t const *xmp, mmbuffer_t *result);\n+\t\txmparam_t const *xmp, mmfile_t *result);\n \n #ifdef __cplusplus\n }\ndiff --git a/xdiff/xmerge.c b/xdiff/xmerge.c\nindex 659ad4ec97..7b37968d25 100644\n--- a/xdiff/xmerge.c\n+++ b/xdiff/xmerge.c\n@@ -504,7 +504,7 @@ static int xdl_simplify_non_conflicts(xdfenv_t *xe1, xdmerge_t *m,\n */\n static int xdl_do_merge(xdfenv_t *xe1, xdchange_t *xscr1,\n \t\txdfenv_t *xe2, xdchange_t *xscr2,\n-\t\txmparam_t const *xmp, mmbuffer_t *result)\n+\t\txmparam_t const *xmp, mmfile_t *result)\n {\n \txdmerge_t *changes, *c;\n \txpparam_t const *xpp = &xmp->xpp;\n@@ -682,7 +682,7 @@ static int xdl_do_merge(xdfenv_t *xe1, xdchange_t *xscr1,\n }\n \n int xdl_merge(mmfile_t *orig, mmfile_t *mf1, mmfile_t *mf2,\n-\t\txmparam_t const *xmp, mmbuffer_t *result)\n+\t\txmparam_t const *xmp, mmfile_t *result)\n {\n \txdchange_t *xscr1 = NULL, *xscr2 = NULL;\n \txdfenv_t xe1, xe2;\ndiff --git a/xdiff/xutils.c b/xdiff/xutils.c\nindex 9a999acdc0..4215f646c5 100644\n--- a/xdiff/xutils.c\n+++ b/xdiff/xutils.c\n@@ -39,7 +39,7 @@ uint64_t xdl_bogosqrt(uint64_t n) {\n int xdl_emit_diffrec(char const *rec, long size, char const *pre, long psize,\n \t\t xdemitcb_t *ecb) {\n \tint i = 2;\n-\tmmbuffer_t mb[3];\n+\tmmfile_t mb[3];\n \n \tmb[0].ptr = (char *) pre;\n \tmb[0].size = psize;\n@@ -392,7 +392,7 @@ static int xdl_format_hunk_hdr(long s1, long c1, long s2, long c2,\n \t\t\t const char *func, long funclen,\n \t\t\t xdemitcb_t *ecb) {\n \tint nb = 0;\n-\tmmbuffer_t mb;\n+\tmmfile_t mb;\n \tchar buf[128];\n \n \tmemcpy(buf, \"@@ -\", 4);\n-- \n2.56.0.325.g545d7e68bc\n\n"},{"id":"553549","messageId":"20260929065414.GC1697497@coredump.intra.peff.net","threadId":"66416","inReplyTo":"20260929064935.GA1276867@coredump.intra.peff.net","subject":"[PATCH 3/5] xdiff: use size_t for buffer sizes","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-09-29T06:54:14Z","receivedAt":"2026-09-29T06:54:16Z","isPatch":true,"body":"An mmfile_t stores its size as a signed long, but the more natural type\nfor a buffer size is size_t. This not only limits the size of entry we\ncan hold, but also creates some possible integer overflow issues.\n\nFor example, read_mmfile() checks that the file size fits in a size_t\nbefore allocating, but then assigns it to a long. Likewise,\nread_mmblob() and fill_mmfile() copy sizes from other types without\nchecking that they fit.\n\nOn LP64 systems like Linux, this is mostly academic. You could wrap to a\nnegative long value, but you'd need an object that's 2^63 bytes, which\nis impractical.\n\nBut on an LLP64 system like Windows, a 2^31+1-byte blob could perhaps\ncause mischief. We do prevent large values from entering the xdiff code\ndue to MAX_XDIFF_SIZE (which is itself marked as unsigned, so we'd\nconvert any negative \"long\" back to a large unsigned value). But if you\nask for binary diffs, that negative long value could instead be\nconverted to a huge 64-bit size_t when passed to memcmp(), diff_delta(),\netc. So probably there are paths that can cause an out-of-bounds read,\ngiven the right set of options, but I didn't really dig for them.\n\nOn a 32-bit system things are less clear. Because \"long\" and \"size_t\"\nhave the same width, any time we implicitly convert to size_t, we should\nget back the original size (even if the intermediate \"long\" is itself\nnegative). Probably iterating using a long could be a problem, but most\nof that happens inside xdiff, which is protected by MAX_XDIFF_SIZE\n(which, again, compares in the unsigned space).\n\nLet's just use the obvious size_t type for counting the bytes. I suspect\nyou could still find truncation problems on LLP64 systems due to the use\nof \"unsigned long\" throughout the code, but that's a larger problem.\nThis should at least nudge us in the right direction.\n\nNote that we have to update the printf format in emit_binary_diff_body()\nto accommodate the new type. Curiously it was using \"%lu\", even though\nthe type was signed (I guess compiler printf-linting is happy enough if\njust the width of the format and the type match).\n\nSigned-off-by: Jeff King \n---\n diff.c | 2 +-\n xdiff/xdiff.h | 2 +-\n 2 files changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 414532d09f..b4ac17f8ef 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -3646,7 +3646,7 @@ static void emit_binary_diff_body(struct diff_options *o,\n \t\tdata = delta;\n \t\tdata_size = delta_size;\n \t} else {\n-\t\tchar *s = xstrfmt(\"%lu\", two->size);\n+\t\tchar *s = xstrfmt(\"%\"PRIuMAX, (uintmax_t)two->size);\n \t\temit_diff_symbol(o, DIFF_SYMBOL_BINARY_DIFF_HEADER_LITERAL,\n \t\t\t\t s, strlen(s), 0);\n \t\tfree(s);\ndiff --git a/xdiff/xdiff.h b/xdiff/xdiff.h\nindex 334eb436f6..8fa513fc4e 100644\n--- a/xdiff/xdiff.h\n+++ b/xdiff/xdiff.h\n@@ -70,7 +70,7 @@ extern \"C\" {\n \n typedef struct s_mmfile {\n \tchar *ptr;\n-\tlong size;\n+\tsize_t size;\n } mmfile_t;\n \n typedef struct s_xpparam {\n-- \n2.56.0.325.g545d7e68bc\n\n"},{"id":"553550","messageId":"20260929065442.GD1697497@coredump.intra.peff.net","threadId":"66416","inReplyTo":"20260929064935.GA1276867@coredump.intra.peff.net","subject":"[PATCH 4/5] merge-ll: use read_mmfile() to read external merge results","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-09-29T06:54:42Z","receivedAt":"2026-09-29T06:54:43Z","isPatch":true,"body":"After running an external merge driver, ll_ext_merge() reads the result\nback from a temporary file. We can do the same thing with much less code\nby using read_mmfile().\n\nAs a bonus, note that read_mmfile() correctly uses xsize_t() to detect\nthe case when we'd truncate the result.\n\nSigned-off-by: Jeff King \n---\n merge-ll.c | 21 +++++----------------\n 1 file changed, 5 insertions(+), 16 deletions(-)\n\ndiff --git a/merge-ll.c b/merge-ll.c\nindex dfed6411a8..7fab7c5438 100644\n--- a/merge-ll.c\n+++ b/merge-ll.c\n@@ -201,8 +201,7 @@ static enum ll_merge_result ll_ext_merge(const struct ll_merge_driver *fn,\n \tstruct strbuf cmd = STRBUF_INIT;\n \tconst char *format = fn->cmdline;\n \tstruct child_process child = CHILD_PROCESS_INIT;\n-\tint status, fd, i;\n-\tstruct stat st;\n+\tint status, i;\n \tenum ll_merge_result ret;\n \tassert(opts);\n \n@@ -241,20 +240,10 @@ static enum ll_merge_result ll_ext_merge(const struct ll_merge_driver *fn,\n \tchild.use_shell = 1;\n \tstrvec_push(&child.args, cmd.buf);\n \tstatus = run_command(&child);\n-\tfd = open(temp[1], O_RDONLY);\n-\tif (fd < 0)\n-\t\tgoto bad;\n-\tif (fstat(fd, &st))\n-\t\tgoto close_bad;\n-\tresult->size = st.st_size;\n-\tresult->ptr = xmallocz(result->size);\n-\tif (read_in_full(fd, result->ptr, result->size) != result->size) {\n-\t\tFREE_AND_NULL(result->ptr);\n-\t\tresult->size = 0;\n-\t}\n- close_bad:\n-\tclose(fd);\n- bad:\n+\n+\t/* We can ignore errors; result is left NULL/0 in that case. */\n+\tread_mmfile(result, temp[1]);\n+\n \tfor (i = 0; i < 3; i++)\n \t\tunlink_or_warn(temp[i]);\n \tstrbuf_release(&cmd);\n-- \n2.56.0.325.g545d7e68bc\n\n"},{"id":"553551","messageId":"20260929065504.GE1697497@coredump.intra.peff.net","threadId":"66416","inReplyTo":"20260929064935.GA1276867@coredump.intra.peff.net","subject":"[PATCH 5/5] xdiff: NUL-terminate buffers read by read_mmfile()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-09-29T06:55:04Z","receivedAt":"2026-09-29T06:55:05Z","isPatch":true,"body":"Since an mmfile_t is a ptr/len pair, our read_mmfile() allocates exactly\nthe number of bytes we claim to store. But in many other places in Git,\nwe add an extra NUL \"just in case\", which can help avoid read overruns\ndue to off-by-ones or the use of string functions.\n\nI don't know of any path that would benefit from this, but I noticed it\nwhile converting ll_ext_merge() to use read_mmfile(), since its original\ncode did add a NUL byte (even though I cannot find any case where it\nwould have mattered). Let's teach read_mmfile() to add this defensive\nNUL; it probably doesn't help anything, but nor should it hurt.\n\nNote that the matching read_mmblob() doesn't need the same treatment.\nIts buffers already have a NUL from the object-reading code (which uses\nthe same defensive trick).\n\nAs a bonus, we can get rid of the hack in read_mmfile() to handle empty\nfiles by allocating a single byte.\n\nSigned-off-by: Jeff King \n---\nThis one is obviously optional, which is why I put it last.\n\n xdiff-interface.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/xdiff-interface.c b/xdiff-interface.c\nindex bc340d5a8a..b3e9f1952b 100644\n--- a/xdiff-interface.c\n+++ b/xdiff-interface.c\n@@ -166,7 +166,7 @@ int read_mmfile(mmfile_t *ptr, const char *filename)\n \tif (!(f = fopen(filename, \"rb\")))\n \t\treturn error_errno(\"Could not open %s\", filename);\n \tsz = xsize_t(st.st_size);\n-\tptr->ptr = xmalloc(sz ? sz : 1);\n+\tptr->ptr = xmallocz(sz);\n \tif (sz && fread(ptr->ptr, sz, 1, f) != 1) {\n \t\tFREE_AND_NULL(ptr->ptr);\n \t\tfclose(f);\n-- \n2.56.0.325.g545d7e68bc\n"},{"id":"553580","messageId":"CALnO6CCW8K1bajbk3jqS54bP1=jyiYqnqZVRK66yO594ChoASQ@mail.gmail.com","threadId":"66416","inReplyTo":"20260929065239.GB1697497@coredump.intra.peff.net","subject":"Re: [PATCH 2/5] xdiff: replace mmbuffer_t with mmfile_t","fromName":"D. Ben Knoble","fromEmail":"ben.knoble@gmail.com","sentAt":"2026-09-29T11:08:22Z","receivedAt":"2026-09-29T11:08:34Z","isPatch":true,"body":"On Tue, Sep 29, 2026 at 2:57 AM Jeff King wrote:\n>\n> Our import of xdiff has two identical buffer structures: mmfile_t and\n> mmbuffer_t. In upstream xdiff these were actually different, but the\n> import in 3443546f6e (Use a *real* built-in diff generator, 2006-03-24)\n> simplified mmfile_t to a simple buffer.\n\n[snip]\n\n> I guess this step might be controversial, but I hope not. I think the\n> ship has long sailed on trying to pull \"upstream\" changes from xdiff\n> (there haven't been any, and we've hacked it up quite a bit already).\n\nI think for a while Vim has also pulled in xdiff from Git (since our\ncopy is actively maintained), but I might have that wrong. I think\nthey decided to stop doing so after the Rust bits merged?\n\nAt any rate, I don't think that should be a strong (or even weak)\nobjection to consolidating our code. Thanks.\n"},{"id":"553628","messageId":"xmqqbj9fhpkj.fsf@gitster.g","threadId":"66416","inReplyTo":"20260929065131.GA1697497@coredump.intra.peff.net","subject":"Re: [PATCH 1/5] xdiff: clean up read_mmfile() allocations on error","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-09-29T18:37:00Z","receivedAt":"2026-09-29T18:37:03Z","isPatch":true,"body":"Jeff King writes:\n\n> When read_mmfile() returns an error, it may or may not have allocated a\n> buffer in the passed-in mmfile_t. So callers must initialize the pointer\n> to NULL and free it even on error.\n>\n> Most callers do this already, but rerere's diff_two() does not, and\n> would leak the buffer after a read error. We could fix it directly, but\n> let's instead try to make the interface less error-prone by freeing the\n> memory when returning failure from read_mmfile().\n>\n> This fixes (part of) the leak in diff_two(). In theory it also lets us\n> simplify other callers to skip initializing the mmfile. But in practice\n> most still need zero-initialization because they may jump to free()\n> before even calling read_mmfile (e.g., in try_merge()). But we can at\n> least simplify rerere_forget_one_path() a bit.\n>\n> I said \"part of\" earlier. There's a related leak in diff_two(): if\n> reading the first file succeeds but reading the second fails, we return\n> early and leak the first buffer. We can fix that by checking each\n> individually.\n\nNice. Thanks for plugging my leaks.\n"},{"id":"553629","messageId":"xmqq7bk3hpfk.fsf@gitster.g","threadId":"66416","inReplyTo":"20260929065239.GB1697497@coredump.intra.peff.net","subject":"Re: [PATCH 2/5] xdiff: replace mmbuffer_t with mmfile_t","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-09-29T18:39:59Z","receivedAt":"2026-09-29T18:40:01Z","isPatch":true,"body":"Jeff King writes:\n\n> Our import of xdiff has two identical buffer structures: mmfile_t and\n> mmbuffer_t. In upstream xdiff these were actually different, but the\n> import in 3443546f6e (Use a *real* built-in diff generator, 2006-03-24)\n> simplified mmfile_t to a simple buffer.\n>\n> In xdiff we usually use mmfile_t for input and mmbuffer_t for output,\n> but they are really both just a ptr/len pair. I don't think that having\n> different types is buying us anything in terms of type safety or\n> semantics, and having two makes it awkward to use the same helpers for\n> both. In particular, an external merge driver's output is read from a\n> file, but we can't easily use read_mmfile(), since we want the result in\n> an mmbuffer_t.\n>\n> Let's use mmfile_t for both cases and drop mmbuffer_t. The latter is\n> probably a more descriptive name, but we have many more uses of\n> mmfile_t (and helpers like read_mmfile). So let's consolidate using that\n> name; we can always change it to something more sensible later.\n>\n> There should be no behavior change here; this is just consolidating the\n> types.\n\nObviously good.\n\n>\n> Signed-off-by: Jeff King \n> ---\n> I guess this step might be controversial, but I hope not. I think the\n> ship has long sailed on trying to pull \"upstream\" changes from xdiff\n> (there haven't been any, and we've hacked it up quite a bit already).\n\nI share your prediction that we will not be \"synchronizing\" with the\nupstream.\n"},{"id":"553632","messageId":"xmqqzewzg8w0.fsf@gitster.g","threadId":"66416","inReplyTo":"20260929065442.GD1697497@coredump.intra.peff.net","subject":"Re: [PATCH 4/5] merge-ll: use read_mmfile() to read external merge results","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-09-29T19:22:39Z","receivedAt":"2026-09-29T19:22:42Z","isPatch":true,"body":"Jeff King writes:\n\n> After running an external merge driver, ll_ext_merge() reads the result\n> back from a temporary file. We can do the same thing with much less code\n> by using read_mmfile().\n>\n> As a bonus, note that read_mmfile() correctly uses xsize_t() to detect\n> the case when we'd truncate the result.\n>\n> Signed-off-by: Jeff King \n> ---\n> merge-ll.c | 21 +++++----------------\n> 1 file changed, 5 insertions(+), 16 deletions(-)\n>\n> diff --git a/merge-ll.c b/merge-ll.c\n> index dfed6411a8..7fab7c5438 100644\n> --- a/merge-ll.c\n> +++ b/merge-ll.c\n> @@ -201,8 +201,7 @@ static enum ll_merge_result ll_ext_merge(const struct ll_merge_driver *fn,\n> \tstruct strbuf cmd = STRBUF_INIT;\n> \tconst char *format = fn->cmdline;\n> \tstruct child_process child = CHILD_PROCESS_INIT;\n> -\tint status, fd, i;\n> -\tstruct stat st;\n> +\tint status, i;\n> \tenum ll_merge_result ret;\n> \tassert(opts);\n> \n> @@ -241,20 +240,10 @@ static enum ll_merge_result ll_ext_merge(const struct ll_merge_driver *fn,\n> \tchild.use_shell = 1;\n> \tstrvec_push(&child.args, cmd.buf);\n> \tstatus = run_command(&child);\n> -\tfd = open(temp[1], O_RDONLY);\n> -\tif (fd < 0)\n> -\t\tgoto bad;\n> -\tif (fstat(fd, &st))\n> -\t\tgoto close_bad;\n> -\tresult->size = st.st_size;\n> -\tresult->ptr = xmallocz(result->size);\n> -\tif (read_in_full(fd, result->ptr, result->size) != result->size) {\n> -\t\tFREE_AND_NULL(result->ptr);\n> -\t\tresult->size = 0;\n> -\t}\n> - close_bad:\n> -\tclose(fd);\n> - bad:\n> +\n> +\t/* We can ignore errors; result is left NULL/0 in that case. */\n> +\tread_mmfile(result, temp[1]);\n> +\n> \tfor (i = 0; i < 3; i++)\n> \t\tunlink_or_warn(temp[i]);\n> \tstrbuf_release(&cmd);\n\nLets see if I understand why we can safely ignore errors.\n\nIf the external driver claims that it successfully merged (i.e.,\nstatus = run_command(&child) returns 0), and yet read_mmfile() fails\n(e.g., perhaps the driver unlinks \"%A\"), read_mmfile() will leave\nresult->ptr and result->size as initialized, and we return\nLL_MERGE_OK from this function. The result is eventually relayed to\nthe caller of ll_merge(), like merge-ort.c:merge_3way(), or\napply.c:three_way_merge(). Both have something like\n\n\tstatus = ll_merge(&result, path,\n\t\t\t &base_file, \"base\",\n\t\t\t &our_file, \"ours\",\n\t\t\t &their_file, \"theirs\",\n\t\t\t state->repo->index,\n\t\t\t &merge_opts);\n\tif (status == LL_MERGE_BINARY_CONFLICT)\n\t\twarning(\"Cannot merge binary files: %s (%s vs. %s)\",\n\t\t\tpath, \"ours\", \"theirs\");\n\tfree(base_file.ptr);\n\tfree(our_file.ptr);\n\tfree(their_file.ptr);\n\tif (status < 0 || !result.ptr) {\n\t\tfree(result.ptr);\n\t\treturn -1;\n\t}\n\nto treat that result.ptr==NULL is just as bad as any error from\nll_merge() (i.e., status < 0).\n\nmerge-blobs.c:merge_blobs() does not check the !result.ptr\ncondition, and its sole caller builtin/merge-tree.c:result() passes\nthe NULL to show_diff(), which uses a mmfile_t as one side\nof xdi_diff(), which the callee is prepared to handle, so this is OK.\n\nrerere.c:try_merge() does not check the !result.ptr condition, and\nits caller rerere.c:merge() ends up calling\n\n\tfwrite(NULL, (size_t)0, 1, f)\n\nwhich may happen to work on most systems, but is not exactly kosher.\n\nPerhaps something like this on top might make it safer? Not even\ncompile tested and I haven't thought through the ramifications to\nrerere.c:merge() code path, that used to take such a bogus merge\nresult as successful merge and relied on the fwrite(NULL) becoming\na no-op to produce an empty file.\n\n merge-ll.c | 16 +++++++++++++---\n merge-ll.h | 4 ++++\n 2 files changed, 17 insertions(+), 3 deletions(-)\n\ndiff --git c/merge-ll.c w/merge-ll.c\nindex 7fab7c5438..518c05636f 100644\n--- c/merge-ll.c\n+++ w/merge-ll.c\n@@ -405,6 +405,7 @@ enum ll_merge_result ll_merge(mmfile_t *result_buf,\n \tconst char *ll_driver_name = NULL;\n \tint marker_size = DEFAULT_CONFLICT_MARKER_SIZE;\n \tconst struct ll_merge_driver *driver;\n+\tenum ll_merge_result result;\n \n \tif (!opts)\n \t\topts = &default_opts;\n@@ -434,9 +435,18 @@ enum ll_merge_result ll_merge(mmfile_t *result_buf,\n \tif (opts->extra_marker_size) {\n \t\tmarker_size += opts->extra_marker_size;\n \t}\n-\treturn driver->fn(driver, result_buf, path, ancestor, ancestor_label,\n-\t\t\t ours, our_label, theirs, their_label,\n-\t\t\t opts, marker_size);\n+\tresult = driver->fn(driver, result_buf, path, ancestor, ancestor_label,\n+\t\t\t ours, our_label, theirs, their_label,\n+\t\t\t opts, marker_size);\n+\tif (!result_buf.ptr && result == LL_MERGE_OK) {\n+\t\t/*\n+\t\t * Forbid the driver from giving bogus result and claim\n+\t\t * that the merge succeeded.\n+\t\t */\n+\t\tresult = LL_MERGE_ERROR;\n+\t\tresult_buf.size = 0;\n+\t}\n+\treturn result;\n }\n \n int ll_merge_marker_size(struct index_state *istate, const char *path)\ndiff --git c/merge-ll.h w/merge-ll.h\nindex f95332c682..c5e11f397e 100644\n--- c/merge-ll.h\n+++ w/merge-ll.h\n@@ -23,6 +23,10 @@\n *\n * - Call `ll_merge()`.\n *\n+ * - Notice a merge error by checking return value from ll_merge(). If the\n+ * .ptr member of the result is NULL, that may indicate that we failed to\n+ * read the merge results from an external merge driver.\n+ *\n * - Read the merged content from `result_buf.ptr` and `result_buf.size`.\n *\n * - Release buffers when finished. A simple\n"},{"id":"553638","messageId":"20260929201134.GA1713437@coredump.intra.peff.net","threadId":"66416","inReplyTo":"xmqqzewzg8w0.fsf@gitster.g","subject":"Re: [PATCH 4/5] merge-ll: use read_mmfile() to read external merge results","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-09-29T20:11:34Z","receivedAt":"2026-09-29T20:11:36Z","isPatch":true,"body":"On Tue, Sep 29, 2026 at 12:22:39PM -0700, Junio C Hamano wrote:\n\n> > +\t/* We can ignore errors; result is left NULL/0 in that case. */\n> > +\tread_mmfile(result, temp[1]);\n> > +\n> > \tfor (i = 0; i < 3; i++)\n> > \t\tunlink_or_warn(temp[i]);\n> > \tstrbuf_release(&cmd);\n> \n> Lets see if I understand why we can safely ignore errors.\n> \n> If the external driver claims that it successfully merged (i.e.,\n> status = run_command(&child) returns 0), and yet read_mmfile() fails\n> (e.g., perhaps the driver unlinks \"%A\"), read_mmfile() will leave\n> result->ptr and result->size as initialized, and we return\n> LL_MERGE_OK from this function. The result is eventually relayed to\n> the caller of ll_merge(), like merge-ort.c:merge_3way(), or\n> apply.c:three_way_merge(). Both have something like\n> \n> \tstatus = ll_merge(&result, path,\n> \t\t\t &base_file, \"base\",\n> \t\t\t &our_file, \"ours\",\n> \t\t\t &their_file, \"theirs\",\n> \t\t\t state->repo->index,\n> \t\t\t &merge_opts);\n> \tif (status == LL_MERGE_BINARY_CONFLICT)\n> \t\twarning(\"Cannot merge binary files: %s (%s vs. %s)\",\n> \t\t\tpath, \"ours\", \"theirs\");\n> \tfree(base_file.ptr);\n> \tfree(our_file.ptr);\n> \tfree(their_file.ptr);\n> \tif (status < 0 || !result.ptr) {\n> \t\tfree(result.ptr);\n> \t\treturn -1;\n> \t}\n> \n> to treat that result.ptr==NULL is just as bad as any error from\n> ll_merge() (i.e., status < 0).\n\nYeah, exactly. This confused me quite a bit at first, and I thought I'd\nfound another bug. It feels like we should return LL_MERGE_ERROR for\nthis case (it is not the external merge driver's error, but rather ours,\nbut from the caller's perspective does it matter?).\n\nBut then I saw that the callers did check for NULL already (which is\nwhat the existing code reliably returned on error). So there's no bug,\nbut I agree it's subtle. For the purposes of this refactor I tried to\ndraw the line at retaining the same visible behavior from\nll_ext_merge(), just to keep scope creep to a minimum.\n\nBut I'm definitely not opposed to refactoring further on top, and I\nthink you may have actually found a bug below.\n\n> merge-blobs.c:merge_blobs() does not check the !result.ptr\n> condition, and its sole caller builtin/merge-tree.c:result() passes\n> the NULL to show_diff(), which uses a mmfile_t as one side\n> of xdi_diff(), which the callee is prepared to handle, so this is OK.\n> \n> rerere.c:try_merge() does not check the !result.ptr condition, and\n> its caller rerere.c:merge() ends up calling\n> \n> \tfwrite(NULL, (size_t)0, 1, f)\n> \n> which may happen to work on most systems, but is not exactly kosher.\n\nEven if it works and sends an empty output, I think it is the wrong\nbehavior. It's possible the driver actually returned a real output, but\nwe failed to read it in. And now we're propagating a bogus empty value\ninstead.\n\nIt's hard to test, though, because the easiest way to trigger a read\nfailure is for the driver to actually _not_ return an output (i.e., to\ndelete the %A file). And in that case it happens to coincide with the\ncorrect behavior. ;)\n\nI guess a more interesting one is one where the driver changes the mode\non %A so that it cannot be read.\n\nWe can trigger that case like this:\n\n-- >8 --\ngit init\n\necho base >file\ngit add file\ngit commit -am base\n\ngit checkout -b one\necho one >file\ngit commit -am one\n\ngit checkout -b two HEAD^\necho two >file\ngit commit -am two\n\ngit config merge.foo.driver 'echo result >%A; chmod 0 %A'\necho 'file merge=foo' >.gitattributes\n\ngit merge one\n-- 8< --\n\nBut I'm not sure how to convince rerere to work on it. The merge command\nproduces output like:\n\n error: Could not open /home/peff/tmp/repo/.merge_file_ma1Kcq: Permission denied\n error: failed to execute internal merge for file\n Merge with strategy ort failed.\n\nwhich is reasonable (probably mentioning the external driver would be\nbetter still, but at least we notice the problem).\n\nI guess to confuse rerere we probably have to do a regular merge, record\nthe result, and then configure our broken driver, and then try to merge\nto run rerere on the result.\n\nSo if we amend the end of that script to:\n\n-- >8 --\n# merge that records resolution (we abort here, but it\n# could just be that we create the same merge elsewhere)\ngit -c rerere.enabled=true merge one\necho result >file\ngit rerere\ngit reset --hard\n\n# now we merge in a way that creates the conflict again\ngit -c rerere.enabled=false merge one\n\n# but then in the middle we start using the broken driver\ngit config merge.foo.driver 'echo result >%A; chmod 0 %A'\necho 'file merge=foo' >.gitattributes\n\n# and now rerere gets confused; we claim to use the recorded\n# resolution, but it's incorrectly empty\ngit rerere\n-- 8< --\n\nThat sequence is quite fishy (changing the attributes mid-merge!?) but\nin theory it could trigger racily due to a system error, fread()\nfailing, and so on.\n\n> Perhaps something like this on top might make it safer? Not even\n> compile tested and I haven't thought through the ramifications to\n> rerere.c:merge() code path, that used to take such a bogus merge\n> result as successful merge and relied on the fwrite(NULL) becoming\n> a no-op to produce an empty file.\n\nThis does fix the case above (modulo some s/./->/ in your patch). We end\nup with the unresolved contents in \"file\".\n\n> +\tif (!result_buf.ptr && result == LL_MERGE_OK) {\n> +\t\t/*\n> +\t\t * Forbid the driver from giving bogus result and claim\n> +\t\t * that the merge succeeded.\n> +\t\t */\n> +\t\tresult = LL_MERGE_ERROR;\n> +\t\tresult_buf.size = 0;\n> +\t}\n\nI had imagined just fixing this in ll_ext_merge(), like:\n\ndiff --git a/merge-ll.c b/merge-ll.c\nindex 7fab7c5438..0e56e303fa 100644\n--- a/merge-ll.c\n+++ b/merge-ll.c\n@@ -241,8 +241,13 @@ static enum ll_merge_result ll_ext_merge(const struct ll_merge_driver *fn,\n \tstrvec_push(&child.args, cmd.buf);\n \tstatus = run_command(&child);\n \n-\t/* We can ignore errors; result is left NULL/0 in that case. */\n-\tread_mmfile(result, temp[1]);\n+\t/*\n+\t * fake a driver error when we can't read the result; a slightly more\n+\t * elegant solution is to hoist the status-to-ret conversion from\n+\t * below, and then we can directly assign ret = LL_MERGE_ERROR.\n+\t */\n+\tif (read_mmfile(result, temp[1]) < 0)\n+\t\tstatus = 129;\n \n \tfor (i = 0; i < 3; i++)\n \t\tunlink_or_warn(temp[i]);\n\nwhich reduces the weirdness coming out of that function. But it wouldn't\nhelp with other drivers (which may or may not have similar problems? I'd\nguess not, since they are all operating internally).\n\n-Peff\n"},{"id":"553640","messageId":"20260929204157.GA1733321@coredump.intra.peff.net","threadId":"66416","inReplyTo":"20260929201134.GA1713437@coredump.intra.peff.net","subject":"Re: [PATCH 4/5] merge-ll: use read_mmfile() to read external merge results","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-09-29T20:41:57Z","receivedAt":"2026-09-29T20:41:58Z","isPatch":true,"body":"On Tue, Sep 29, 2026 at 04:11:34PM -0400, Jeff King wrote:\n\n> I had imagined just fixing this in ll_ext_merge(), like:\n> [...]\n> which reduces the weirdness coming out of that function. But it wouldn't\n> help with other drivers (which may or may not have similar problems? I'd\n> guess not, since they are all operating internally).\n\nSo here are patches to do that, including a cleaned-up version of the\nreproduction I posted.\n\nI think ll_ext_merge() is the only driver that has this weird error\ncase, so it should be sufficient. Your patch would protect a potential\nfuture driver, but I'd be surprised if we had one that introduced the\nsame NULL-but-not-an-error behavior.\n\n [6/5]: merge-ll: handle external driver status before reading result\n [7/5]: merge-ll: report an error when reading external merge results fails\n\n merge-ll.c | 14 ++++++-------\n t/t4200-rerere.sh | 51 +++++++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 58 insertions(+), 7 deletions(-)\n\n-Peff\n"},{"id":"553641","messageId":"20260929204320.GA1734030@coredump.intra.peff.net","threadId":"66416","inReplyTo":"20260929204157.GA1733321@coredump.intra.peff.net","subject":"[PATCH 6/5] merge-ll: handle external driver status before reading result","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-09-29T20:43:20Z","receivedAt":"2026-09-29T20:43:22Z","isPatch":true,"body":"After running an external merge driver, ll_ext_merge() reads its output\nand cleans up the temporary files before converting the exit status to\nan ll_merge_result.\n\nMove that conversion immediately after run_command(). This will let us\noverride the result if reading the output fails, without having to fake\nan exit status. No behavior change yet.\n\nIt is tempting to only call read_mmfile() when we have LL_MERGE_OK, but\ncallers do care about the result even with LL_MERGE_CONFLICT (e.g., the\noutput may contain a partial). I think we could safely skip it for\nLL_MERGE_ERROR, but that's a rare case and not worth complicating the\ncode for.\n\nSigned-off-by: Jeff King \n---\n merge-ll.c | 14 +++++++-------\n 1 file changed, 7 insertions(+), 7 deletions(-)\n\ndiff --git a/merge-ll.c b/merge-ll.c\nindex 7fab7c5438..4d82836bc5 100644\n--- a/merge-ll.c\n+++ b/merge-ll.c\n@@ -240,20 +240,20 @@ static enum ll_merge_result ll_ext_merge(const struct ll_merge_driver *fn,\n \tchild.use_shell = 1;\n \tstrvec_push(&child.args, cmd.buf);\n \tstatus = run_command(&child);\n-\n-\t/* We can ignore errors; result is left NULL/0 in that case. */\n-\tread_mmfile(result, temp[1]);\n-\n-\tfor (i = 0; i < 3; i++)\n-\t\tunlink_or_warn(temp[i]);\n-\tstrbuf_release(&cmd);\n \tif (!status)\n \t\tret = LL_MERGE_OK;\n \telse if (status <= 128)\n \t\tret = LL_MERGE_CONFLICT;\n \telse\n \t\t/* died due to a signal: WTERMSIG(status) + 128 */\n \t\tret = LL_MERGE_ERROR;\n+\n+\t/* We can ignore errors; result is left NULL/0 in that case. */\n+\tread_mmfile(result, temp[1]);\n+\n+\tfor (i = 0; i < 3; i++)\n+\t\tunlink_or_warn(temp[i]);\n+\tstrbuf_release(&cmd);\n \treturn ret;\n }\n \n-- \n2.56.0.325.g545d7e68bc\n\n"},{"id":"553642","messageId":"20260929204421.GB1734030@coredump.intra.peff.net","threadId":"66416","inReplyTo":"20260929204157.GA1733321@coredump.intra.peff.net","subject":"[PATCH 7/5] merge-ll: report an error when reading external merge results fails","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-09-29T20:44:21Z","receivedAt":"2026-09-29T20:44:24Z","isPatch":true,"body":"If we can't read an external merge driver's output, ll_ext_merge()\nleaves the result buffer as NULL but returns a status based only on the\ndriver's exit code. So a driver which exits successfully can cause us to\nreturn LL_MERGE_OK without a result.\n\nMost callers of ll_merge() check for a NULL buffer in addition to an\nerror return, so they're fine. But rerere's merge() checks only the\nreturn value, and may write out the (incorrect) empty result as the\nrecorded resolution.\n\nLet's return LL_MERGE_ERROR when read_mmfile() fails, regardless of the\ndriver's exit status, to make it clear that the returned value is not\nvalid.\n\nOur test is a little funny; the bad case happens when reading back the\nfile happens to fail. That can happen randomly due to system errors, but\nof course we want it to be deterministic. We can make that happen by\nbreaking the permissions on the result file. But if we configure a\ndriver that always does that, we'd never record a rerere result in the\nfirst place! So we instead create a driver that \"breaks\" the read only\nwhen we instruct it do so, simulating a flaky system.\n\nSigned-off-by: Jeff King \n---\n merge-ll.c | 4 ++--\n t/t4200-rerere.sh | 51 +++++++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 53 insertions(+), 2 deletions(-)\n\ndiff --git a/merge-ll.c b/merge-ll.c\nindex 4d82836bc5..3b5327e7df 100644\n--- a/merge-ll.c\n+++ b/merge-ll.c\n@@ -248,8 +248,8 @@ static enum ll_merge_result ll_ext_merge(const struct ll_merge_driver *fn,\n \t\t/* died due to a signal: WTERMSIG(status) + 128 */\n \t\tret = LL_MERGE_ERROR;\n \n-\t/* We can ignore errors; result is left NULL/0 in that case. */\n-\tread_mmfile(result, temp[1]);\n+\tif (read_mmfile(result, temp[1]) < 0)\n+\t\tret = LL_MERGE_ERROR;\n \n \tfor (i = 0; i < 3; i++)\n \t\tunlink_or_warn(temp[i]);\ndiff --git a/t/t4200-rerere.sh b/t/t4200-rerere.sh\nindex 7bb601e117..74945a4e3c 100755\n--- a/t/t4200-rerere.sh\n+++ b/t/t4200-rerere.sh\n@@ -734,4 +734,55 @@ test_expect_success 'rerere does not crash with unmatched conflict marker' '\n \ttest_must_fail git rebase --continue\n '\n \n+test_expect_success SANITY 'rerere preserves conflicts when driver output is unreadable' '\n+\ttest_create_repo unreadable-output &&\n+\t(\n+\t\tcd unreadable-output &&\n+\t\tgit config rerere.enabled true &&\n+\t\tgit config rerere.autoupdate true &&\n+\t\twrite_script merge-driver <<-\\EOF &&\n+\t\tgit merge-file \"$@\"\n+\t\tstatus=$?\n+\t\tif test -f fail-read\n+\t\tthen\n+\t\t\tchmod 0 \"$1\" || exit 1\n+\t\tfi\n+\t\texit \"$status\"\n+\t\tEOF\n+\t\tgit config merge.unreadable.driver \"./merge-driver %A %O %B\" &&\n+\t\techo \"file merge=unreadable\" >.gitattributes &&\n+\t\ttest_commit base file base &&\n+\t\tgit checkout -b one &&\n+\t\ttest_commit --no-tag one file one &&\n+\t\tgit checkout -b two base &&\n+\t\ttest_commit --no-tag two file two &&\n+\n+\t\t# Teach rerere a resolution while the driver works normally.\n+\t\ttest_must_fail git merge one &&\n+\t\techo resolved >file &&\n+\t\tgit rerere &&\n+\t\tgit merge --abort &&\n+\n+\t\t# Recreate the conflict without replaying the resolution yet.\n+\t\ttest_must_fail git -c rerere.enabled=false merge one &&\n+\n+\t\t# We will expect the same conflicted content after rerere fails\n+\t\t# below.\n+\t\tcp file expect &&\n+\t\tgit ls-files -u >expect-index &&\n+\t\ttest_file_not_empty expect-index &&\n+\n+\t\t# Now we try rerere again, but the merge driver will cause the\n+\t\t# read to fail.\n+\t\t>fail-read &&\n+\t\tgit rerere 2>err &&\n+\t\ttest_grep \"Could not open\" err &&\n+\n+\t\t# And we expect the conflicted state.\n+\t\ttest_cmp expect file &&\n+\t\tgit ls-files -u >actual-index &&\n+\t\ttest_cmp expect-index actual-index\n+\t)\n+'\n+\n test_done\n-- \n2.56.0.325.g545d7e68bc\n"},{"id":"553646","messageId":"xmqqv77neowx.fsf@gitster.g","threadId":"66416","inReplyTo":"20260929204421.GB1734030@coredump.intra.peff.net","subject":"Re: [PATCH 7/5] merge-ll: report an error when reading external merge results fails","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-09-29T21:19:26Z","receivedAt":"2026-09-29T21:19:29Z","isPatch":true,"body":"Jeff King writes:\n\n> +test_expect_success SANITY 'rerere preserves conflicts when driver output is unreadable' '\n> +\ttest_create_repo unreadable-output &&\n> +\t(\n\n> +\t\tcd unreadable-output &&\n> +\t\tgit config rerere.enabled true &&\n> +\t\tgit config rerere.autoupdate true &&\n> +\t\twrite_script merge-driver <<-\\EOF &&\n> +\t\tgit merge-file \"$@\"\n> +\t\tstatus=$?\n> +\t\tif test -f fail-read\n> +\t\tthen\n> +\t\t\tchmod 0 \"$1\" || exit 1\n> +\t\tfi\n\nCan we lose SANITY by \"rm $1\" instead of \"chmod 0\"?\n\n> +\t\texit \"$status\"\n> +\t\tEOF\n> +\t\tgit config merge.unreadable.driver \"./merge-driver %A %O %B\" &&\n> +\t\techo \"file merge=unreadable\" >.gitattributes &&\n> +\t\ttest_commit base file base &&\n> +\t\tgit checkout -b one &&\n> +\t\ttest_commit --no-tag one file one &&\n> +\t\tgit checkout -b two base &&\n> +\t\ttest_commit --no-tag two file two &&\n> +\n> +\t\t# Teach rerere a resolution while the driver works normally.\n> +\t\ttest_must_fail git merge one &&\n> +\t\techo resolved >file &&\n> +\t\tgit rerere &&\n> +\t\tgit merge --abort &&\n> +\n> +\t\t# Recreate the conflict without replaying the resolution yet.\n> +\t\ttest_must_fail git -c rerere.enabled=false merge one &&\n> +\n> +\t\t# We will expect the same conflicted content after rerere fails\n> +\t\t# below.\n> +\t\tcp file expect &&\n> +\t\tgit ls-files -u >expect-index &&\n> +\t\ttest_file_not_empty expect-index &&\n> +\n> +\t\t# Now we try rerere again, but the merge driver will cause the\n> +\t\t# read to fail.\n> +\t\t>fail-read &&\n> +\t\tgit rerere 2>err &&\n> +\t\ttest_grep \"Could not open\" err &&\n> +\n> +\t\t# And we expect the conflicted state.\n> +\t\ttest_cmp expect file &&\n> +\t\tgit ls-files -u >actual-index &&\n> +\t\ttest_cmp expect-index actual-index\n> +\t)\n> +'\n> +\n> test_done\n"},{"id":"553649","messageId":"20260929214943.GA1735259@coredump.intra.peff.net","threadId":"66416","inReplyTo":"xmqqv77neowx.fsf@gitster.g","subject":"Re: [PATCH 7/5] merge-ll: report an error when reading external merge results fails","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-09-29T21:49:43Z","receivedAt":"2026-09-29T21:49:48Z","isPatch":true,"body":"On Tue, Sep 29, 2026 at 02:19:26PM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > +test_expect_success SANITY 'rerere preserves conflicts when driver output is unreadable' '\n> > +\ttest_create_repo unreadable-output &&\n> > +\t(\n> \n> > +\t\tcd unreadable-output &&\n> > +\t\tgit config rerere.enabled true &&\n> > +\t\tgit config rerere.autoupdate true &&\n> > +\t\twrite_script merge-driver <<-\\EOF &&\n> > +\t\tgit merge-file \"$@\"\n> > +\t\tstatus=$?\n> > +\t\tif test -f fail-read\n> > +\t\tthen\n> > +\t\t\tchmod 0 \"$1\" || exit 1\n> > +\t\tfi\n> \n> Can we lose SANITY by \"rm $1\" instead of \"chmod 0\"?\n\nHmm, I guess we can. I was thinking for some reason that we need to fail\nlater in read_mmfile(). But I think I was just confusing that with the\nearlier leak fix. With a stat failure, read_mmfile() will leave the\nmmfile_t untouched, but we initialize it in ll_ext_merge() to NULL/0. So\nthe outcome should be the same from the caller's perspective.\n\nAnd that's actually a more realistic example, I think. Instead of\nsimulating a racy read failure, we are considering a driver that\nsometimes accidentally deletes the file while returning 0. Buggy, but a\nplausible bug. ;)\n\nHere's a resend of that final patch (not just a squash, because the\ncommit message mentioned the chmod).\n\n-- >8 --\nSubject: merge-ll: report an error when reading external merge results fails\n\nIf we can't read an external merge driver's output, ll_ext_merge()\nleaves the result buffer as NULL but returns a status based only on the\ndriver's exit code. So a driver which exits successfully can cause us to\nreturn LL_MERGE_OK without a result.\n\nMost callers of ll_merge() check for a NULL buffer in addition to an\nerror return, so they're fine. But rerere's merge() checks only the\nreturn value, and may write out the (incorrect) empty result as the\nrecorded resolution.\n\nLet's return LL_MERGE_ERROR when read_mmfile() fails, regardless of the\ndriver's exit status, to make it clear that the returned value is not\nvalid.\n\nOur test is a little funny; the bad case happens when reading back the\nfile happens to fail. That can happen due to system errors, but of\ncourse we want it to be deterministic. We can make that happen by\nremoving the result file. But if we configure a driver that always does\nthat, we'd never record a rerere result in the first place! So we\ninstead create a driver that \"breaks\" the read only when we instruct it\nto do so.\n\nSigned-off-by: Jeff King \n---\n merge-ll.c | 4 ++--\n t/t4200-rerere.sh | 51 +++++++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 53 insertions(+), 2 deletions(-)\n\ndiff --git a/merge-ll.c b/merge-ll.c\nindex 4d82836bc5..3b5327e7df 100644\n--- a/merge-ll.c\n+++ b/merge-ll.c\n@@ -248,8 +248,8 @@ static enum ll_merge_result ll_ext_merge(const struct ll_merge_driver *fn,\n \t\t/* died due to a signal: WTERMSIG(status) + 128 */\n \t\tret = LL_MERGE_ERROR;\n \n-\t/* We can ignore errors; result is left NULL/0 in that case. */\n-\tread_mmfile(result, temp[1]);\n+\tif (read_mmfile(result, temp[1]) < 0)\n+\t\tret = LL_MERGE_ERROR;\n \n \tfor (i = 0; i < 3; i++)\n \t\tunlink_or_warn(temp[i]);\ndiff --git a/t/t4200-rerere.sh b/t/t4200-rerere.sh\nindex 7bb601e117..5be3f056f5 100755\n--- a/t/t4200-rerere.sh\n+++ b/t/t4200-rerere.sh\n@@ -734,4 +734,55 @@ test_expect_success 'rerere does not crash with unmatched conflict marker' '\n \ttest_must_fail git rebase --continue\n '\n \n+test_expect_success 'rerere preserves conflicts when driver output is unreadable' '\n+\ttest_create_repo unreadable-output &&\n+\t(\n+\t\tcd unreadable-output &&\n+\t\tgit config rerere.enabled true &&\n+\t\tgit config rerere.autoupdate true &&\n+\t\twrite_script merge-driver <<-\\EOF &&\n+\t\tgit merge-file \"$@\"\n+\t\tstatus=$?\n+\t\tif test -f fail-read\n+\t\tthen\n+\t\t\trm \"$1\" || exit 1\n+\t\tfi\n+\t\texit \"$status\"\n+\t\tEOF\n+\t\tgit config merge.unreadable.driver \"./merge-driver %A %O %B\" &&\n+\t\techo \"file merge=unreadable\" >.gitattributes &&\n+\t\ttest_commit base file base &&\n+\t\tgit checkout -b one &&\n+\t\ttest_commit --no-tag one file one &&\n+\t\tgit checkout -b two base &&\n+\t\ttest_commit --no-tag two file two &&\n+\n+\t\t# Teach rerere a resolution while the driver works normally.\n+\t\ttest_must_fail git merge one &&\n+\t\techo resolved >file &&\n+\t\tgit rerere &&\n+\t\tgit merge --abort &&\n+\n+\t\t# Recreate the conflict without replaying the resolution yet.\n+\t\ttest_must_fail git -c rerere.enabled=false merge one &&\n+\n+\t\t# We will expect the same conflicted content after rerere fails\n+\t\t# below.\n+\t\tcp file expect &&\n+\t\tgit ls-files -u >expect-index &&\n+\t\ttest_file_not_empty expect-index &&\n+\n+\t\t# Now we try rerere again, but the merge driver will cause the\n+\t\t# read to fail.\n+\t\t>fail-read &&\n+\t\tgit rerere 2>err &&\n+\t\ttest_grep \"Could not stat\" err &&\n+\n+\t\t# And we expect the conflicted state.\n+\t\ttest_cmp expect file &&\n+\t\tgit ls-files -u >actual-index &&\n+\t\ttest_cmp expect-index actual-index\n+\t)\n+'\n+\n test_done\n-- \n2.56.0.325.g545d7e68bc\n\n"},{"id":"553719","messageId":"ar0roZKCwALv0n_A@pks.im","threadId":"66416","inReplyTo":"20260929065239.GB1697497@coredump.intra.peff.net","subject":"Re: [PATCH 2/5] xdiff: replace mmbuffer_t with mmfile_t","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-09-30T15:32:49Z","receivedAt":"2026-09-30T15:32:59Z","isPatch":true,"body":"On Tue, Sep 29, 2026 at 02:52:39AM -0400, Jeff King wrote:\n> Our import of xdiff has two identical buffer structures: mmfile_t and\n> mmbuffer_t. In upstream xdiff these were actually different, but the\n> import in 3443546f6e (Use a *real* built-in diff generator, 2006-03-24)\n> simplified mmfile_t to a simple buffer.\n> \n> In xdiff we usually use mmfile_t for input and mmbuffer_t for output,\n> but they are really both just a ptr/len pair. I don't think that having\n> different types is buying us anything in terms of type safety or\n> semantics, and having two makes it awkward to use the same helpers for\n> both. In particular, an external merge driver's output is read from a\n> file, but we can't easily use read_mmfile(), since we want the result in\n> an mmbuffer_t.\n> \n> Let's use mmfile_t for both cases and drop mmbuffer_t. The latter is\n> probably a more descriptive name, but we have many more uses of\n> mmfile_t (and helpers like read_mmfile). So let's consolidate using that\n> name; we can always change it to something more sensible later.\n\nYeah, that was my initial reaction, too. `mmbuffer_t` is indeed a better\nname as `mmfile_t` indicates that it's coming from... well, a file. And\nthat's not necessarily true.\n\nI do wonder whether we should just aim for gradual improvement and use\n`mmbuffer_t` regardless or even shoot for something altogether different\nlike `struct xdiff_buf` and then simply not mind the fact that we're\nbeing inconsistent. That would at least be an initial step into a better\ndirection in my opinion, and we can then touch up things over some time.\n\nBut I won't insist on any change like that, I'm okay with keeping\n`mmfile_t`.\n\nPatrick\n"},{"id":"553718","messageId":"ar0rp1cSIKuCMZyQ@pks.im","threadId":"66416","inReplyTo":"20260929065504.GE1697497@coredump.intra.peff.net","subject":"Re: [PATCH 5/5] xdiff: NUL-terminate buffers read by read_mmfile()","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-09-30T15:32:55Z","receivedAt":"2026-09-30T15:33:00Z","isPatch":true,"body":"On Tue, Sep 29, 2026 at 02:55:04AM -0400, Jeff King wrote:\n> Since an mmfile_t is a ptr/len pair, our read_mmfile() allocates exactly\n> the number of bytes we claim to store. But in many other places in Git,\n> we add an extra NUL \"just in case\", which can help avoid read overruns\n> due to off-by-ones or the use of string functions.\n> \n> I don't know of any path that would benefit from this, but I noticed it\n> while converting ll_ext_merge() to use read_mmfile(), since its original\n> code did add a NUL byte (even though I cannot find any case where it\n> would have mattered). Let's teach read_mmfile() to add this defensive\n> NUL; it probably doesn't help anything, but nor should it hurt.\n> \n> Note that the matching read_mmblob() doesn't need the same treatment.\n> Its buffers already have a NUL from the object-reading code (which uses\n> the same defensive trick).\n> \n> As a bonus, we can get rid of the hack in read_mmfile() to handle empty\n> files by allocating a single byte.\n> \n> Signed-off-by: Jeff King \n> ---\n> This one is obviously optional, which is why I put it last.\n\nHm, I'm somewhat indifferent here. It always feels a bit weird to be\nthis defensive because \"programming errors\", as the next question then\nis \"but what about all the other errors where we're not defensive?\" But\nthe xdiff code is complex enough with a bunch of pointer arithmetics, so\nmaybe it's not even that bad of an idea.\n\nThat being said, I feel like a better course of action could be to use a\nfuzzer for this code, because as far as I'm aware we have none yet, and\nthat would potentially shake out a bunch of bugs. But that still doesn't\nreally help us to catch platform-specific bugs due to different integer\nsizes.\n\nThe counterargument is that before your 3/5 we used to use xmallocz, so\nyou're essentially just reinstating the previous safety guards.\n\n> diff --git a/xdiff-interface.c b/xdiff-interface.c\n> index bc340d5a8a..b3e9f1952b 100644\n> --- a/xdiff-interface.c\n> +++ b/xdiff-interface.c\n> @@ -166,7 +166,7 @@ int read_mmfile(mmfile_t *ptr, const char *filename)\n> \tif (!(f = fopen(filename, \"rb\")))\n> \t\treturn error_errno(\"Could not open %s\", filename);\n> \tsz = xsize_t(st.st_size);\n> -\tptr->ptr = xmalloc(sz ? sz : 1);\n> +\tptr->ptr = xmallocz(sz);\n\nI was staring at this code a while before I noticed the added `z` at the\nend of this function.\n\nPatrick\n"},{"id":"553720","messageId":"ar0rrVE0ZxcU7uG-@pks.im","threadId":"66416","inReplyTo":"20260929065442.GD1697497@coredump.intra.peff.net","subject":"Re: [PATCH 4/5] merge-ll: use read_mmfile() to read external merge results","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-09-30T15:33:01Z","receivedAt":"2026-09-30T15:33:09Z","isPatch":true,"body":"On Tue, Sep 29, 2026 at 02:54:42AM -0400, Jeff King wrote:\n> diff --git a/merge-ll.c b/merge-ll.c\n> index dfed6411a8..7fab7c5438 100644\n> --- a/merge-ll.c\n> +++ b/merge-ll.c\n> @@ -241,20 +240,10 @@ static enum ll_merge_result ll_ext_merge(const struct ll_merge_driver *fn,\n> \tchild.use_shell = 1;\n> \tstrvec_push(&child.args, cmd.buf);\n> \tstatus = run_command(&child);\n> -\tfd = open(temp[1], O_RDONLY);\n> -\tif (fd < 0)\n> -\t\tgoto bad;\n> -\tif (fstat(fd, &st))\n> -\t\tgoto close_bad;\n> -\tresult->size = st.st_size;\n> -\tresult->ptr = xmallocz(result->size);\n\nSo we do lose the NUL-termination that `xmallocz()` gave us, as\n`read_mmfile()` doesn't do that. You reinstate that in the last patch\nthough, which makes me lean more into the direction of having that last\noptional patch. If so though, we may want to reorder it to come first.\n\n> -\tif (read_in_full(fd, result->ptr, result->size) != result->size) {\n> -\t\tFREE_AND_NULL(result->ptr);\n> -\t\tresult->size = 0;\n> -\t}\n> - close_bad:\n> -\tclose(fd);\n> - bad:\n> +\n> +\t/* We can ignore errors; result is left NULL/0 in that case. */\n> +\tread_mmfile(result, temp[1]);\n\nOne change in behaviour that wasn't called out is that this will now\nmake us write an error message in case we failed reading the file. That\ncould be a good change, but that's hard to say.\n\nPatrick\n"},{"id":"553733","messageId":"xmqq8q4ibouf.fsf@gitster.g","threadId":"66416","inReplyTo":"20260929214943.GA1735259@coredump.intra.peff.net","subject":"Re: [PATCH 7/5] merge-ll: report an error when reading external merge results fails","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-09-30T18:01:28Z","receivedAt":"2026-09-30T18:01:30Z","isPatch":true,"body":"Jeff King writes:\n\n> Here's a resend of that final patch (not just a squash, because the\n> commit message mentioned the chmod).\n\nMakes sense.\n\nThese 6/5 and 7/5 are probably better squashed into 5/5 than left as\n\"oops that was bad, so here is a preliminary clean-up to make the\nfix easier (6/5), and here is the fix of the fifth step (7/5)\", no?\n"},{"id":"553749","messageId":"xmqq5wzma4ts.fsf@gitster.g","threadId":"66416","inReplyTo":"ar0rp1cSIKuCMZyQ@pks.im","subject":"Re: [PATCH 5/5] xdiff: NUL-terminate buffers read by read_mmfile()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-09-30T19:59:11Z","receivedAt":"2026-09-30T19:59:13Z","isPatch":true,"body":"Patrick Steinhardt writes:\n\n> On Tue, Sep 29, 2026 at 02:55:04AM -0400, Jeff King wrote:\n>> Since an mmfile_t is a ptr/len pair, our read_mmfile() allocates exactly\n>> the number of bytes we claim to store. But in many other places in Git,\n>> we add an extra NUL \"just in case\", which can help avoid read overruns\n>> due to off-by-ones or the use of string functions.\n>> \n>> I don't know of any path that would benefit from this, but I noticed it\n>> while converting ll_ext_merge() to use read_mmfile(), since its original\n>> code did add a NUL byte (even though I cannot find any case where it\n>> would have mattered). Let's teach read_mmfile() to add this defensive\n>> NUL; it probably doesn't help anything, but nor should it hurt.\n>> \n>> Note that the matching read_mmblob() doesn't need the same treatment.\n>> Its buffers already have a NUL from the object-reading code (which uses\n>> the same defensive trick).\n>> \n>> As a bonus, we can get rid of the hack in read_mmfile() to handle empty\n>> files by allocating a single byte.\n>> \n>> Signed-off-by: Jeff King \n>> ---\n>> This one is obviously optional, which is why I put it last.\n>> ...\n>> -\tptr->ptr = xmalloc(sz ? sz : 1);\n>> +\tptr->ptr = xmallocz(sz);\n>\n> I was staring at this code a while before I noticed the added `z` at the\n> end of this function.\n\nI had the same reaction yesterday. The proposed log message never\nsaid how it added the extra NUL (or for that matter, it wasn't clear\nif it actually did the adding). The usual imperative \"Add the same\n'just in case' NUL by using xmallocz().\" would have helped a lot.\n"},{"id":"553773","messageId":"20260930224142.GB763270@coredump.intra.peff.net","threadId":"66416","inReplyTo":"xmqq8q4ibouf.fsf@gitster.g","subject":"Re: [PATCH 7/5] merge-ll: report an error when reading external merge results fails","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-09-30T22:41:42Z","receivedAt":"2026-09-30T22:41:44Z","isPatch":true,"body":"On Wed, Sep 30, 2026 at 11:01:28AM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > Here's a resend of that final patch (not just a squash, because the\n> > commit message mentioned the chmod).\n> \n> Makes sense.\n> \n> These 6/5 and 7/5 are probably better squashed into 5/5 than left as\n> \"oops that was bad, so here is a preliminary clean-up to make the\n> fix easier (6/5), and here is the fix of the fifth step (7/5)\", no?\n\nI don't think it is the fault of 5/5 at all (which carefully tried to\nmaintain the NULL behavior). The problem fixed by 7/5 existed before my\nseries.\n\nIn theory that fix _could_ come earlier in the series, but it's actually\nmuch easier to fix after 5/5, because we have a single spot to error\ncheck.\n\n-Peff\n"},{"id":"553774","messageId":"20260930224613.GA765052@coredump.intra.peff.net","threadId":"66416","inReplyTo":"ar0roZKCwALv0n_A@pks.im","subject":"Re: [PATCH 2/5] xdiff: replace mmbuffer_t with mmfile_t","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-09-30T22:46:13Z","receivedAt":"2026-09-30T22:46:15Z","isPatch":true,"body":"On Wed, Sep 30, 2026 at 05:32:49PM +0200, Patrick Steinhardt wrote:\n\n> > Let's use mmfile_t for both cases and drop mmbuffer_t. The latter is\n> > probably a more descriptive name, but we have many more uses of\n> > mmfile_t (and helpers like read_mmfile). So let's consolidate using that\n> > name; we can always change it to something more sensible later.\n> \n> Yeah, that was my initial reaction, too. `mmbuffer_t` is indeed a better\n> name as `mmfile_t` indicates that it's coming from... well, a file. And\n> that's not necessarily true.\n> \n> I do wonder whether we should just aim for gradual improvement and use\n> `mmbuffer_t` regardless or even shoot for something altogether different\n> like `struct xdiff_buf` and then simply not mind the fact that we're\n> being inconsistent. That would at least be an initial step into a better\n> direction in my opinion, and we can then touch up things over some time.\n> \n> But I won't insist on any change like that, I'm okay with keeping\n> `mmfile_t`.\n\nI'd really prefer to punt on it for now, just because the diff would be\n_so_ big, and has so many extra rabbit holes (e.g., should \"mmfile_t\n*mf\" get a new variable name?).\n\n-Peff\n"},{"id":"553775","messageId":"20260930224935.GB765052@coredump.intra.peff.net","threadId":"66416","inReplyTo":"ar0rp1cSIKuCMZyQ@pks.im","subject":"Re: [PATCH 5/5] xdiff: NUL-terminate buffers read by read_mmfile()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-09-30T22:49:35Z","receivedAt":"2026-09-30T22:49:37Z","isPatch":true,"body":"On Wed, Sep 30, 2026 at 05:32:55PM +0200, Patrick Steinhardt wrote:\n\n> > This one is obviously optional, which is why I put it last.\n> \n> Hm, I'm somewhat indifferent here. It always feels a bit weird to be\n> this defensive because \"programming errors\", as the next question then\n> is \"but what about all the other errors where we're not defensive?\" But\n> the xdiff code is complex enough with a bunch of pointer arithmetics, so\n> maybe it's not even that bad of an idea.\n> \n> That being said, I feel like a better course of action could be to use a\n> fuzzer for this code, because as far as I'm aware we have none yet, and\n> that would potentially shake out a bunch of bugs. But that still doesn't\n> really help us to catch platform-specific bugs due to different integer\n> sizes.\n\nI look at it as: why not do both?\n\nMostly the lack of extra NUL surprised me, as we routinely add one in\nmost other places (and it has prevented some memory bugs in the past).\n\n> The counterargument is that before your 3/5 we used to use xmallocz, so\n> you're essentially just reinstating the previous safety guards.\n\nYes, though I did confirm that those guards were doing nothing. This is\nless about protecting the new ll_ext_merge() caller and more about all\nof the _other_ callers of read_mmfile().\n\nBut yeah, it is obviously a lot easier to explain if this patch comes\nfirst. I just wasn't sure if we'd want to drop it or not (though yeah,\nwe probably should explain in the earlier patch that the lack of NUL\ntermination is OK).\n\nI'll re-roll with this patch earlier in the series.\n\n> > diff --git a/xdiff-interface.c b/xdiff-interface.c\n> > index bc340d5a8a..b3e9f1952b 100644\n> > --- a/xdiff-interface.c\n> > +++ b/xdiff-interface.c\n> > @@ -166,7 +166,7 @@ int read_mmfile(mmfile_t *ptr, const char *filename)\n> > \tif (!(f = fopen(filename, \"rb\")))\n> > \t\treturn error_errno(\"Could not open %s\", filename);\n> > \tsz = xsize_t(st.st_size);\n> > -\tptr->ptr = xmalloc(sz ? sz : 1);\n> > +\tptr->ptr = xmallocz(sz);\n> \n> I was staring at this code a while before I noticed the added `z` at the\n> end of this function.\n\nHeh, fair. I'll say something more explicit in the commit message when\nre-rolling.\n\n-Peff\n"},{"id":"553776","messageId":"20260930225011.GC765052@coredump.intra.peff.net","threadId":"66416","inReplyTo":"ar0rrVE0ZxcU7uG-@pks.im","subject":"Re: [PATCH 4/5] merge-ll: use read_mmfile() to read external merge results","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-09-30T22:50:11Z","receivedAt":"2026-09-30T22:50:13Z","isPatch":true,"body":"On Wed, Sep 30, 2026 at 05:33:01PM +0200, Patrick Steinhardt wrote:\n\n> So we do lose the NUL-termination that `xmallocz()` gave us, as\n> `read_mmfile()` doesn't do that. You reinstate that in the last patch\n> though, which makes me lean more into the direction of having that last\n> optional patch. If so though, we may want to reorder it to come first.\n\nYeah, I'll do that re-order.\n\n> > -\tif (read_in_full(fd, result->ptr, result->size) != result->size) {\n> > -\t\tFREE_AND_NULL(result->ptr);\n> > -\t\tresult->size = 0;\n> > -\t}\n> > - close_bad:\n> > -\tclose(fd);\n> > - bad:\n> > +\n> > +\t/* We can ignore errors; result is left NULL/0 in that case. */\n> > +\tread_mmfile(result, temp[1]);\n> \n> One change in behaviour that wasn't called out is that this will now\n> make us write an error message in case we failed reading the file. That\n> could be a good change, but that's hard to say.\n\nTrue, I hadn't even thought about that. It seems like a strict\nimprovement to me, but I'll mention it in the commit message.\n\n-Peff\n"},{"id":"553778","messageId":"20260930234348.GA1340390@coredump.intra.peff.net","threadId":"66416","inReplyTo":"20260929064935.GA1276867@coredump.intra.peff.net","subject":"[PATCH v2 0/7] use size_t for xdiff mmfile_t","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-09-30T23:43:48Z","receivedAt":"2026-09-30T23:43:50Z","isPatch":true,"body":"On Tue, Sep 29, 2026 at 02:49:36AM -0400, Jeff King wrote:\n\n> An earlier series tried to simplify ll_ext_merge()'s code to read back\n> the merge result from a temporary file, but Elijah pointed out some\n> subtle integer overflow confusion:\n> \n> https://lore.kernel.org/git/CABPp-BG9Hkc7i_JxAbYfyzu+b4Mc_pZUr0jJF=vY0jHSARpHzw@mail.gmail.com/\n> \n> I dug a little bit and found that similar problems exist elsewhere. So\n> here's an attempt to make things at least incrementally better. And\n> patch 4 is the original cleanup I set out to do. ;)\n\nHere's a v2 that addresses review so far. The end state is the same\n(plus the two bonus patches sent earlier), but it moves the xmallocz()\npatch earlier, and fills in a few bits in the commit messages.\n\nRange-diff is below.\n\n [1/7]: xdiff: clean up read_mmfile() allocations on error\n [2/7]: xdiff: replace mmbuffer_t with mmfile_t\n [3/7]: xdiff: use size_t for buffer sizes\n [4/7]: xdiff: NUL-terminate buffers read by read_mmfile()\n [5/7]: merge-ll: use read_mmfile() to read external merge results\n [6/7]: merge-ll: handle external driver status before reading result\n [7/7]: merge-ll: report an error when reading external merge results fails\n\n Documentation/technical/api-merge.adoc | 7 ++--\n apply.c | 2 +-\n builtin/checkout.c | 2 +-\n builtin/merge-file.c | 2 +-\n builtin/merge-tree.c | 2 +-\n builtin/rerere.c | 8 +++-\n diff.c | 2 +-\n merge-blobs.c | 2 +-\n merge-ll.c | 39 +++++++-------------\n merge-ll.h | 4 +-\n merge-ort.c | 4 +-\n notes-merge.c | 2 +-\n rerere.c | 11 +++---\n t/t4200-rerere.sh | 51 ++++++++++++++++++++++++++\n xdiff-interface.c | 5 ++-\n xdiff/xdiff.h | 11 ++----\n xdiff/xmerge.c | 4 +-\n xdiff/xutils.c | 4 +-\n 18 files changed, 100 insertions(+), 62 deletions(-)\n\n1: 985905950f = 1: 985905950f xdiff: clean up read_mmfile() allocations on error\n2: ddcae336eb = 2: ddcae336eb xdiff: replace mmbuffer_t with mmfile_t\n3: 36d932e0ee = 3: 36d932e0ee xdiff: use size_t for buffer sizes\n5: f25902e825 ! 4: c9cd3c7c3c xdiff: NUL-terminate buffers read by read_mmfile()\n @@ Commit message\n I don't know of any path that would benefit from this, but I noticed it\n while converting ll_ext_merge() to use read_mmfile(), since its original\n code did add a NUL byte (even though I cannot find any case where it\n - would have mattered). Let's teach read_mmfile() to add this defensive\n - NUL; it probably doesn't help anything, but nor should it hurt.\n + would have mattered). Let's add the same defensive NUL in read_mmfile()\n + by using xmallocz() instead of xmalloc().\n \n Note that the matching read_mmblob() doesn't need the same treatment.\n Its buffers already have a NUL from the object-reading code (which uses\n4: 6ac0d54bda ! 5: 86fa283e00 merge-ll: use read_mmfile() to read external merge results\n @@ Commit message\n back from a temporary file. We can do the same thing with much less code\n by using read_mmfile().\n \n - As a bonus, note that read_mmfile() correctly uses xsize_t() to detect\n - the case when we'd truncate the result.\n + There are also two behavior improvements.\n +\n + One, read_mmfile() correctly uses xsize_t() to detect the case when we'd\n + truncate the result.\n +\n + And two, read_mmfile() will report errors to stderr if it can't read the\n + file (whereas the existing code silently returned NULL). I think most\n + callers would have said _something_ in this case like \"failed to execute\n + merge\" (from merge-ort), but more specifics are probably helpful (e.g.,\n + to distinguish a random system error from a badly configured merge\n + driver).\n \n Signed-off-by: Jeff King \n \n6: 3e5f090284 = 6: 8ad0b774bf merge-ll: handle external driver status before reading result\n7: 30357e6e9a = 7: 896031317b merge-ll: report an error when reading external merge results fails\n"},{"id":"553779","messageId":"20260930234402.GA1347555@coredump.intra.peff.net","threadId":"66416","inReplyTo":"20260930234348.GA1340390@coredump.intra.peff.net","subject":"[PATCH v2 1/7] xdiff: clean up read_mmfile() allocations on error","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-09-30T23:44:02Z","receivedAt":"2026-09-30T23:44:04Z","isPatch":true,"body":"When read_mmfile() returns an error, it may or may not have allocated a\nbuffer in the passed-in mmfile_t. So callers must initialize the pointer\nto NULL and free it even on error.\n\nMost callers do this already, but rerere's diff_two() does not, and\nwould leak the buffer after a read error. We could fix it directly, but\nlet's instead try to make the interface less error-prone by freeing the\nmemory when returning failure from read_mmfile().\n\nThis fixes (part of) the leak in diff_two(). In theory it also lets us\nsimplify other callers to skip initializing the mmfile. But in practice\nmost still need zero-initialization because they may jump to free()\nbefore even calling read_mmfile (e.g., in try_merge()). But we can at\nleast simplify rerere_forget_one_path() a bit.\n\nI said \"part of\" earlier. There's a related leak in diff_two(): if\nreading the first file succeeds but reading the second fails, we return\nearly and leak the first buffer. We can fix that by checking each\nindividually.\n\nSigned-off-by: Jeff King \n---\n builtin/rerere.c | 6 +++++-\n rerere.c | 3 +--\n xdiff-interface.c | 1 +\n 3 files changed, 7 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/rerere.c b/builtin/rerere.c\nindex a056cb791b..d39c6e8445 100644\n--- a/builtin/rerere.c\n+++ b/builtin/rerere.c\n@@ -34,8 +34,12 @@ static int diff_two(const char *file1, const char *label1,\n \tmmfile_t minus, plus;\n \tint ret;\n \n-\tif (read_mmfile(&minus, file1) || read_mmfile(&plus, file2))\n+\tif (read_mmfile(&minus, file1))\n \t\treturn -1;\n+\tif (read_mmfile(&plus, file2)) {\n+\t\tfree(minus.ptr);\n+\t\treturn -1;\n+\t}\n \n \tprintf(\"--- a/%s\\n+++ b/%s\\n\", label1, label2);\n \tfflush(stdout);\ndiff --git a/rerere.c b/rerere.c\nindex 1c3745d9e3..856347c9ae 100644\n--- a/rerere.c\n+++ b/rerere.c\n@@ -1039,7 +1039,7 @@ static int rerere_forget_one_path(struct index_state *istate,\n \tfor (id->variant = 0;\n \t id->variant < id->collection->status_nr;\n \t id->variant++) {\n-\t\tmmfile_t cur = { NULL, 0 };\n+\t\tmmfile_t cur;\n \t\tmmbuffer_t result = {NULL, 0};\n \t\tint cleanly_resolved;\n \n@@ -1048,7 +1048,6 @@ static int rerere_forget_one_path(struct index_state *istate,\n \n \t\thandle_cache(istate, path, hash, rerere_path(&buf, id, \"thisimage\"));\n \t\tif (read_mmfile(&cur, rerere_path(&buf, id, \"thisimage\"))) {\n-\t\t\tfree(cur.ptr);\n \t\t\terror(_(\"failed to update conflicted state in '%s'\"), path);\n \t\t\tgoto fail_exit;\n \t\t}\ndiff --git a/xdiff-interface.c b/xdiff-interface.c\nindex db6938689f..e3dd2184ae 100644\n--- a/xdiff-interface.c\n+++ b/xdiff-interface.c\n@@ -168,6 +168,7 @@ int read_mmfile(mmfile_t *ptr, const char *filename)\n \tsz = xsize_t(st.st_size);\n \tptr->ptr = xmalloc(sz ? sz : 1);\n \tif (sz && fread(ptr->ptr, sz, 1, f) != 1) {\n+\t\tFREE_AND_NULL(ptr->ptr);\n \t\tfclose(f);\n \t\treturn error(\"Could not read %s\", filename);\n \t}\n-- \n2.56.0.354.gb6b32d5be5\n\n"},{"id":"553780","messageId":"20260930234406.GB1347555@coredump.intra.peff.net","threadId":"66416","inReplyTo":"20260930234348.GA1340390@coredump.intra.peff.net","subject":"[PATCH v2 2/7] xdiff: replace mmbuffer_t with mmfile_t","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-09-30T23:44:06Z","receivedAt":"2026-09-30T23:44:07Z","isPatch":true,"body":"Our import of xdiff has two identical buffer structures: mmfile_t and\nmmbuffer_t. In upstream xdiff these were actually different, but the\nimport in 3443546f6e (Use a *real* built-in diff generator, 2006-03-24)\nsimplified mmfile_t to a simple buffer.\n\nIn xdiff we usually use mmfile_t for input and mmbuffer_t for output,\nbut they are really both just a ptr/len pair. I don't think that having\ndifferent types is buying us anything in terms of type safety or\nsemantics, and having two makes it awkward to use the same helpers for\nboth. In particular, an external merge driver's output is read from a\nfile, but we can't easily use read_mmfile(), since we want the result in\nan mmbuffer_t.\n\nLet's use mmfile_t for both cases and drop mmbuffer_t. The latter is\nprobably a more descriptive name, but we have many more uses of\nmmfile_t (and helpers like read_mmfile). So let's consolidate using that\nname; we can always change it to something more sensible later.\n\nThere should be no behavior change here; this is just consolidating the\ntypes.\n\nSigned-off-by: Jeff King \n---\n Documentation/technical/api-merge.adoc | 7 +++----\n apply.c | 2 +-\n builtin/checkout.c | 2 +-\n builtin/merge-file.c | 2 +-\n builtin/merge-tree.c | 2 +-\n builtin/rerere.c | 2 +-\n merge-blobs.c | 2 +-\n merge-ll.c | 12 ++++++------\n merge-ll.h | 4 ++--\n merge-ort.c | 4 ++--\n notes-merge.c | 2 +-\n rerere.c | 8 ++++----\n xdiff-interface.c | 2 +-\n xdiff/xdiff.h | 9 ++-------\n xdiff/xmerge.c | 4 ++--\n xdiff/xutils.c | 4 ++--\n 16 files changed, 31 insertions(+), 37 deletions(-)\n\ndiff --git a/Documentation/technical/api-merge.adoc b/Documentation/technical/api-merge.adoc\nindex c2ba01828c..b691599393 100644\n--- a/Documentation/technical/api-merge.adoc\n+++ b/Documentation/technical/api-merge.adoc\n@@ -20,11 +20,10 @@ responsible for a few things.\n Data structures\n ---------------\n \n-* `mmbuffer_t`, `mmfile_t`\n+* `mmfile_t`\n \n-These store data usable for use by the xdiff backend, for writing and\n-for reading, respectively. See `xdiff/xdiff.h` for the definitions\n-and `diff.c` for examples.\n+This stores a buffer and its size for input to or output from the xdiff\n+backend. See `xdiff/xdiff.h` for the definition and `diff.c` for examples.\n \n * `struct ll_merge_options`\n \ndiff --git a/apply.c b/apply.c\nindex f00b7ba4d3..faf3c1dba0 100644\n--- a/apply.c\n+++ b/apply.c\n@@ -3646,7 +3646,7 @@ static int three_way_merge(struct apply_state *state,\n {\n \tmmfile_t base_file, our_file, their_file;\n \tstruct ll_merge_options merge_opts = LL_MERGE_OPTIONS_INIT;\n-\tmmbuffer_t result = { NULL };\n+\tmmfile_t result = { NULL };\n \tenum ll_merge_result status;\n \n \t/* resolve trivial cases first */\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex c0f0d2c700..2d575a4f57 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -320,7 +320,7 @@ static int checkout_merged(int pos, const struct checkout *state,\n \tenum ll_merge_result merge_status;\n \tint status;\n \tstruct object_id oid;\n-\tmmbuffer_t result_buf;\n+\tmmfile_t result_buf;\n \tstruct object_id threeway[3];\n \tunsigned mode = 0;\n \tstruct ll_merge_options ll_opts = LL_MERGE_OPTIONS_INIT;\ndiff --git a/builtin/merge-file.c b/builtin/merge-file.c\nindex 8fa5765239..ddca408c46 100644\n--- a/builtin/merge-file.c\n+++ b/builtin/merge-file.c\n@@ -64,7 +64,7 @@ int cmd_merge_file(int argc,\n {\n \tconst char *names[3] = { 0 };\n \tmmfile_t mmfs[3] = { 0 };\n-\tmmbuffer_t result = { 0 };\n+\tmmfile_t result = { 0 };\n \txmparam_t xmp = { 0 };\n \tint ret = 0, i = 0, to_stdout = 0, object_id = 0;\n \tint quiet = 0;\ndiff --git a/builtin/merge-tree.c b/builtin/merge-tree.c\nindex 49f41e520f..552c2ad736 100644\n--- a/builtin/merge-tree.c\n+++ b/builtin/merge-tree.c\n@@ -109,7 +109,7 @@ static void *origin(struct merge_list *entry, size_t *size)\n \treturn NULL;\n }\n \n-static int show_outf(void *priv UNUSED, mmbuffer_t *mb, int nbuf)\n+static int show_outf(void *priv UNUSED, mmfile_t *mb, int nbuf)\n {\n \tint i;\n \tfor (i = 0; i < nbuf; i++)\ndiff --git a/builtin/rerere.c b/builtin/rerere.c\nindex d39c6e8445..ef03b79f5b 100644\n--- a/builtin/rerere.c\n+++ b/builtin/rerere.c\n@@ -16,7 +16,7 @@ static const char * const rerere_usage[] = {\n \tNULL,\n };\n \n-static int outf(void *dummy UNUSED, mmbuffer_t *ptr, int nbuf)\n+static int outf(void *dummy UNUSED, mmfile_t *ptr, int nbuf)\n {\n \tint i;\n \tfor (i = 0; i < nbuf; i++)\ndiff --git a/merge-blobs.c b/merge-blobs.c\nindex 16a75bd1e3..49dbec9529 100644\n--- a/merge-blobs.c\n+++ b/merge-blobs.c\n@@ -38,7 +38,7 @@ static void *three_way_filemerge(struct index_state *istate,\n \t\t\t\t size_t *size)\n {\n \tenum ll_merge_result merge_status;\n-\tmmbuffer_t res;\n+\tmmfile_t res;\n \n \t/*\n \t * This function is only used by cmd_merge_tree, which\ndiff --git a/merge-ll.c b/merge-ll.c\nindex ef5287dee8..dfed6411a8 100644\n--- a/merge-ll.c\n+++ b/merge-ll.c\n@@ -21,7 +21,7 @@\n struct ll_merge_driver;\n \n typedef enum ll_merge_result (*ll_merge_fn)(const struct ll_merge_driver *,\n-\t\t\t mmbuffer_t *result,\n+\t\t\t mmfile_t *result,\n \t\t\t const char *path,\n \t\t\t mmfile_t *orig, const char *orig_name,\n \t\t\t mmfile_t *src1, const char *name1,\n@@ -56,7 +56,7 @@ void reset_merge_attributes(void)\n * Built-in low-levels\n */\n static enum ll_merge_result ll_binary_merge(const struct ll_merge_driver *drv UNUSED,\n-\t\t\t mmbuffer_t *result,\n+\t\t\t mmfile_t *result,\n \t\t\t const char *path UNUSED,\n \t\t\t mmfile_t *orig, const char *orig_name UNUSED,\n \t\t\t mmfile_t *src1, const char *name1 UNUSED,\n@@ -101,7 +101,7 @@ static enum ll_merge_result ll_binary_merge(const struct ll_merge_driver *drv UN\n }\n \n static enum ll_merge_result ll_xdl_merge(const struct ll_merge_driver *drv_unused,\n-\t\t\tmmbuffer_t *result,\n+\t\t\tmmfile_t *result,\n \t\t\tconst char *path,\n \t\t\tmmfile_t *orig, const char *orig_name,\n \t\t\tmmfile_t *src1, const char *name1,\n@@ -147,7 +147,7 @@ static enum ll_merge_result ll_xdl_merge(const struct ll_merge_driver *drv_unuse\n }\n \n static enum ll_merge_result ll_union_merge(const struct ll_merge_driver *drv_unused,\n-\t\t\t mmbuffer_t *result,\n+\t\t\t mmfile_t *result,\n \t\t\t const char *path,\n \t\t\t mmfile_t *orig, const char *orig_name,\n \t\t\t mmfile_t *src1, const char *name1,\n@@ -189,7 +189,7 @@ static void create_temp(mmfile_t *src, char *path, size_t len)\n * User defined low-level merge driver support.\n */\n static enum ll_merge_result ll_ext_merge(const struct ll_merge_driver *fn,\n-\t\t\tmmbuffer_t *result,\n+\t\t\tmmfile_t *result,\n \t\t\tconst char *path,\n \t\t\tmmfile_t *orig, const char *orig_name,\n \t\t\tmmfile_t *src1, const char *name1,\n@@ -403,7 +403,7 @@ static void normalize_file(mmfile_t *mm, const char *path, struct index_state *i\n \t}\n }\n \n-enum ll_merge_result ll_merge(mmbuffer_t *result_buf,\n+enum ll_merge_result ll_merge(mmfile_t *result_buf,\n \t const char *path,\n \t mmfile_t *ancestor, const char *ancestor_label,\n \t mmfile_t *ours, const char *our_label,\ndiff --git a/merge-ll.h b/merge-ll.h\nindex f26aef238d..f95332c682 100644\n--- a/merge-ll.h\n+++ b/merge-ll.h\n@@ -16,7 +16,7 @@\n * If you have no special requests, skip this and pass `NULL`\n * as the `opts` parameter to use the default options.\n *\n- * - Allocate an mmbuffer_t variable for the result.\n+ * - Allocate an mmfile_t variable for the result.\n *\n * - Allocate and fill variables with the file's original content\n * and two modified versions (using `read_mmfile`, for example).\n@@ -100,7 +100,7 @@ enum ll_merge_result {\n * `.gitattributes` or `.git/info/attributes` into account.\n * Returns 0 for a clean merge.\n */\n-enum ll_merge_result ll_merge(mmbuffer_t *result_buf,\n+enum ll_merge_result ll_merge(mmfile_t *result_buf,\n \t const char *path,\n \t mmfile_t *ancestor, const char *ancestor_label,\n \t mmfile_t *ours, const char *our_label,\ndiff --git a/merge-ort.c b/merge-ort.c\nindex c410a5d353..1d3d193d35 100644\n--- a/merge-ort.c\n+++ b/merge-ort.c\n@@ -2111,7 +2111,7 @@ static int merge_3way(struct merge_options *opt,\n \t\t const struct object_id *b,\n \t\t const char *pathnames[3],\n \t\t const int extra_marker_size,\n-\t\t mmbuffer_t *result_buf)\n+\t\t mmfile_t *result_buf)\n {\n \tmmfile_t orig, src1, src2;\n \tstruct ll_merge_options ll_opts = LL_MERGE_OPTIONS_INIT;\n@@ -2247,7 +2247,7 @@ static int handle_content_merge(struct merge_options *opt,\n \n \t/* Remaining rules depend on file vs. submodule vs. symlink. */\n \telse if (S_ISREG(a->mode)) {\n-\t\tmmbuffer_t result_buf;\n+\t\tmmfile_t result_buf;\n \t\tint ret = 0, merge_status;\n \t\tint two_way;\n \ndiff --git a/notes-merge.c b/notes-merge.c\nindex 118cad2518..d361e70a48 100644\n--- a/notes-merge.c\n+++ b/notes-merge.c\n@@ -355,7 +355,7 @@ static void write_note_to_worktree(const struct object_id *obj,\n static int ll_merge_in_worktree(struct notes_merge_options *o,\n \t\t\t\tstruct notes_merge_pair *p)\n {\n-\tmmbuffer_t result_buf;\n+\tmmfile_t result_buf;\n \tmmfile_t base, local, remote;\n \tenum ll_merge_result status;\n \ndiff --git a/rerere.c b/rerere.c\nindex 856347c9ae..8696f8e7b7 100644\n--- a/rerere.c\n+++ b/rerere.c\n@@ -597,7 +597,7 @@ int rerere_remaining(struct repository *r, struct string_list *merge_rr)\n */\n static int try_merge(struct index_state *istate,\n \t\t const struct rerere_id *id, const char *path,\n-\t\t mmfile_t *cur, mmbuffer_t *result)\n+\t\t mmfile_t *cur, mmfile_t *result)\n {\n \tenum ll_merge_result ret;\n \tmmfile_t base = {NULL, 0}, other = {NULL, 0};\n@@ -638,7 +638,7 @@ static int merge(struct index_state *istate, const struct rerere_id *id, const c\n \tint ret;\n \tstruct strbuf buf = STRBUF_INIT;\n \tmmfile_t cur = {NULL, 0};\n-\tmmbuffer_t result = {NULL, 0};\n+\tmmfile_t result = {NULL, 0};\n \n \t/*\n \t * Normalize the conflicts in path and write it out to\n@@ -947,7 +947,7 @@ static int handle_cache(struct index_state *istate,\n \t\t\tconst char *path, unsigned char *hash, const char *output)\n {\n \tmmfile_t mmfile[3] = {{NULL}};\n-\tmmbuffer_t result = {NULL, 0};\n+\tmmfile_t result = {NULL, 0};\n \tconst struct cache_entry *ce;\n \tint pos, len, i, has_conflicts;\n \tstruct rerere_io_mem io;\n@@ -1040,7 +1040,7 @@ static int rerere_forget_one_path(struct index_state *istate,\n \t id->variant < id->collection->status_nr;\n \t id->variant++) {\n \t\tmmfile_t cur;\n-\t\tmmbuffer_t result = {NULL, 0};\n+\t\tmmfile_t result = {NULL, 0};\n \t\tint cleanly_resolved;\n \n \t\tif (!has_rerere_resolution(id))\ndiff --git a/xdiff-interface.c b/xdiff-interface.c\nindex e3dd2184ae..bc340d5a8a 100644\n--- a/xdiff-interface.c\n+++ b/xdiff-interface.c\n@@ -53,7 +53,7 @@ static int consume_one(void *priv_, char *s, unsigned long size)\n \treturn 0;\n }\n \n-static int xdiff_outf(void *priv_, mmbuffer_t *mb, int nbuf)\n+static int xdiff_outf(void *priv_, mmfile_t *mb, int nbuf)\n {\n \tstruct xdiff_emit_state *priv = priv_;\n \tint i;\ndiff --git a/xdiff/xdiff.h b/xdiff/xdiff.h\nindex dc370712e9..334eb436f6 100644\n--- a/xdiff/xdiff.h\n+++ b/xdiff/xdiff.h\n@@ -73,11 +73,6 @@ typedef struct s_mmfile {\n \tlong size;\n } mmfile_t;\n \n-typedef struct s_mmbuffer {\n-\tchar *ptr;\n-\tlong size;\n-} mmbuffer_t;\n-\n typedef struct s_xpparam {\n \tunsigned long flags;\n \n@@ -96,7 +91,7 @@ typedef struct s_xdemitcb {\n \t\t\tlong old_begin, long old_nr,\n \t\t\tlong new_begin, long new_nr,\n \t\t\tconst char *func, long funclen);\n-\tint (*out_line)(void *, mmbuffer_t *, int);\n+\tint (*out_line)(void *, mmfile_t *, int);\n } xdemitcb_t;\n \n typedef long (*find_func_t)(const char *line, long line_len, char *buffer, long buffer_size, void *priv);\n@@ -144,7 +139,7 @@ typedef struct s_xmparam {\n #define DEFAULT_CONFLICT_MARKER_SIZE 7\n \n int xdl_merge(mmfile_t *orig, mmfile_t *mf1, mmfile_t *mf2,\n-\t\txmparam_t const *xmp, mmbuffer_t *result);\n+\t\txmparam_t const *xmp, mmfile_t *result);\n \n #ifdef __cplusplus\n }\ndiff --git a/xdiff/xmerge.c b/xdiff/xmerge.c\nindex 659ad4ec97..7b37968d25 100644\n--- a/xdiff/xmerge.c\n+++ b/xdiff/xmerge.c\n@@ -504,7 +504,7 @@ static int xdl_simplify_non_conflicts(xdfenv_t *xe1, xdmerge_t *m,\n */\n static int xdl_do_merge(xdfenv_t *xe1, xdchange_t *xscr1,\n \t\txdfenv_t *xe2, xdchange_t *xscr2,\n-\t\txmparam_t const *xmp, mmbuffer_t *result)\n+\t\txmparam_t const *xmp, mmfile_t *result)\n {\n \txdmerge_t *changes, *c;\n \txpparam_t const *xpp = &xmp->xpp;\n@@ -682,7 +682,7 @@ static int xdl_do_merge(xdfenv_t *xe1, xdchange_t *xscr1,\n }\n \n int xdl_merge(mmfile_t *orig, mmfile_t *mf1, mmfile_t *mf2,\n-\t\txmparam_t const *xmp, mmbuffer_t *result)\n+\t\txmparam_t const *xmp, mmfile_t *result)\n {\n \txdchange_t *xscr1 = NULL, *xscr2 = NULL;\n \txdfenv_t xe1, xe2;\ndiff --git a/xdiff/xutils.c b/xdiff/xutils.c\nindex 9a999acdc0..4215f646c5 100644\n--- a/xdiff/xutils.c\n+++ b/xdiff/xutils.c\n@@ -39,7 +39,7 @@ uint64_t xdl_bogosqrt(uint64_t n) {\n int xdl_emit_diffrec(char const *rec, long size, char const *pre, long psize,\n \t\t xdemitcb_t *ecb) {\n \tint i = 2;\n-\tmmbuffer_t mb[3];\n+\tmmfile_t mb[3];\n \n \tmb[0].ptr = (char *) pre;\n \tmb[0].size = psize;\n@@ -392,7 +392,7 @@ static int xdl_format_hunk_hdr(long s1, long c1, long s2, long c2,\n \t\t\t const char *func, long funclen,\n \t\t\t xdemitcb_t *ecb) {\n \tint nb = 0;\n-\tmmbuffer_t mb;\n+\tmmfile_t mb;\n \tchar buf[128];\n \n \tmemcpy(buf, \"@@ -\", 4);\n-- \n2.56.0.354.gb6b32d5be5\n\n"},{"id":"553781","messageId":"20260930234410.GC1347555@coredump.intra.peff.net","threadId":"66416","inReplyTo":"20260930234348.GA1340390@coredump.intra.peff.net","subject":"[PATCH v2 3/7] xdiff: use size_t for buffer sizes","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-09-30T23:44:10Z","receivedAt":"2026-09-30T23:44:11Z","isPatch":true,"body":"An mmfile_t stores its size as a signed long, but the more natural type\nfor a buffer size is size_t. This not only limits the size of entry we\ncan hold, but also creates some possible integer overflow issues.\n\nFor example, read_mmfile() checks that the file size fits in a size_t\nbefore allocating, but then assigns it to a long. Likewise,\nread_mmblob() and fill_mmfile() copy sizes from other types without\nchecking that they fit.\n\nOn LP64 systems like Linux, this is mostly academic. You could wrap to a\nnegative long value, but you'd need an object that's 2^63 bytes, which\nis impractical.\n\nBut on an LLP64 system like Windows, a 2^31+1-byte blob could perhaps\ncause mischief. We do prevent large values from entering the xdiff code\ndue to MAX_XDIFF_SIZE (which is itself marked as unsigned, so we'd\nconvert any negative \"long\" back to a large unsigned value). But if you\nask for binary diffs, that negative long value could instead be\nconverted to a huge 64-bit size_t when passed to memcmp(), diff_delta(),\netc. So probably there are paths that can cause an out-of-bounds read,\ngiven the right set of options, but I didn't really dig for them.\n\nOn a 32-bit system things are less clear. Because \"long\" and \"size_t\"\nhave the same width, any time we implicitly convert to size_t, we should\nget back the original size (even if the intermediate \"long\" is itself\nnegative). Probably iterating using a long could be a problem, but most\nof that happens inside xdiff, which is protected by MAX_XDIFF_SIZE\n(which, again, compares in the unsigned space).\n\nLet's just use the obvious size_t type for counting the bytes. I suspect\nyou could still find truncation problems on LLP64 systems due to the use\nof \"unsigned long\" throughout the code, but that's a larger problem.\nThis should at least nudge us in the right direction.\n\nNote that we have to update the printf format in emit_binary_diff_body()\nto accommodate the new type. Curiously it was using \"%lu\", even though\nthe type was signed (I guess compiler printf-linting is happy enough if\njust the width of the format and the type match).\n\nSigned-off-by: Jeff King \n---\n diff.c | 2 +-\n xdiff/xdiff.h | 2 +-\n 2 files changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 414532d09f..b4ac17f8ef 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -3646,7 +3646,7 @@ static void emit_binary_diff_body(struct diff_options *o,\n \t\tdata = delta;\n \t\tdata_size = delta_size;\n \t} else {\n-\t\tchar *s = xstrfmt(\"%lu\", two->size);\n+\t\tchar *s = xstrfmt(\"%\"PRIuMAX, (uintmax_t)two->size);\n \t\temit_diff_symbol(o, DIFF_SYMBOL_BINARY_DIFF_HEADER_LITERAL,\n \t\t\t\t s, strlen(s), 0);\n \t\tfree(s);\ndiff --git a/xdiff/xdiff.h b/xdiff/xdiff.h\nindex 334eb436f6..8fa513fc4e 100644\n--- a/xdiff/xdiff.h\n+++ b/xdiff/xdiff.h\n@@ -70,7 +70,7 @@ extern \"C\" {\n \n typedef struct s_mmfile {\n \tchar *ptr;\n-\tlong size;\n+\tsize_t size;\n } mmfile_t;\n \n typedef struct s_xpparam {\n-- \n2.56.0.354.gb6b32d5be5\n\n"},{"id":"553782","messageId":"20260930234413.GD1347555@coredump.intra.peff.net","threadId":"66416","inReplyTo":"20260930234348.GA1340390@coredump.intra.peff.net","subject":"[PATCH v2 4/7] xdiff: NUL-terminate buffers read by read_mmfile()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-09-30T23:44:13Z","receivedAt":"2026-09-30T23:44:14Z","isPatch":true,"body":"Since an mmfile_t is a ptr/len pair, our read_mmfile() allocates exactly\nthe number of bytes we claim to store. But in many other places in Git,\nwe add an extra NUL \"just in case\", which can help avoid read overruns\ndue to off-by-ones or the use of string functions.\n\nI don't know of any path that would benefit from this, but I noticed it\nwhile converting ll_ext_merge() to use read_mmfile(), since its original\ncode did add a NUL byte (even though I cannot find any case where it\nwould have mattered). Let's add the same defensive NUL in read_mmfile()\nby using xmallocz() instead of xmalloc().\n\nNote that the matching read_mmblob() doesn't need the same treatment.\nIts buffers already have a NUL from the object-reading code (which uses\nthe same defensive trick).\n\nAs a bonus, we can get rid of the hack in read_mmfile() to handle empty\nfiles by allocating a single byte.\n\nSigned-off-by: Jeff King \n---\n xdiff-interface.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/xdiff-interface.c b/xdiff-interface.c\nindex bc340d5a8a..b3e9f1952b 100644\n--- a/xdiff-interface.c\n+++ b/xdiff-interface.c\n@@ -166,7 +166,7 @@ int read_mmfile(mmfile_t *ptr, const char *filename)\n \tif (!(f = fopen(filename, \"rb\")))\n \t\treturn error_errno(\"Could not open %s\", filename);\n \tsz = xsize_t(st.st_size);\n-\tptr->ptr = xmalloc(sz ? sz : 1);\n+\tptr->ptr = xmallocz(sz);\n \tif (sz && fread(ptr->ptr, sz, 1, f) != 1) {\n \t\tFREE_AND_NULL(ptr->ptr);\n \t\tfclose(f);\n-- \n2.56.0.354.gb6b32d5be5\n\n"},{"id":"553783","messageId":"20260930234416.GE1347555@coredump.intra.peff.net","threadId":"66416","inReplyTo":"20260930234348.GA1340390@coredump.intra.peff.net","subject":"[PATCH v2 5/7] merge-ll: use read_mmfile() to read external merge results","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-09-30T23:44:16Z","receivedAt":"2026-09-30T23:44:17Z","isPatch":true,"body":"After running an external merge driver, ll_ext_merge() reads the result\nback from a temporary file. We can do the same thing with much less code\nby using read_mmfile().\n\nThere are also two behavior improvements.\n\nOne, read_mmfile() correctly uses xsize_t() to detect the case when we'd\ntruncate the result.\n\nAnd two, read_mmfile() will report errors to stderr if it can't read the\nfile (whereas the existing code silently returned NULL). I think most\ncallers would have said _something_ in this case like \"failed to execute\nmerge\" (from merge-ort), but more specifics are probably helpful (e.g.,\nto distinguish a random system error from a badly configured merge\ndriver).\n\nSigned-off-by: Jeff King \n---\n merge-ll.c | 21 +++++----------------\n 1 file changed, 5 insertions(+), 16 deletions(-)\n\ndiff --git a/merge-ll.c b/merge-ll.c\nindex dfed6411a8..7fab7c5438 100644\n--- a/merge-ll.c\n+++ b/merge-ll.c\n@@ -201,8 +201,7 @@ static enum ll_merge_result ll_ext_merge(const struct ll_merge_driver *fn,\n \tstruct strbuf cmd = STRBUF_INIT;\n \tconst char *format = fn->cmdline;\n \tstruct child_process child = CHILD_PROCESS_INIT;\n-\tint status, fd, i;\n-\tstruct stat st;\n+\tint status, i;\n \tenum ll_merge_result ret;\n \tassert(opts);\n \n@@ -241,20 +240,10 @@ static enum ll_merge_result ll_ext_merge(const struct ll_merge_driver *fn,\n \tchild.use_shell = 1;\n \tstrvec_push(&child.args, cmd.buf);\n \tstatus = run_command(&child);\n-\tfd = open(temp[1], O_RDONLY);\n-\tif (fd < 0)\n-\t\tgoto bad;\n-\tif (fstat(fd, &st))\n-\t\tgoto close_bad;\n-\tresult->size = st.st_size;\n-\tresult->ptr = xmallocz(result->size);\n-\tif (read_in_full(fd, result->ptr, result->size) != result->size) {\n-\t\tFREE_AND_NULL(result->ptr);\n-\t\tresult->size = 0;\n-\t}\n- close_bad:\n-\tclose(fd);\n- bad:\n+\n+\t/* We can ignore errors; result is left NULL/0 in that case. */\n+\tread_mmfile(result, temp[1]);\n+\n \tfor (i = 0; i < 3; i++)\n \t\tunlink_or_warn(temp[i]);\n \tstrbuf_release(&cmd);\n-- \n2.56.0.354.gb6b32d5be5\n\n"},{"id":"553784","messageId":"20260930234418.GF1347555@coredump.intra.peff.net","threadId":"66416","inReplyTo":"20260930234348.GA1340390@coredump.intra.peff.net","subject":"[PATCH v2 6/7] merge-ll: handle external driver status before reading result","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-09-30T23:44:18Z","receivedAt":"2026-09-30T23:44:20Z","isPatch":true,"body":"After running an external merge driver, ll_ext_merge() reads its output\nand cleans up the temporary files before converting the exit status to\nan ll_merge_result.\n\nMove that conversion immediately after run_command(). This will let us\noverride the result if reading the output fails, without having to fake\nan exit status. No behavior change yet.\n\nIt is tempting to only call read_mmfile() when we have LL_MERGE_OK, but\ncallers do care about the result even with LL_MERGE_CONFLICT (e.g., the\noutput may contain a partial). I think we could safely skip it for\nLL_MERGE_ERROR, but that's a rare case and not worth complicating the\ncode for.\n\nSigned-off-by: Jeff King \n---\n merge-ll.c | 14 +++++++-------\n 1 file changed, 7 insertions(+), 7 deletions(-)\n\ndiff --git a/merge-ll.c b/merge-ll.c\nindex 7fab7c5438..4d82836bc5 100644\n--- a/merge-ll.c\n+++ b/merge-ll.c\n@@ -240,20 +240,20 @@ static enum ll_merge_result ll_ext_merge(const struct ll_merge_driver *fn,\n \tchild.use_shell = 1;\n \tstrvec_push(&child.args, cmd.buf);\n \tstatus = run_command(&child);\n-\n-\t/* We can ignore errors; result is left NULL/0 in that case. */\n-\tread_mmfile(result, temp[1]);\n-\n-\tfor (i = 0; i < 3; i++)\n-\t\tunlink_or_warn(temp[i]);\n-\tstrbuf_release(&cmd);\n \tif (!status)\n \t\tret = LL_MERGE_OK;\n \telse if (status <= 128)\n \t\tret = LL_MERGE_CONFLICT;\n \telse\n \t\t/* died due to a signal: WTERMSIG(status) + 128 */\n \t\tret = LL_MERGE_ERROR;\n+\n+\t/* We can ignore errors; result is left NULL/0 in that case. */\n+\tread_mmfile(result, temp[1]);\n+\n+\tfor (i = 0; i < 3; i++)\n+\t\tunlink_or_warn(temp[i]);\n+\tstrbuf_release(&cmd);\n \treturn ret;\n }\n \n-- \n2.56.0.354.gb6b32d5be5\n\n"},{"id":"553785","messageId":"20260930234420.GG1347555@coredump.intra.peff.net","threadId":"66416","inReplyTo":"20260930234348.GA1340390@coredump.intra.peff.net","subject":"[PATCH v2 7/7] merge-ll: report an error when reading external merge results fails","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-09-30T23:44:20Z","receivedAt":"2026-09-30T23:44:22Z","isPatch":true,"body":"If we can't read an external merge driver's output, ll_ext_merge()\nleaves the result buffer as NULL but returns a status based only on the\ndriver's exit code. So a driver which exits successfully can cause us to\nreturn LL_MERGE_OK without a result.\n\nMost callers of ll_merge() check for a NULL buffer in addition to an\nerror return, so they're fine. But rerere's merge() checks only the\nreturn value, and may write out the (incorrect) empty result as the\nrecorded resolution.\n\nLet's return LL_MERGE_ERROR when read_mmfile() fails, regardless of the\ndriver's exit status, to make it clear that the returned value is not\nvalid.\n\nOur test is a little funny; the bad case happens when reading back the\nfile happens to fail. That can happen due to system errors, but of\ncourse we want it to be deterministic. We can make that happen by\nremoving the result file. But if we configure a driver that always does\nthat, we'd never record a rerere result in the first place! So we\ninstead create a driver that \"breaks\" the read only when we instruct it\nto do so.\n\nSigned-off-by: Jeff King \n---\n merge-ll.c | 4 ++--\n t/t4200-rerere.sh | 51 +++++++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 53 insertions(+), 2 deletions(-)\n\ndiff --git a/merge-ll.c b/merge-ll.c\nindex 4d82836bc5..3b5327e7df 100644\n--- a/merge-ll.c\n+++ b/merge-ll.c\n@@ -248,8 +248,8 @@ static enum ll_merge_result ll_ext_merge(const struct ll_merge_driver *fn,\n \t\t/* died due to a signal: WTERMSIG(status) + 128 */\n \t\tret = LL_MERGE_ERROR;\n \n-\t/* We can ignore errors; result is left NULL/0 in that case. */\n-\tread_mmfile(result, temp[1]);\n+\tif (read_mmfile(result, temp[1]) < 0)\n+\t\tret = LL_MERGE_ERROR;\n \n \tfor (i = 0; i < 3; i++)\n \t\tunlink_or_warn(temp[i]);\ndiff --git a/t/t4200-rerere.sh b/t/t4200-rerere.sh\nindex 7bb601e117..5be3f056f5 100755\n--- a/t/t4200-rerere.sh\n+++ b/t/t4200-rerere.sh\n@@ -734,4 +734,55 @@ test_expect_success 'rerere does not crash with unmatched conflict marker' '\n \ttest_must_fail git rebase --continue\n '\n \n+test_expect_success 'rerere preserves conflicts when driver output is unreadable' '\n+\ttest_create_repo unreadable-output &&\n+\t(\n+\t\tcd unreadable-output &&\n+\t\tgit config rerere.enabled true &&\n+\t\tgit config rerere.autoupdate true &&\n+\t\twrite_script merge-driver <<-\\EOF &&\n+\t\tgit merge-file \"$@\"\n+\t\tstatus=$?\n+\t\tif test -f fail-read\n+\t\tthen\n+\t\t\trm \"$1\" || exit 1\n+\t\tfi\n+\t\texit \"$status\"\n+\t\tEOF\n+\t\tgit config merge.unreadable.driver \"./merge-driver %A %O %B\" &&\n+\t\techo \"file merge=unreadable\" >.gitattributes &&\n+\t\ttest_commit base file base &&\n+\t\tgit checkout -b one &&\n+\t\ttest_commit --no-tag one file one &&\n+\t\tgit checkout -b two base &&\n+\t\ttest_commit --no-tag two file two &&\n+\n+\t\t# Teach rerere a resolution while the driver works normally.\n+\t\ttest_must_fail git merge one &&\n+\t\techo resolved >file &&\n+\t\tgit rerere &&\n+\t\tgit merge --abort &&\n+\n+\t\t# Recreate the conflict without replaying the resolution yet.\n+\t\ttest_must_fail git -c rerere.enabled=false merge one &&\n+\n+\t\t# We will expect the same conflicted content after rerere fails\n+\t\t# below.\n+\t\tcp file expect &&\n+\t\tgit ls-files -u >expect-index &&\n+\t\ttest_file_not_empty expect-index &&\n+\n+\t\t# Now we try rerere again, but the merge driver will cause the\n+\t\t# read to fail.\n+\t\t>fail-read &&\n+\t\tgit rerere 2>err &&\n+\t\ttest_grep \"Could not stat\" err &&\n+\n+\t\t# And we expect the conflicted state.\n+\t\ttest_cmp expect file &&\n+\t\tgit ls-files -u >actual-index &&\n+\t\ttest_cmp expect-index actual-index\n+\t)\n+'\n+\n test_done\n-- \n2.56.0.354.gb6b32d5be5\n"},{"id":"553834","messageId":"ar5dB4p6pQITEUq6@pks.im","threadId":"66416","inReplyTo":"20260930234413.GD1347555@coredump.intra.peff.net","subject":"Re: [PATCH v2 4/7] xdiff: NUL-terminate buffers read by read_mmfile()","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-10-01T13:15:51Z","receivedAt":"2026-10-01T13:15:58Z","isPatch":true,"body":"On Wed, Sep 30, 2026 at 07:44:13PM -0400, Jeff King wrote:\n> Since an mmfile_t is a ptr/len pair, our read_mmfile() allocates exactly\n> the number of bytes we claim to store. But in many other places in Git,\n> we add an extra NUL \"just in case\", which can help avoid read overruns\n> due to off-by-ones or the use of string functions.\n> \n> I don't know of any path that would benefit from this, but I noticed it\n> while converting ll_ext_merge() to use read_mmfile(), since its original\n> code did add a NUL byte (even though I cannot find any case where it\n> would have mattered). Let's add the same defensive NUL in read_mmfile()\n> by using xmallocz() instead of xmalloc().\n\nNit: I guess this is an artifact from the reorder, but this sounds as if\n`ll_ext_merge()` wouldn't append the NUL byte anymore. But at this step\nit still does, as the change to `read_mmfile()` now happens before the\nchange to `ll_ext_merge()`.\n\nPatrick\n"},{"id":"553835","messageId":"ar5dDe02hgodgOHS@pks.im","threadId":"66416","inReplyTo":"20260930234416.GE1347555@coredump.intra.peff.net","subject":"Re: [PATCH v2 5/7] merge-ll: use read_mmfile() to read external merge results","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-10-01T13:15:57Z","receivedAt":"2026-10-01T13:16:08Z","isPatch":true,"body":"On Wed, Sep 30, 2026 at 07:44:16PM -0400, Jeff King wrote:\n> After running an external merge driver, ll_ext_merge() reads the result\n> back from a temporary file. We can do the same thing with much less code\n> by using read_mmfile().\n> \n> There are also two behavior improvements.\n> \n> One, read_mmfile() correctly uses xsize_t() to detect the case when we'd\n> truncate the result.\n> \n> And two, read_mmfile() will report errors to stderr if it can't read the\n> file (whereas the existing code silently returned NULL). I think most\n> callers would have said _something_ in this case like \"failed to execute\n> merge\" (from merge-ort), but more specifics are probably helpful (e.g.,\n> to distinguish a random system error from a badly configured merge\n> driver).\n\nOkay. Those code paths would now print two error messages, but that's\nprobably fine.\n\nPatrick\n"},{"id":"553839","messageId":"xmqqpkxt77pd.fsf@gitster.g","threadId":"66416","inReplyTo":"20260930224142.GB763270@coredump.intra.peff.net","subject":"Re: [PATCH 7/5] merge-ll: report an error when reading external merge results fails","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-10-01T15:37:34Z","receivedAt":"2026-10-01T15:37:36Z","isPatch":true,"body":"Jeff King writes:\n\n> On Wed, Sep 30, 2026 at 11:01:28AM -0700, Junio C Hamano wrote:\n>\n>> Jeff King writes:\n>> \n>> > Here's a resend of that final patch (not just a squash, because the\n>> > commit message mentioned the chmod).\n>> \n>> Makes sense.\n>> \n>> These 6/5 and 7/5 are probably better squashed into 5/5 than left as\n>> \"oops that was bad, so here is a preliminary clean-up to make the\n>> fix easier (6/5), and here is the fix of the fifth step (7/5)\", no?\n>\n> I don't think it is the fault of 5/5 at all (which carefully tried to\n> maintain the NULL behavior). The problem fixed by 7/5 existed before my\n> series.\n\nAh, OK, rereading the code before 5/5 is applied, I notice that we\nare not declaring the result is bad when we jump to \"bad:\" label\nafter noticing an I/O error. The code only paid attention to the\nstatus returned by run_command().\n\n> In theory that fix _could_ come earlier in the series, but it's actually\n> much easier to fix after 5/5, because we have a single spot to error\n> check.\n\nTrue. Thanks.\n"},{"id":"553840","messageId":"xmqqld8h77jo.fsf@gitster.g","threadId":"66416","inReplyTo":"20260930224613.GA765052@coredump.intra.peff.net","subject":"Re: [PATCH 2/5] xdiff: replace mmbuffer_t with mmfile_t","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-10-01T15:40:59Z","receivedAt":"2026-10-01T15:41:01Z","isPatch":true,"body":"Jeff King writes:\n\n> On Wed, Sep 30, 2026 at 05:32:49PM +0200, Patrick Steinhardt wrote:\n>\n>> > Let's use mmfile_t for both cases and drop mmbuffer_t. The latter is\n>> > probably a more descriptive name, but we have many more uses of\n>> > mmfile_t (and helpers like read_mmfile). So let's consolidate using that\n>> > name; we can always change it to something more sensible later.\n>> \n>> Yeah, that was my initial reaction, too. `mmbuffer_t` is indeed a better\n>> name as `mmfile_t` indicates that it's coming from... well, a file. And\n>> that's not necessarily true.\n>> \n>> I do wonder whether we should just aim for gradual improvement and use\n>> `mmbuffer_t` regardless or even shoot for something altogether different\n>> like `struct xdiff_buf` and then simply not mind the fact that we're\n>> being inconsistent. That would at least be an initial step into a better\n>> direction in my opinion, and we can then touch up things over some time.\n>> \n>> But I won't insist on any change like that, I'm okay with keeping\n>> `mmfile_t`.\n>\n> I'd really prefer to punt on it for now, just because the diff would be\n> _so_ big, and has so many extra rabbit holes (e.g., should \"mmfile_t\n> *mf\" get a new variable name?).\n\nI am happy enough with the fact that mmfile is shorter than mmbuffer ;-)\n\nAfter all xdiff is about comparing two files, and if you do not have\nfiles to compare, you create mmfile out of what you have (which may\nnot be a file) and pass it to xdiff, pretending it were a file. You\ntell the API that that mmfile has contents from what path etc., so\nat that point, the argument that says mmbuffer_t is more generic and\ncan represent any non-file sources does not really matter, I would\nhave to say.\n"}]}