Re: [PATCH ps/stash-in-c] strbuf_vinsertf: provide the correct buffer size to vsnprintf

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Feb 4, 2019, 10:54 UTC
Message-ID: <nycvar.QRO.7.76.6.1902041138550.41@tvgsbejvaqbjf.bet>
In-Reply-To: <896ae9dd-7ac3-182e-6692-c09bc4864de0@kdbg.org>

Hi Hannes,

On Sun, 3 Feb 2019, Johannes Sixt wrote:

Show 33 quoted lines

> strbuf_vinsertf inserts a formatted string in the middle of an existing
> strbuf value. It makes room in the strbuf by moving existing string to
> the back, then formats the string to insert directly into the hole.
> 
> It uses vsnprintf to format the string. The buffer size provided in the
> invocation is the number of characters available in the allocated space
> behind the final string. This does not make any sense at all.
> 
> Fix it to pass the length of the inserted string plus one for the NUL.
> (The functions saves and restores the character that the NUL occupies.)
> 
> Signed-off-by: Johannes Sixt <j6t@kdbg.org>
> ---
>  I found this, because in my environment I have to compile with
>  SNPRINTF_RETURNS_BOGUS. Our implementation of vsnprintf in
>  compat/snprintf.c writes into the end of the buffer unconditionally,
>  at a spot that is unrelated to the formatted string, and this leads to
>  "BUG: a NUL byte in commit log message not allowed" in some "stash"
>  tests.
> 
>  strbuf.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/strbuf.c b/strbuf.c
> index bfbbdadbf3..87ecf7f975 100644
> --- a/strbuf.c
> +++ b/strbuf.c
> @@ -270,7 +270,7 @@ void strbuf_vinsertf(struct strbuf *sb, size_t pos, const char *fmt, va_list ap)
>  	memmove(sb->buf + pos + len, sb->buf + pos, sb->len - pos);
>  	/* vsnprintf() will append a NUL, overwriting one of our characters */
>  	save = sb->buf[pos + len];
> -	len2 = vsnprintf(sb->buf + pos, sb->alloc - sb->len, fmt, ap);
> +	len2 = vsnprintf(sb->buf + pos, len + 1, fmt, ap);

It is really unfortunate that we use a non-dynamic code review system where it is pretty impossible to increase the amount of context lines easily.

And in this instance, a single line before the shown context would suffice:

	strbuf_grow(sb, len);

Which is in line with moving from `pos` to `pos + len`, and it is also in line with saving the byte at `pos + len`. And since we must consider that byte as part of the buffer, and since `vsnprintf()` wants that size of the buffer (including trailing NUL), `len + 1` is correct.

And since `strbuf_grow(sb, len)` would not in general grow the buffer by exactly `len` bytes, you indeed fixed a bug.

For historical context, when I first implemented `strbuf_vinsertf()`, I first grew the buffer, then let `vsnprintf()` write to the end, and then would rotate the bytes into their correct location. This required the implementation of an in-place rotation scheme, which was a lot of fun, and totally unnecessary. That `sb->alloc - sb->len` parameter you fixed was a remainder of that fun side project.

Thanks for fixing it, Dscho

P.S.: Side note: I just realized that we could also write

	save = sb->buf[pos];
  	memmove(sb->buf + pos + len + 1, sb->buf + pos + 1, sb->len - pos - 1);

instead, i.e. not move the first byte just to have it overwritten by vsnprintf() immediately, saving on moving one byte. But quite frankly, in this case I do agree that readability is more important than trying to squeeze out the last bit of performance.

Show 6 quoted lines

>  	sb->buf[pos + len] = save;
>  	if (len2 != len)
>  		BUG("your vsnprintf is broken (returns inconsistent lengths)");
> -- 
> 2.20.1.86.gb0de946387
>