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

[Jeff King <peff@peff.net>]

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

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

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

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

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

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

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