From: Junio C Hamano <gitster@pobox•com>
To: Johannes Sixt <j6t@kdbg•org>
Cc: Paul-Sebastian Ungureanu <ungureanupaulsebastian@gmail•com>,
Git Mailing List <git@vger•kernel.org>
Subject: Re: [PATCH ps/stash-in-c] strbuf_vinsertf: provide the correct buffer size to vsnprintf
Date: Mon, 04 Feb 2019 13:57:02 -0800 [thread overview]
Message-ID: <xmqqd0o7tdxt.fsf@gitster-ct.c.googlers.com> (raw)
In-Reply-To: <896ae9dd-7ac3-182e-6692-c09bc4864de0@kdbg.org> (Johannes Sixt's message of "Sun, 3 Feb 2019 17:51:54 +0100")
Johannes Sixt <j6t@kdbg•org> 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@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.
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)");
next prev parent reply other threads:[~2019-02-04 21:57 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-02-03 16:51 [PATCH ps/stash-in-c] strbuf_vinsertf: provide the correct buffer size to vsnprintf Johannes Sixt
2019-02-04 7:25 ` Johannes Sixt
2019-02-04 10:38 ` Johannes Schindelin
2019-02-04 21:13 ` Johannes Sixt
2019-02-05 10:38 ` Johannes Schindelin
2019-02-04 10:54 ` Johannes Schindelin
2019-02-04 21:57 ` Junio C Hamano [this message]
2019-02-05 11:06 ` Johannes Schindelin
2019-02-05 18:01 ` Junio C Hamano
2019-02-06 11:41 ` Johannes Schindelin
2019-02-05 20:30 ` Johannes Sixt
2019-02-06 11:56 ` Johannes Schindelin
2019-02-06 18:11 ` Johannes Sixt
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=xmqqd0o7tdxt.fsf@gitster-ct.c.googlers.com \
--to=gitster@pobox$(echo .)com \
--cc=git@vger$(echo .)kernel.org \
--cc=j6t@kdbg$(echo .)org \
--cc=ungureanupaulsebastian@gmail$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox