[v2,1/3] sideband: avoid reporting incomplete sideband messages

Commit Message

Johannes Schindelin Oct. 19, 2020, 7:35 p.m. UTC

From: Johannes Schindelin <[e-mail]>

In 2b695ecd74d (t5500: count objects through stderr, not trace,
2020-05-06) we tried to ensure that the "Total 3" message could be
grepped in Git's output, even if it sometimes got chopped up into
multiple lines in the trace machinery.

However, the first instance where this mattered now goes through the
sideband machinery, where it is _still_ possible for messages to get
chopped up: it *is* possible for the standard error stream to be sent
byte-for-byte and hence it can be easily interrupted. Meaning: it is
possible for the single line that we're looking for to be chopped up
into multiple sideband packets, with a primary packet being delivered
between them.

This seems to happen occasionally in the `vs-test` part of our CI
builds, i.e. with binaries built using Visual C, but not when building
with GCC or clang; The symptom is that t5500.43 fails to find a line
matching `remote: Total 3` in the `log` file, which ends in something
along these lines:

	remote: Tota
	remote: l 3 (delta 0), reused 0 (delta 0), pack-reused 0

This should not happen, though: we have code in `demultiplex_sideband()`
_specifically_ to stitch back together lines that were delivered in
separate sideband packets.

However, this stitching was broken in a subtle way in fbd76cd450
(sideband: reverse its dependency on pkt-line, 2019-01-16): before that
change, incomplete sideband lines would not be flushed upon receiving a
primary packet, but after that patch, they would be.

The subtleness of this bug comes from the fact that it is easy to get
confused by the ambiguous meaning of the `break` keyword: after writing
the primary packet contents, the `break;` in the original version of
`recv_sideband()` does _not_ break out of the `while` loop, but instead
only ends the `switch` case:

	while (!retval) {
		[...]
		switch (band) {
			[...]
		case 1:
/* Write the contents of the primary packet */
			write_or_die(out, buf + 1, len);
/* Here, we do *not* break out of the loop, `retval` is unchanged */
			break;
		[...]
	}

	if (outbuf.len) {
/* Write any remaining sideband messages lacking a trailing LF */
		strbuf_addch(&outbuf, '\n');
		xwrite(2, outbuf.buf, outbuf.len);
	}

In contrast, after fbd76cd450 (sideband: reverse its dependency on
pkt-line, 2019-01-16), the body of the `while` loop was extracted into
`demultiplex_sideband()`, crucially _including_ the logic to write
incomplete sideband messages:

	switch (band) {
	[...]
	case 1:
		*sideband_type = SIDEBAND_PRIMARY;
/* This does not break out of the loop: the loop is in the caller */
		break;
	[...]
	}

cleanup:
	[...]
/* This logic is now no longer _outside_ the loop but _inside_ */
	if (scratch->len) {
		strbuf_addch(scratch, '\n');
		xwrite(2, scratch->buf, scratch->len);
	}

The correct way to fix this is to return from `demultiplex_sideband()`
early. The caller will then write out the contents of the primary packet
and continue looping. The `scratch` buffer for incomplete sideband
messages is owned by that caller, and will continue to accumulate the
remainder(s) of those messages. The loop will only end once
`demultiplex_sideband()` returned non-zero _and_ did not indicate a
primary packet, which is the case only when we hit the `cleanup:` path,
in which we take care of flushing any unfinished sideband messages and
release the `scratch` buffer.

To ensure that this does not get broken again, we introduce a pair of
subcommands of the `pkt-line` test helper that specifically chop up the
sideband message and squeeze a primary packet into the middle.

Final note: The other test case touched by 2b695ecd74d (t5500: count
objects through stderr, not trace, 2020-05-06) is not affected by this
issue because the sideband machinery is not involved there.

Signed-off-by: Johannes Schindelin <[e-mail]>
---
 sideband.c               |  2 +-
 t/helper/test-pkt-line.c | 23 +++++++++++++++++++++++
 t/t0070-fundamental.sh   |  6 ++++++
 3 files changed, 30 insertions(+), 1 deletion(-)

Comments

Junio C Hamano Oct. 20, 2020, 8:33 p.m. UTC | #1

"Johannes Schindelin via GitGitGadget" <[e-mail]>
writes:

> From: Johannes Schindelin <[e-mail]>
>
> In 2b695ecd74d (t5500: count objects through stderr, not trace,
> 2020-05-06) we tried to ensure that the "Total 3" message could be
> grepped in Git's output, even if it sometimes got chopped up into
> multiple lines in the trace machinery.
> ...
> The correct way to fix this is to return from `demultiplex_sideband()`
> early. The caller will then write out the contents of the primary packet
> and continue looping. The `scratch` buffer for incomplete sideband
> messages is owned by that caller, and will continue to accumulate the
> remainder(s) of those messages. The loop will only end once
> `demultiplex_sideband()` returned non-zero _and_ did not indicate a
> primary packet, which is the case only when we hit the `cleanup:` path,
> in which we take care of flushing any unfinished sideband messages and
> release the `scratch` buffer.
>
> To ensure that this does not get broken again, we introduce a pair of
> subcommands of the `pkt-line` test helper that specifically chop up the
> sideband message and squeeze a primary packet into the middle.
>
> Final note: The other test case touched by 2b695ecd74d (t5500: count
> objects through stderr, not trace, 2020-05-06) is not affected by this
> issue because the sideband machinery is not involved there.
>
> Signed-off-by: Johannes Schindelin <[e-mail]>
> ---

Nicely explained.  Do we want to also give credit to Peff for the
single-liner fix here, perhaps with a suggested/helped-by trailer?

>  sideband.c               |  2 +-
>  t/helper/test-pkt-line.c | 23 +++++++++++++++++++++++
>  t/t0070-fundamental.sh   |  6 ++++++
>  3 files changed, 30 insertions(+), 1 deletion(-)
>
> diff --git a/sideband.c b/sideband.c
> index 0a60662fa6..a5405b9aaa 100644
> --- a/sideband.c
> +++ b/sideband.c
> @@ -190,7 +190,7 @@ int demultiplex_sideband(const char *me, char *buf, int len,
>  		return 0;
>  	case 1:
>  		*sideband_type = SIDEBAND_PRIMARY;
> -		break;
> +		return 1;
>  	default:
>  		strbuf_addf(scratch, "%s%s: protocol error: bad band #%d",
>  			    scratch->len ? "\n" : "", me, band);

> diff --git a/t/helper/test-pkt-line.c b/t/helper/test-pkt-line.c
> index 69152958e5..0bf20642be 100644
> --- a/t/helper/test-pkt-line.c
> +++ b/t/helper/test-pkt-line.c
> @@ -84,6 +84,25 @@ static void unpack_sideband(void)
>  	}
>  }
>  
> +static int send_split_sideband(void)
> +{
> +	const char *part1 = "Hello,";
> +	const char *primary = "\001primary: regular output\n";
> +	const char *part2 = " world!\n";
> +
> +	send_sideband(1, 2, part1, strlen(part1), LARGE_PACKET_MAX);
> +	packet_write(1, primary, strlen(primary));
> +	send_sideband(1, 2, part2, strlen(part2), LARGE_PACKET_MAX);
> +	packet_response_end(1);
> +
> +	return 0;
> +}

