Re: [PATCH v2 1/3] merge-ll: use strbuf to read back external merge result

From: Jeff King <peff@peff.net>
Date: Sep 14, 2026, 16:53 UTC
Message-ID: <20260914165350.GA32247@peff.net>
In-Reply-To: <CABPp-BG9Hkc7i_JxAbYfyzu+b4Mc_pZUr0jJF=vY0jHSARpHzw@mail.gmail.com>

On Fri, Sep 11, 2026 at 11:06:33AM -0700, Elijah Newren wrote:

Show 18 quoted lines

> > -       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;
> > +
> > +       if (strbuf_read_file(&result_buf, temp[1], 0) >= 0) {
> > +               result->size = result_buf.len;
> > +               result->ptr = strbuf_detach(&result_buf, NULL);
> 
> I know the type mismatch is pre-existing, but the order makes the new
> behavior different. On LLP64, assuming the usual wraparound, a result
> of LONG_MAX + 101  narrows to the negative value  LONG_MIN + 100 .
> 
> The old code narrows before  xmallocz() , so it requests an impossibly
> large allocation and dies. The new code allocates the actual buffer
> first, then records a negative size; callers converting that size back
> to size_t could read past the allocation.

Hmm, yeah. I noticed the possible truncation, but reasoned that it was roughly the same before and after (the only difference being that we know would actually have the full buffer, just a truncated size). But you're right that negative values introduce their own distinct type of confusion.

> Would a simple fail-fast make sense?
> 
> if (result_buf.len > LONG_MAX)
>         die(_("external merge result is too large"));

Yeah. I think we should be doing that even with the current code, as it's possible for us to silently truncate a merge result (e.g., wrapping beyond 4GB goes back to 0).

There's a similar case in read_mmfile(). There we actually bother to use xsize_t() to catch _some_ problems, but of course we are using "long" and not "size_t" in the mmfile, so it's still subject to truncation.

We can't just use read_mmfile() here, because there is an artificial distinction between mmfile_t and mmbuffer_t, even though they hold the exact same members (IIRC, one is for "output"). But possibly we can use it and just assign the members, which is no worse than what we have to do with the strbuf.

I'll plan to add a check like the one above here and in read_mmfile(), and then look at re-working this cleanup to use that function. I'll probably also peel this off of the other patches. It's really two separate topics: this file-read cleanup, and the tempfile-deletion improvement that started the thread.

-Peff