On Tue, Oct 29, 2019 at 08:01:20PM +0000, Johannes Schindelin via GitGitGadget wrote:
> diff --git a/usage.c b/usage.c
> index 2fdb20086b..471efb2de9 100644
> --- a/usage.c
> +++ b/usage.c
> @@ -9,14 +9,21 @@
> void vreportf(const char *prefix, const char *err, va_list params)
> {
> char msg[4096];
> - char *p;
> + size_t off = strlcpy(msg, prefix, sizeof(msg));
> + char *p, *pend = msg + sizeof(msg);
>
> - vsnprintf(msg, sizeof(msg), err, params);
> - for (p = msg; *p; p++) {
> + p = off < pend - msg ? msg + off : pend - 1;
> + if (vsnprintf(p, pend - p, err, params) < 0)
> + *p = '\0'; /* vsnprintf() failed, clip at prefix */
> +
> + for (; p != pend - 1 && *p; p++) {
> if (iscntrl(*p) && *p != '\t' && *p != '\n')
> *p = '?';
> }
This version looks OK to me. Some bikeshedding:
- I suspect it may be more readable to just stick to offsets instead
of pointers, since that's what strlcpy() and vsnprintf() give us.
- I don't think "p == pend - 1" can ever trigger, since either
vsnprintf() or we will have just written a NUL.
- Do we need to contend with vsnprintf() return a negative value in
general in our codebase? We already BUG() on it elsewhere. Yes, that
BUG() would try to write via this code path, but it implies to me
that we've already dealt with any such broken vsnprintf()
implementations (via compat/snprintf.c).
If you're sick of bikeshedding, though, I can live without any of those
being addressed.
> + *(p++) = '\n'; /* we no longer need a NUL */
> + fflush(stderr);
> + write_in_full(2, msg, p - msg);
One non-bikeshed question: would fprintf() on some platforms have sent
"\r\n", which is no longer happening with our write()? Do we need to
care about that?
-Peff
