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:

quoted

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.

quoted

+	*(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