From: Jeff King <peff@peff.net>
Date: Mon, 14 Sep 2026 16:53:50 GMT
Subject: Re: [PATCH v2 1/3] merge-ll: use strbuf to read back external merge result
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:

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