OK.

> +static int receive_sideband(void)
> +{
> +	return recv_sideband("sideband: ", 0, 1);
> +}
> +
>  int cmd__pkt_line(int argc, const char **argv)
>  {
>  	if (argc < 2)
> @@ -95,6 +114,10 @@ int cmd__pkt_line(int argc, const char **argv)
>  		unpack();
>  	else if (!strcmp(argv[1], "unpack-sideband"))
>  		unpack_sideband();
> +	else if (!strcmp(argv[1], "send-split-sideband"))
> +		send_split_sideband();
> +	else if (!strcmp(argv[1], "receive-sideband"))
> +		receive_sideband();
>  	else
>  		die("invalid argument '%s'", argv[1]);
>  
> diff --git a/t/t0070-fundamental.sh b/t/t0070-fundamental.sh
> index 7b111a56fd..357201640a 100755
> --- a/t/t0070-fundamental.sh
> +++ b/t/t0070-fundamental.sh
> @@ -34,4 +34,10 @@ test_expect_success 'check for a bug in the regex routines' '
>  	test-tool regex --bug
>  '
>  
> +test_expect_success 'incomplete sideband messages are reassembled' '
> +	test-tool pkt-line send-split-sideband >split-sideband &&
> +	test-tool pkt-line receive-sideband <split-sideband 2>err &&
> +	grep "Hello, world" err
> +'
> +
>  test_done

Johannes Schindelin Oct. 20, 2020, 8:48 p.m. UTC | #2

Hi Junio,

On Tue, 20 Oct 2020, Junio C Hamano wrote:

> "Johannes Schindelin via GitGitGadget" <[e-mail]>
> writes:
>
> > From: Johannes Schindelin <[e-mail]>
> >
> > In 2b695ecd74d (t5500: count objects through stderr, not trace,
> > 2020-05-06) we tried to ensure that the "Total 3" message could be
> > grepped in Git's output, even if it sometimes got chopped up into
> > multiple lines in the trace machinery.
> > ...
> > The correct way to fix this is to return from `demultiplex_sideband()`
> > early. The caller will then write out the contents of the primary packet
> > and continue looping. The `scratch` buffer for incomplete sideband
> > messages is owned by that caller, and will continue to accumulate the
> > remainder(s) of those messages. The loop will only end once
> > `demultiplex_sideband()` returned non-zero _and_ did not indicate a
> > primary packet, which is the case only when we hit the `cleanup:` path,
> > in which we take care of flushing any unfinished sideband messages and
> > release the `scratch` buffer.
> >
> > To ensure that this does not get broken again, we introduce a pair of
> > subcommands of the `pkt-line` test helper that specifically chop up the
> > sideband message and squeeze a primary packet into the middle.
> >
> > Final note: The other test case touched by 2b695ecd74d (t5500: count
> > objects through stderr, not trace, 2020-05-06) is not affected by this
> > issue because the sideband machinery is not involved there.
> >
> > Signed-off-by: Johannes Schindelin <[e-mail]>
> > ---
>
> Nicely explained.  Do we want to also give credit to Peff for the
> single-liner fix here, perhaps with a suggested/helped-by trailer?

Sure.

I had not added that because I had actually come up with the fix
independently in my analysis before I read Peff's reply thoroughly.

But credit where credit is due: without Peff's reply, I would not have
worked on the full fix and stayed with the work-around.

Ciao,
Dscho

Junio C Hamano Oct. 20, 2020, 9:33 p.m. UTC | #3

Johannes Schindelin <[e-mail]> writes:

>> Nicely explained.  Do we want to also give credit to Peff for the
>> single-liner fix here, perhaps with a suggested/helped-by trailer?
>
> Sure.
>
> I had not added that because I had actually come up with the fix
> independently in my analysis before I read Peff's reply thoroughly.

Ah, of course, e-mails cross all the time so these things happen.
Thanks, both.

Patch

diff --git a/sideband.c b/sideband.c
index 0a60662fa6..a5405b9aaa 100644
--- a/sideband.c
+++ b/sideband.c
@@ -190,7 +190,7 @@  int demultiplex_sideband(const char *me, char *buf, int len,
 		return 0;
 	case 1:
 		*sideband_type = SIDEBAND_PRIMARY;
-		break;
+		return 1;
 	default:
 		strbuf_addf(scratch, "%s%s: protocol error: bad band #%d",
 			    scratch->len ? "\n" : "", me, band);
diff --git a/t/helper/test-pkt-line.c b/t/helper/test-pkt-line.c
index 69152958e5..0bf20642be 100644
--- a/t/helper/test-pkt-line.c
+++ b/t/helper/test-pkt-line.c
@@ -84,6 +84,25 @@  static void unpack_sideband(void)
 	}
 }
 
