Merge branch 'jk/xdiff-size_t'

The internal diff and merge machinery has been updated to use
standard size_t types rather than signed longs for buffer
allocations and offsets, preventing subtle overflow issues on LLP64
architectures, and plugging uninitialized memory leaks along error
paths.

* jk/xdiff-size_t:
  merge-ll: report an error when reading external merge results fails
  merge-ll: handle external driver status before reading result
  merge-ll: use read_mmfile() to read external merge results
  xdiff: NUL-terminate buffers read by read_mmfile()
  xdiff: use size_t for buffer sizes
  xdiff: replace mmbuffer_t with mmfile_t
  xdiff: clean up read_mmfile() allocations on error

Documentation/technical/api-merge.adoc

@@ -20,11 +20,10 @@ responsible for a few things.
 Data structures
 ---------------
 
-* `mmbuffer_t`, `mmfile_t`
+* `mmfile_t`
 
-These store data usable for use by the xdiff backend, for writing and
-for reading, respectively.  See `xdiff/xdiff.h` for the definitions
-and `diff.c` for examples.
+This stores a buffer and its size for input to or output from the xdiff
+backend. See `xdiff/xdiff.h` for the definition and `diff.c` for examples.
 
 * `struct ll_merge_options`
 

apply.c

@@ -3647,7 +3647,7 @@ static int three_way_merge(struct apply_state *state,
 {
 	mmfile_t base_file, our_file, their_file;
 	struct ll_merge_options merge_opts = LL_MERGE_OPTIONS_INIT;
-	mmbuffer_t result = { NULL };
+	mmfile_t result = { NULL };
 	enum ll_merge_result status;
 
 	/* resolve trivial cases first */

builtin/checkout.c

@@ -320,7 +320,7 @@ static int checkout_merged(int pos, const struct checkout *state,
 	enum ll_merge_result merge_status;
 	int status;
 	struct object_id oid;
-	mmbuffer_t result_buf;
+	mmfile_t result_buf;
 	struct object_id threeway[3];
 	unsigned mode = 0;
 	struct ll_merge_options ll_opts = LL_MERGE_OPTIONS_INIT;

builtin/merge-file.c

@@ -64,7 +64,7 @@ int cmd_merge_file(int argc,
 {
 	const char *names[3] = { 0 };
 	mmfile_t mmfs[3] = { 0 };
-	mmbuffer_t result = { 0 };
+	mmfile_t result = { 0 };
 	xmparam_t xmp = { 0 };
 	int ret = 0, i = 0, to_stdout = 0, object_id = 0;
 	int quiet = 0;

builtin/merge-tree.c

@@ -109,7 +109,7 @@ static void *origin(struct merge_list *entry, size_t *size)
 	return NULL;
 }
 
-static int show_outf(void *priv UNUSED, mmbuffer_t *mb, int nbuf)
+static int show_outf(void *priv UNUSED, mmfile_t *mb, int nbuf)
 {
 	int i;
 	for (i = 0; i < nbuf; i++)

builtin/rerere.c

@@ -16,7 +16,7 @@ static const char * const rerere_usage[] = {
 	NULL,
 };
 
-static int outf(void *dummy UNUSED, mmbuffer_t *ptr, int nbuf)
+static int outf(void *dummy UNUSED, mmfile_t *ptr, int nbuf)
 {
 	int i;
 	for (i = 0; i < nbuf; i++)
@@ -34,8 +34,12 @@ static int diff_two(const char *file1, const char *label1,
 	mmfile_t minus, plus;
 	int ret;
 
-	if (read_mmfile(&minus, file1) || read_mmfile(&plus, file2))
+	if (read_mmfile(&minus, file1))
 		return -1;
+	if (read_mmfile(&plus, file2)) {
+		free(minus.ptr);
+		return -1;
+	}
 
 	printf("--- a/%s\n+++ b/%s\n", label1, label2);
 	fflush(stdout);

diff.c

@@ -3646,7 +3646,7 @@ static void emit_binary_diff_body(struct diff_options *o,
 		data = delta;
 		data_size = delta_size;
 	} else {
-		char *s = xstrfmt("%lu", two->size);
+		char *s = xstrfmt("%"PRIuMAX, (uintmax_t)two->size);
 		emit_diff_symbol(o, DIFF_SYMBOL_BINARY_DIFF_HEADER_LITERAL,
 				 s, strlen(s), 0);
 		free(s);

merge-blobs.c

@@ -38,7 +38,7 @@ static void *three_way_filemerge(struct index_state *istate,
 				 size_t *size)
 {
 	enum ll_merge_result merge_status;
-	mmbuffer_t res;
+	mmfile_t res;
 
 	/*
 	 * This function is only used by cmd_merge_tree, which

merge-ll.c

@@ -22,7 +22,7 @@
 struct ll_merge_driver;
 
 typedef enum ll_merge_result (*ll_merge_fn)(const struct ll_merge_driver *,
-			   mmbuffer_t *result,
+			   mmfile_t *result,
 			   const char *path,
 			   mmfile_t *orig, const char *orig_name,
 			   mmfile_t *src1, const char *name1,
@@ -57,7 +57,7 @@ void reset_merge_attributes(void)
  * Built-in low-levels
  */
 static enum ll_merge_result ll_binary_merge(const struct ll_merge_driver *drv UNUSED,
-			   mmbuffer_t *result,
+			   mmfile_t *result,
 			   const char *path UNUSED,
 			   mmfile_t *orig, const char *orig_name UNUSED,
 			   mmfile_t *src1, const char *name1 UNUSED,
@@ -102,7 +102,7 @@ static enum ll_merge_result ll_binary_merge(const struct ll_merge_driver *drv UN
 }
 
 static enum ll_merge_result ll_xdl_merge(const struct ll_merge_driver *drv_unused,
-			mmbuffer_t *result,
+			mmfile_t *result,
 			const char *path,
 			mmfile_t *orig, const char *orig_name,
 			mmfile_t *src1, const char *name1,
@@ -148,7 +148,7 @@ static enum ll_merge_result ll_xdl_merge(const struct ll_merge_driver *drv_unuse
 }
 
 static enum ll_merge_result ll_union_merge(const struct ll_merge_driver *drv_unused,
-			  mmbuffer_t *result,
+			  mmfile_t *result,
 			  const char *path,
 			  mmfile_t *orig, const char *orig_name,
 			  mmfile_t *src1, const char *name1,
@@ -203,7 +203,7 @@ static const char *temp_path_basename(struct tempfile *t)
  * User defined low-level merge driver support.
  */
 static enum ll_merge_result ll_ext_merge(const struct ll_merge_driver *fn,
-			mmbuffer_t *result,
+			mmfile_t *result,
 			const char *path,
 			mmfile_t *orig, const char *orig_name,
 			mmfile_t *src1, const char *name1,
@@ -215,8 +215,7 @@ static enum ll_merge_result ll_ext_merge(const struct ll_merge_driver *fn,
 	struct strbuf cmd = STRBUF_INIT;
 	const char *format = fn->cmdline;
 	struct child_process child = CHILD_PROCESS_INIT;
-	int status, fd;
-	struct stat st;
+	int status;
 	enum ll_merge_result ret;
 	assert(opts);
 
@@ -255,24 +254,6 @@ static enum ll_merge_result ll_ext_merge(const struct ll_merge_driver *fn,
 	child.use_shell = 1;
 	strvec_push(&child.args, cmd.buf);
 	status = run_command(&child);
-	fd = open(get_tempfile_path(tmp_a), O_RDONLY);
-	if (fd < 0)
-		goto bad;
-	if (fstat(fd, &st))
-		goto close_bad;
-	result->size = st.st_size;
-	result->ptr = xmallocz(result->size);
-	if (read_in_full(fd, result->ptr, result->size) != result->size) {
-		FREE_AND_NULL(result->ptr);
-		result->size = 0;
-	}
- close_bad:
-	close(fd);
- bad:
-	delete_tempfile(&tmp_o);
-	delete_tempfile(&tmp_a);
-	delete_tempfile(&tmp_b);
-	strbuf_release(&cmd);
 	if (!status)
 		ret = LL_MERGE_OK;
 	else if (status <= 128)
@@ -280,6 +261,14 @@ static enum ll_merge_result ll_ext_merge(const struct ll_merge_driver *fn,
 	else
 		/* died due to a signal: WTERMSIG(status) + 128 */
 		ret = LL_MERGE_ERROR;
+
+	if (read_mmfile(result, temp_path_basename(tmp_a)) < 0)
+		ret = LL_MERGE_ERROR;
+
+	delete_tempfile(&tmp_o);
+	delete_tempfile(&tmp_a);
+	delete_tempfile(&tmp_b);
+	strbuf_release(&cmd);
 	return ret;
 }
 
@@ -418,7 +407,7 @@ static void normalize_file(mmfile_t *mm, const char *path, struct index_state *i
 	}
 }
 
-enum ll_merge_result ll_merge(mmbuffer_t *result_buf,
+enum ll_merge_result ll_merge(mmfile_t *result_buf,
 	     const char *path,
 	     mmfile_t *ancestor, const char *ancestor_label,
 	     mmfile_t *ours, const char *our_label,

merge-ll.h

@@ -16,7 +16,7 @@
  *   If you have no special requests, skip this and pass `NULL`
  *   as the `opts` parameter to use the default options.
  *
- * - Allocate an mmbuffer_t variable for the result.
+ * - Allocate an mmfile_t variable for the result.
  *
  * - Allocate and fill variables with the file's original content
  *   and two modified versions (using `read_mmfile`, for example).
@@ -100,7 +100,7 @@ enum ll_merge_result {
  * `.gitattributes` or `.git/info/attributes` into account.
  * Returns 0 for a clean merge.
  */
-enum ll_merge_result ll_merge(mmbuffer_t *result_buf,
+enum ll_merge_result ll_merge(mmfile_t *result_buf,
 	     const char *path,
 	     mmfile_t *ancestor, const char *ancestor_label,
 	     mmfile_t *ours, const char *our_label,

merge-ort.c

@@ -2111,7 +2111,7 @@ static int merge_3way(struct merge_options *opt,
 		      const struct object_id *b,
 		      const char *pathnames[3],
 		      const int extra_marker_size,
-		      mmbuffer_t *result_buf)
+		      mmfile_t *result_buf)
 {
 	mmfile_t orig, src1, src2;
 	struct ll_merge_options ll_opts = LL_MERGE_OPTIONS_INIT;
@@ -2247,7 +2247,7 @@ static int handle_content_merge(struct merge_options *opt,
 
 	/* Remaining rules depend on file vs. submodule vs. symlink. */
 	else if (S_ISREG(a->mode)) {
-		mmbuffer_t result_buf;
+		mmfile_t result_buf;
 		int ret = 0, merge_status;
 		int two_way;
 

notes-merge.c

@@ -355,7 +355,7 @@ static void write_note_to_worktree(const struct object_id *obj,
 static int ll_merge_in_worktree(struct notes_merge_options *o,
 				struct notes_merge_pair *p)
 {
-	mmbuffer_t result_buf;
+	mmfile_t result_buf;
 	mmfile_t base, local, remote;
 	enum ll_merge_result status;
 

rerere.c

@@ -600,7 +600,7 @@ int rerere_remaining(struct repository *r, struct string_list *merge_rr)
  */
 static int try_merge(struct index_state *istate,
 		     const struct rerere_id *id, const char *path,
-		     mmfile_t *cur, mmbuffer_t *result)
+		     mmfile_t *cur, mmfile_t *result)
 {
 	enum ll_merge_result ret;
 	mmfile_t base = {NULL, 0}, other = {NULL, 0};
@@ -641,7 +641,7 @@ static int merge(struct index_state *istate, const struct rerere_id *id, const c
 	int ret;
 	struct strbuf buf = STRBUF_INIT;
 	mmfile_t cur = {NULL, 0};
-	mmbuffer_t result = {NULL, 0};
+	mmfile_t result = {NULL, 0};
 
 	/*
 	 * Normalize the conflicts in path and write it out to
@@ -969,7 +969,7 @@ static int handle_cache(struct index_state *istate,
 			const char *path, unsigned char *hash, const char *output)
 {
 	mmfile_t mmfile[3] = {{NULL}};
-	mmbuffer_t result = {NULL, 0};
+	mmfile_t result = {NULL, 0};
 	const struct cache_entry *ce;
 	int pos, len, i, has_conflicts;
 	struct rerere_io_mem io;
@@ -1061,8 +1061,8 @@ static int rerere_forget_one_path(struct index_state *istate,
 	for (id->variant = 0;
 	     id->variant < id->collection->status_nr;
 	     id->variant++) {
-		mmfile_t cur = { NULL, 0 };
-		mmbuffer_t result = {NULL, 0};
+		mmfile_t cur;
+		mmfile_t result = {NULL, 0};
 		int cleanly_resolved;
 
 		if (!has_rerere_resolution(id))
@@ -1070,7 +1070,6 @@ static int rerere_forget_one_path(struct index_state *istate,
 
 		handle_cache(istate, path, hash, rerere_path(&buf, id, "thisimage"));
 		if (read_mmfile(&cur, rerere_path(&buf, id, "thisimage"))) {
-			free(cur.ptr);
 			error(_("failed to update conflicted state in '%s'"), path);
 			goto fail_exit;
 		}

t/t4200-rerere.sh

@@ -734,4 +734,55 @@ test_expect_success 'rerere does not crash with unmatched conflict marker' '
 	test_must_fail git rebase --continue
 '
 
+test_expect_success 'rerere preserves conflicts when driver output is unreadable' '
+	test_create_repo unreadable-output &&
+	(
+		cd unreadable-output &&
+		git config rerere.enabled true &&
+		git config rerere.autoupdate true &&
+		write_script merge-driver <<-\EOF &&
+		git merge-file "$@"
+		status=$?
+		if test -f fail-read
+		then
+			rm "$1" || exit 1
+		fi
+		exit "$status"
+		EOF
+		git config merge.unreadable.driver "./merge-driver %A %O %B" &&
+		echo "file merge=unreadable" >.gitattributes &&
+		test_commit base file base &&
+		git checkout -b one &&
+		test_commit --no-tag one file one &&
+		git checkout -b two base &&
+		test_commit --no-tag two file two &&
+
+		# Teach rerere a resolution while the driver works normally.
+		test_must_fail git merge one &&
+		echo resolved >file &&
+		git rerere &&
+		git merge --abort &&
+
+		# Recreate the conflict without replaying the resolution yet.
+		test_must_fail git -c rerere.enabled=false merge one &&
+
+		# We will expect the same conflicted content after rerere fails
+		# below.
+		cp file expect &&
+		git ls-files -u >expect-index &&
+		test_file_not_empty expect-index &&
+
+		# Now we try rerere again, but the merge driver will cause the
+		# read to fail.
+		>fail-read &&
+		git rerere 2>err &&
+		test_grep "Could not stat" err &&
+
+		# And we expect the conflicted state.
+		test_cmp expect file &&
+		git ls-files -u >actual-index &&
+		test_cmp expect-index actual-index
+	)
+'
+
 test_done

xdiff-interface.c

@@ -53,7 +53,7 @@ static int consume_one(void *priv_, char *s, unsigned long size)
 	return 0;
 }
 
-static int xdiff_outf(void *priv_, mmbuffer_t *mb, int nbuf)
+static int xdiff_outf(void *priv_, mmfile_t *mb, int nbuf)
 {
 	struct xdiff_emit_state *priv = priv_;
 	int i;
@@ -166,8 +166,9 @@ int read_mmfile(mmfile_t *ptr, const char *filename)
 	if (!(f = fopen(filename, "rb")))
 		return error_errno("Could not open %s", filename);
 	sz = xsize_t(st.st_size);
-	ptr->ptr = xmalloc(sz ? sz : 1);
+	ptr->ptr = xmallocz(sz);
 	if (sz && fread(ptr->ptr, sz, 1, f) != 1) {
+		FREE_AND_NULL(ptr->ptr);
 		fclose(f);
 		return error("Could not read %s", filename);
 	}

xdiff/xdiff.h

@@ -70,14 +70,9 @@ extern "C" {
 
 typedef struct s_mmfile {
 	char *ptr;
-	long size;
+	size_t size;
 } mmfile_t;
 
-typedef struct s_mmbuffer {
-	char *ptr;
-	long size;
-} mmbuffer_t;
-
 typedef struct s_xpparam {
 	unsigned long flags;
 
@@ -96,7 +91,7 @@ typedef struct s_xdemitcb {
 			long old_begin, long old_nr,
 			long new_begin, long new_nr,
 			const char *func, long funclen);
-	int (*out_line)(void *, mmbuffer_t *, int);
+	int (*out_line)(void *, mmfile_t *, int);
 } xdemitcb_t;
 
 typedef long (*find_func_t)(const char *line, long line_len, char *buffer, long buffer_size, void *priv);
@@ -144,7 +139,7 @@ typedef struct s_xmparam {
 #define DEFAULT_CONFLICT_MARKER_SIZE 7
 
 int xdl_merge(mmfile_t *orig, mmfile_t *mf1, mmfile_t *mf2,
-		xmparam_t const *xmp, mmbuffer_t *result);
+		xmparam_t const *xmp, mmfile_t *result);
 
 #ifdef __cplusplus
 }

xdiff/xmerge.c

@@ -504,7 +504,7 @@ static int xdl_simplify_non_conflicts(xdfenv_t *xe1, xdmerge_t *m,
  */
 static int xdl_do_merge(xdfenv_t *xe1, xdchange_t *xscr1,
 		xdfenv_t *xe2, xdchange_t *xscr2,
-		xmparam_t const *xmp, mmbuffer_t *result)
+		xmparam_t const *xmp, mmfile_t *result)
 {
 	xdmerge_t *changes, *c;
 	xpparam_t const *xpp = &xmp->xpp;
@@ -682,7 +682,7 @@ static int xdl_do_merge(xdfenv_t *xe1, xdchange_t *xscr1,
 }
 
 int xdl_merge(mmfile_t *orig, mmfile_t *mf1, mmfile_t *mf2,
-		xmparam_t const *xmp, mmbuffer_t *result)
+		xmparam_t const *xmp, mmfile_t *result)
 {
 	xdchange_t *xscr1 = NULL, *xscr2 = NULL;
 	xdfenv_t xe1, xe2;

xdiff/xutils.c

@@ -39,7 +39,7 @@ uint64_t xdl_bogosqrt(uint64_t n) {
 int xdl_emit_diffrec(char const *rec, long size, char const *pre, long psize,
 		     xdemitcb_t *ecb) {
 	int i = 2;
-	mmbuffer_t mb[3];
+	mmfile_t mb[3];
 
 	mb[0].ptr = (char *) pre;
 	mb[0].size = psize;
@@ -392,7 +392,7 @@ static int xdl_format_hunk_hdr(long s1, long c1, long s2, long c2,
 			       const char *func, long funclen,
 			       xdemitcb_t *ecb) {
 	int nb = 0;
-	mmbuffer_t mb;
+	mmfile_t mb;
 	char buf[128];
 
 	memcpy(buf, "@@ -", 4);