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

From: Jeff King <peff@peff.net>
Date: Sep 30, 2026, 22:49 UTC
Message-ID: <20260930224935.GB765052@coredump.intra.peff.net>
In-Reply-To: <ar0rp1cSIKuCMZyQ@pks.im>

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

Show 13 quoted lines

> > 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.

Show 13 quoted lines

> > 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