Hi Peff,
On Tue, 29 Oct 2019, Jeff King wrote:
> 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.
An earlier (unsent) iteration did exactly that, but it was quite a bit
more unreadable because of the required arithmetics with `sizeof(msg)`.
> - I don't think "p == pend - 1" can ever trigger, since either
> vsnprintf() or we will have just written a NUL.
You are right, but I wanted to make extra sure that this code is robust
even (or: especially) in the presence of buggy libc functions.
It's not even expensive, I don't think.
> - 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).
It is true that the test suite bails out with a `BUG()` when
`vsnprintf()` is broken. But I figured that we want to be safe rather
than sorry. Besides, I have no full picture about what potential reasons
could make `vsnprintf()` return a negative value: for what I know, an
invalid format string could trigger that. And I _really_ want this code
path to be as robust as I can make it.
> If you're sick of bikeshedding, though, I can live without any of those
> being addressed.
Oh, that's okay, and I would not even call it bikeshedding, I think you
raised valid concerns.
> > + *(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?
I am not aware of any platform where `fprintf()` would automatically
transform `\n` to `\r\n`. Not unless the `FILE *` in question has been
opened with the `t` flag. And I am rather certain that `stderr` is not
opened with that flag. And if it was, I would force it off in Git for
Windows.
Thanks,
Dscho