+static int send_split_sideband(void)
+{
+	const char *part1 = "Hello,";
+	const char *primary = "\001primary: regular output\n";
+	const char *part2 = " world!\n";
+
+	send_sideband(1, 2, part1, strlen(part1), LARGE_PACKET_MAX);
+	packet_write(1, primary, strlen(primary));
+	send_sideband(1, 2, part2, strlen(part2), LARGE_PACKET_MAX);
+	packet_response_end(1);
+
+	return 0;
+}
+
+static int receive_sideband(void)
+{
+	return recv_sideband("sideband: ", 0, 1);
+}
+
 int cmd__pkt_line(int argc, const char **argv)
 {
 	if (argc < 2)
@@ -95,6 +114,10 @@  int cmd__pkt_line(int argc, const char **argv)
 		unpack();
 	else if (!strcmp(argv[1], "unpack-sideband"))
 		unpack_sideband();
+	else if (!strcmp(argv[1], "send-split-sideband"))
+		send_split_sideband();
+	else if (!strcmp(argv[1], "receive-sideband"))
+		receive_sideband();
 	else
 		die("invalid argument '%s'", argv[1]);
 
diff --git a/t/t0070-fundamental.sh b/t/t0070-fundamental.sh
index 7b111a56fd..357201640a 100755
--- a/t/t0070-fundamental.sh
+++ b/t/t0070-fundamental.sh
@@ -34,4 +34,10 @@  test_expect_success 'check for a bug in the regex routines' '
 	test-tool regex --bug
 '
 
+test_expect_success 'incomplete sideband messages are reassembled' '
+	test-tool pkt-line send-split-sideband >split-sideband &&
+	test-tool pkt-line receive-sideband <split-sideband 2>err &&
+	grep "Hello, world" err
+'
+
 test_done