Re: [PATCH v3 1/3] strbuf: fix incorrect alloc size in strbuf_reencode()

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 17, 2026, 20:51 UTC
Message-ID: <xmqqseaz9jrd.fsf@gitster.g>
In-Reply-To: <821043c664e41d8e395e944df3ada8f697a69d0b.1771326521.git.gitgitgadget@gmail.com>

"Vaidas Pilkauskas via GitGitGadget" <gitgitgadget@gmail.com> writes:

Show 18 quoted lines

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

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

Show 17 quoted lines

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