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

[Junio C Hamano <gitster@pobox.com>]

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

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




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