Hi Hannes,
On Sun, 3 Feb 2019, Johannes Sixt wrote:
> 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@xxxxxxxx>
> ---
> 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.
> sb->buf[pos + len] = save;
> if (len2 != len)
> BUG("your vsnprintf is broken (returns inconsistent lengths)");
> --
> 2.20.1.86.gb0de946387
>
