Johannes Sixt <j6t@xxxxxxxx> writes:
> 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.
An embarrassing indication that not every line is read during
development or review with fine toothed comb. I guess it won't
trigger without the returns-bogus thing, and the "testing" done on
platforms did not help.
Thanks for finding it. This needs to be squashed into bfc3fe33
("strbuf.c: add `strbuf_insertf()` and `strbuf_vinsertf()`",
2018-12-20)?
As you mention in the direct response of this message, it might make
sense to get rid of the helper with YAGNI, but that's something we
need to see what its potential users should be doing (note: I did
not say "what its potential users are doing").
It could turn out that there may be many places that could use this
helper, but it may merely be an indication that these many places
are structured poorly to compute the "shell" string too early before
the string to be inserted becomes computable, in which case the real
fix may not be to use this helper but to change the order of
building up the string. Perhaps we want a string that is
concatenation of part #1, part #2 and part #3, but we somehow first
compute concatenation of part #1 and part #3 in a buffer, which
requires us to insert part #2 in between. If we restructure the
code to keep parts #1 and #3 separate, or delay computing part #3,
until part #2 becomes available, that would fix the "potential
users" in a better way without having to make calls into this
helper, for example.
> 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);
> sb->buf[pos + len] = save;
> if (len2 != len)
> BUG("your vsnprintf is broken (returns inconsistent lengths)");
