Re: [PATCH 5/5] xdiff: NUL-terminate buffers read by read_mmfile()

On Wed, Sep 30, 2026 at 05:32:55PM +0200, Patrick Steinhardt wrote:

quoted

This one is obviously optional, which is why I put it last.

Hm, I'm somewhat indifferent here. It always feels a bit weird to be
this defensive because "programming errors", as the next question then
is "but what about all the other errors where we're not defensive?" But
the xdiff code is complex enough with a bunch of pointer arithmetics, so
maybe it's not even that bad of an idea.

That being said, I feel like a better course of action could be to use a
fuzzer for this code, because as far as I'm aware we have none yet, and
that would potentially shake out a bunch of bugs. But that still doesn't
really help us to catch platform-specific bugs due to different integer
sizes.

I look at it as: why not do both?

Mostly the lack of extra NUL surprised me, as we routinely add one in
most other places (and it has prevented some memory bugs in the past).

The counterargument is that before your 3/5 we used to use xmallocz, so
you're essentially just reinstating the previous safety guards.

Yes, though I did confirm that those guards were doing nothing. This is
less about protecting the new ll_ext_merge() caller and more about all
of the _other_ callers of read_mmfile().

But yeah, it is obviously a lot easier to explain if this patch comes
first. I just wasn't sure if we'd want to drop it or not (though yeah,
we probably should explain in the earlier patch that the lack of NUL
termination is OK).

I'll re-roll with this patch earlier in the series.

quoted

diff --git a/xdiff-interface.c b/xdiff-interface.c
index bc340d5a8a..b3e9f1952b 100644
--- a/xdiff-interface.c
+++ b/xdiff-interface.c

@@ -166,7 +166,7 @@ int read_mmfile(mmfile_t *ptr, const char *filename)
 	if (!(f = fopen(filename, "rb")))
 		return error_errno("Could not open %s", filename);
 	sz = xsize_t(st.st_size);
-	ptr->ptr = xmalloc(sz ? sz : 1);
+	ptr->ptr = xmallocz(sz);

I was staring at this code a while before I noticed the added `z` at the
end of this function.

Heh, fair. I'll say something more explicit in the commit message when
re-rolling.

-Peff