From: Vaidas Pilkauskas <vaidas.pilkauskas@shopify.com>
Date: Wed, 18 Feb 2026 13:43:22 GMT
Subject: Re: [PATCH v3 1/3] strbuf: fix incorrect alloc size in strbuf_reencode()
Message-ID: <CAGjQmDODbvzRMO+V4MC_acCAbJh0=A-6ZWswbQCe+tK7ejRGoA@mail.gmail.com>
In-Reply-To: <xmqqseaz9jrd.fsf@gitster.g>

On Tue, Feb 17, 2026 at 10:51 PM Junio C Hamano <gitster@pobox.com> wrote:
> > From: Vaidas Pilkauskas <vaidas.pilkauskas@shopify.com>
> >
> > The strbuf_reencode() function incorrectly passes the string length
> > as the allocation size to strbuf_attach(), when it should pass
> > length + 1 to account for the null terminator.
> >
> > The reencode_string_len() function allocates len + 1 bytes (including
> > the null terminator) and returns the string length (excluding the null
> > terminator) via the len parameter. However, strbuf_reencode() then
> > calls strbuf_attach() with this length value as both the len and alloc
> > parameters:
> >
> >     strbuf_attach(sb, out, len, len);
> >
> > This is incorrect because strbuf_attach()'s alloc parameter should
> > reflect the actual allocated buffer size, which includes space for the
> > null terminator. This could lead to incorrect memory management in code
> > that relies on sb->alloc being accurate.
>
> I do agree that setting the correct number to .alloc member is a
> good thing to do, but I am afraid that the above characterization of
> a potential problem is incorrect.
>
> If we were to extend the resulting strbuf further (by e.g.,
> appending to it), we might end up reallocating the buffer a bit
> prematurely by one byte before it actually fills up, but the
> reallocation would be done by giving the piece of memory pointed at
> by "out" here to realloc(3), so the wrong value of "alloc" would not
> lead to incorrect memory management at all.
>
> Upon further inspection, we see something else interesting.  The
> strbuf_attach() function, immediately after initializing sb with the
> new values of buf/len/alloc, calls strbuf_grow(sb, 0) and triggers
> the ALLOC_GROW() growth thanks to this under specification.  By the
> time the control returns to the caller, the sb->alloc would be
> (((len)+16)*3/2), not (len+1), and it records the actual allocation
> size.  So there is no "could lead to incorrect memory management" at
> all, but this incorrect number forces us to always reallocate
> immediately after the strbuf_attach() call, which is a waste when we
> are not going to further extend the strbuf returned by this function.
>
> And that is a very good reason to make this fix worth doing.

I agree that this is incorrect characterization. What about something like this:

    strbuf: pass correct alloc to strbuf_attach() in strbuf_reencode()

    reencode_string_len() allocates len+1 bytes (including the NUL) and
    returns the string length in len. strbuf_reencode() was calling
    strbuf_attach(sb, out, len, len), so alloc was one byte too small.

    strbuf_attach() then calls strbuf_grow(sb, 0). With alloc < len+1,
    ALLOC_GROW always reallocates, so we reallocated immediately after
    attach even when the strbuf was not extended further. Pass len+1 as
    the alloc argument so the existing buffer is reused and the
    reallocation is avoided.


> > Fix by passing len + 1 as the alloc parameter:
> >
> >     strbuf_attach(sb, out, len, len + 1);
>
> I wonder how widespread this off-by-one error is.  Shouldn't
> strbuf_attach() be doing some sanity checking of its parameters?
>
>         void strbuf_attach(struct strbuf *sb, void *buf, size_t len, size_t alloc)
>         {
>
>                 strbuf_release(sb);
>                 sb->buf   = buf;
>                 sb->len   = len;
>                 sb->alloc = alloc;
>                 strbuf_grow(sb, 0);
>                 sb->buf[sb->len] = '\0';
>         }
>
> Given the above code, it is clear that alloc must be at least as big
> as (len + 1), and the strbuf_grow(sb, 0) in between is papering over
> problems (at least it is doing so here for the caller you corrected).
>
> Perhaps we want to replace the call to strbuf_grow(sb, 0) with
> something like
>
>                 if (alloc <= len)
>                         BUG("alloc must be larger than len");
>
> instead?  The log message of 917c9a71 (New strbuf APIs: splice and
> attach., 2007-09-15) is worth reading, but it is an iffy logic that
> depends too much (at least for my taste) on what strbuf_grow(sb, 0)
> actually does ;-).

I'll send patches, one to clean up call sites (there aren't too many - 7 places)
and another to add BUG() check to enforce the contract.

Thanks, Junio, for the review!

>
> > Signed-off-by: Vaidas Pilkauskas <vaidas.pilkauskas@shopify.com>
> > ---
> >  strbuf.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/strbuf.c b/strbuf.c
> > index 3939863cf3..3e04addc22 100644
> > --- a/strbuf.c
> > +++ b/strbuf.c
> > @@ -168,7 +168,7 @@ int strbuf_reencode(struct strbuf *sb, const char *from, const char *to)
> >       if (!out)
> >               return -1;
> >
> > -     strbuf_attach(sb, out, len, len);
> > +     strbuf_attach(sb, out, len, len + 1);
> >       return 0;
> >  }