Re: [PATCH 1/1] vreportf(): avoid buffered write in favor of unbuffered one

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 29, 2019, 03:18 UTC
Message-ID: <xmqqeeyw6xyr.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <455026ce3ef2b2d7cfecfc4b4bf5b588eebddcfe.1572274859.git.gitgitgadget@gmail.com>

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com> writes:

> Also please note that we `fflush(stderr)` here to help when running in a
> Git Bash on Windows: in this case, `stderr` is not actually truly
> unbuffered, and needs the extra help.

Yuck.  So on all systems, vreportf() now totally bypasses stdio?

Also, this is only to help output from us that goes via vreportf() and other codepaths in us that use stdio to write to the standard error stream can still get mixed on Windows (I think the answer is yes, because we wouldn't need fflush() in this patch if we are covering all writes to the standard error stream)?

By the way, I'd retitle the patch to highlight that we are still doing buffered write, if I were doing this topic. We are just avoiding some implementations of stdio that do not give us buffering and doing the buffering ourselves.

    Subject: vreportf(): don't rely on stdio buffering

or something like that.

Show 17 quoted lines

> Co-authored-by: Alexandr Miloslavskiy <alexandr.miloslavskiy@syntevo.com>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  usage.c | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/usage.c b/usage.c
> index 2fdb20086b..4328894dce 100644
> --- a/usage.c
> +++ b/usage.c
> @@ -10,13 +10,19 @@ void vreportf(const char *prefix, const char *err, va_list params)
>  {
>  	char msg[4096];
>  	char *p;
> -
> -	vsnprintf(msg, sizeof(msg), err, params);
> +	size_t off = strlcpy(msg, prefix, sizeof(msg));

Like snprintf(3) the strlcpy() and strlcat() functions return the total length of the string they tried to create. For strlcpy() that means the length of src.

So "off" would be strlen(prefix), which could be longer than sizeof(msg)? Then what happens to this vsnprintf() we see below?

Show 6 quoted lines

> +	int ret = vsnprintf(msg + off, sizeof(msg) - off, err, params);
>  	for (p = msg; *p; p++) {
>  		if (iscntrl(*p) && *p != '\t' && *p != '\n')
>  			*p = '?';
>  	}
> -	fprintf(stderr, "%s%s\n", prefix, msg);

Strictly speaking this is a breaking change in that control sequences in prefix are now clobbered. Does any caller call this function with prefix like "^M\033[K<some string>" to overwrite the last output line with the new message? If not, then probably we do not have to worry about it (and reusing msg[] does feel attractive).

Show 10 quoted lines

> +	if (ret > 0) {
> +		if (off + ret > sizeof(msg) - 1)
> +			ret = sizeof(msg) - 1 - off;
> +		msg[off + ret] = '\n'; /* we no longer need a NUL */
> +		fflush(stderr);
> +		xwrite(2, msg, off + ret + 1);
> +	}
>  }
>  
>  static NORETURN void usage_builtin(const char *err, va_list params)