Re: [PATCH v3 1/1] vreportf(): avoid relying on stdio buffering
- From
Johannes Schindelin <johannes.schindelin@gmx.de>
- Date
- Oct 30, 2019, 08:54 UTC
- Message-ID
- <nycvar.QRO.7.76.6.1910300947410.46@tvgsbejvaqbjf.bet>
- In-Reply-To
- <20191029203259.GB12337@sigill.intra.peff.net>
Hi Peff,
On Tue, 29 Oct 2019, Jeff King wrote:
Show 29 quoted lines
> 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.
Show 5 quoted lines
> - 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.
Show 7 quoted lines
> > + *(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