[PATCH v2 1/7] xdiff: clean up read_mmfile() allocations on error
- From
Jeff King <peff@peff.net>
- Date
- Sep 30, 2026, 23:44 UTC
- Message-ID
- <20260930234402.GA1347555@coredump.intra.peff.net>
- In-Reply-To
- <20260930234348.GA1340390@coredump.intra.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> --- builtin/rerere.c | 6 +++++- rerere.c | 3 +-- xdiff-interface.c | 1 + 3 files changed, 7 insertions(+), 3 deletions(-)
diff --git a/builtin/rerere.c b/builtin/rerere.c index a056cb791b..d39c6e8445 100644 --- a/builtin/rerere.c +++ b/builtin/rerere.c @@ -34,8 +34,12 @@ static int diff_two(const char *file1, const char *label1, mmfile_t minus, plus; int ret; - if (read_mmfile(&minus, file1) || read_mmfile(&plus, file2)) + if (read_mmfile(&minus, file1)) return -1; + if (read_mmfile(&plus, file2)) { + free(minus.ptr); + return -1; + } printf("--- a/%s\n+++ b/%s\n", label1, label2); fflush(stdout); diff --git a/rerere.c b/rerere.c index 1c3745d9e3..856347c9ae 100644 --- a/rerere.c +++ b/rerere.c @@ -1039,7 +1039,7 @@ static int rerere_forget_one_path(struct index_state *istate, for (id->variant = 0; id->variant < id->collection->status_nr; id->variant++) { - mmfile_t cur = { NULL, 0 }; + mmfile_t cur; mmbuffer_t result = {NULL, 0}; int cleanly_resolved; @@ -1048,7 +1048,6 @@ static int rerere_forget_one_path(struct index_state *istate, handle_cache(istate, path, hash, rerere_path(&buf, id, "thisimage")); if (read_mmfile(&cur, rerere_path(&buf, id, "thisimage"))) { - free(cur.ptr); error(_("failed to update conflicted state in '%s'"), path); goto fail_exit; } diff --git a/xdiff-interface.c b/xdiff-interface.c index db6938689f..e3dd2184ae 100644 --- a/xdiff-interface.c +++ b/xdiff-interface.c @@ -168,6 +168,7 @@ int read_mmfile(mmfile_t *ptr, const char *filename) sz = xsize_t(st.st_size); ptr->ptr = xmalloc(sz ? sz : 1); if (sz && fread(ptr->ptr, sz, 1, f) != 1) { + FREE_AND_NULL(ptr->ptr); fclose(f); return error("Could not read %s", filename); }
-- 2.56.0.354.gb6b32d5be5