git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH] sideband: diagnose more incoming packet anomalies

From
Jeff King <peff@peff.net>
Date
Oct 28, 2020, 09:33 UTC
Message-ID
<20201028093324.GA175617@coredump.intra.peff.net>
In-Reply-To
<xmqqzh479wor.fsf@gitster.c.googlers.com>
On Tue, Oct 27, 2020 at 02:38:12PM -0700, Junio C Hamano wrote:
Show 5 quoted lines
> Jeff King <peff@peff.net> writes:
> 
> > Does this patch seem otherwise worth doing?
> 
> Yeah.

OK, here it is, then with that one error message reworded. I dropped "protocol error" from that one message, as well. One, because it was getting quite long. And two, it's probably _not_ a protocol error, but rather a network drop. So just describing what we see on the client side is the best strategy.

Show 5 quoted lines
> FWIW, I do not find the "dependency" thing disturbing.  sideband is
> an extension of the pkt-line mechansim, so it is natural that it
> depends on pkt-line.  I'd also be happy if enums, structures and
> calls defined in both headers are made available by just including
> one of them (e.g. retire sideband.h, perhaps).

I agree with that. Given how easy it was to work around with an int, I'm not inclined to spend time going further at this point, but it's something to keep in mind.

Show 6 quoted lines
> > An alternate patch would be to keep the behavior the same and just
> > clarify the code a bit. Something like:
> 
> This also looks OK to me from readability's point of view, but it
> does not as much help the end user who is puzzled as the real thing,
> I am afraid.

I suspect these cases are rare enough that nobody minds either way. But I do think the behavior-changing patch is making us more likely to do the right thing. The downside is the risk of some unintended regression, but I feel pretty good about it.

-- >8 --
Subject: [PATCH] sideband: diagnose more sideband anomalies

In demultiplex_sideband(), there are two oddities when we check an incoming packet:

  - if it has zero length, then we assume it's a flush packet. This
    means we fail to notice the difference between a real flush and a
    true zero-length packet that's missing its sideband designator. It's
    not a huge problem in practice because we'd never send a zero-length
    data packet (even our keepalives are otherwise-empty sideband-1
    packets).
    But it would be nice to detect and report the error, since it's
    likely to cause other confusion (we think the other side flushed,
    but they do not).
  - we try to detect packets missing their designator by checking for
    "if (len < 1)". But this will never trigger for "len == 0"; we've
    already detected that and left the function before then.
    It _could_ detect a negative "len" parameter. But in that case, the
    error message is wrong. The issue is not "no sideband" but rather
    "eof while reading the packet". However, this can't actually be
    triggered in practice, because neither of the two callers uses
    pkt_read's GENTLE_ON_EOF flag. Which means they'd die with "the
    remote end hung up unexpectedly" before we even get here.
    So this truly is dead code.

We can improve these cases by passing in a pkt-line status to the demultiplexer, and by having recv_sideband() use GENTLE_ON_EOF. This gives us two improvements:

  - we can now reliably detect flush packets, and will report a normal
    packet missing its sideband designator as an error
  - we'll report an eof with a more detailed "protocol error: eof while
    reading sideband packet", rather than the generic "the remote end
    hung up unexpectedly"
  - when we see an eof, we'll flush the sideband scratch buffer, which
    may provide some hints from the remote about why they hung up
    (though note we already flush on newlines, so it's likely that most
    such messages already made it through)

In some sense this patch goes against fbd76cd450 (sideband: reverse its dependency on pkt-line, 2019-01-16), which caused the sideband code not to depend on the pkt-line code. But that commit was really just trying to deal with the circular header dependency. The two modules are conceptually interlinked, and it was just trying to keep things compiling. And indeed, there's a sticking point in this patch: because pkt-line.h includes sideband.h, we can't add the reverse include we need for the sideband code to have an "enum packet_read_status" parameter. Nor can we forward declare it, because you can't forward declare an enum in C. However, C does guarantee that enums fit in an int, so we can just use that type.

One alternative would be for the callers to check themselves that they got something sane from the pkt-line code. But besides duplicating logic, this gets quite tricky. Any error condition requires flushing the sideband #2 scratch buffer, which only demultiplex_sideband() knows how to do.

Signed-off-by: Jeff King <peff@peff.net>
---
 pkt-line.c             | 14 ++++++++------
 sideband.c             | 29 ++++++++++++++++++++++-------
 sideband.h             |  6 +++++-
 t/t0070-fundamental.sh | 12 ++++++++++++
 4 files changed, 47 insertions(+), 14 deletions(-)
diff --git a/pkt-line.c b/pkt-line.c
index 657a702927..d633005ef7 100644
--- a/pkt-line.c
+++ b/pkt-line.c
@@ -461,9 +461,11 @@ int recv_sideband(const char *me, int in_stream, int out)
 	enum sideband_type sideband_type;
 
 	while (1) {
-		len = packet_read(in_stream, NULL, NULL, buf, LARGE_PACKET_MAX,
-				  0);
-		if (!demultiplex_sideband(me, buf, len, 0, &scratch,
+		int status = packet_read_with_status(in_stream, NULL, NULL,
+						     buf, LARGE_PACKET_MAX,
+						     &len,
+						     PACKET_READ_GENTLE_ON_EOF);
+		if (!demultiplex_sideband(me, status, buf, len, 0, &scratch,
 					  &sideband_type))
 			continue;
 		switch (sideband_type) {
@@ -520,9 +522,9 @@ enum packet_read_status packet_reader_read(struct packet_reader *reader)
 							 reader->options);
 		if (!reader->use_sideband)
 			break;
-		if (demultiplex_sideband(reader->me, reader->buffer,
-					 reader->pktlen, 1, &scratch,
-					 &sideband_type))
+		if (demultiplex_sideband(reader->me, reader->status,
+					 reader->buffer, reader->pktlen, 1,
+					 &scratch, &sideband_type))
 			break;
 	}
 
diff --git a/sideband.c b/sideband.c
index a5405b9aaa..6f9e026732 100644
--- a/sideband.c
+++ b/sideband.c
@@ -3,6 +3,7 @@
 #include "config.h"
 #include "sideband.h"
 #include "help.h"
+#include "pkt-line.h"
 
 struct keyword_entry {
 	/*
@@ -114,7 +115,8 @@ static void maybe_colorize_sideband(struct strbuf *dest, const char *src, int n)
 #define ANSI_SUFFIX "\033[K"
 #define DUMB_SUFFIX "        "
 
-int demultiplex_sideband(const char *me, char *buf, int len,
+int demultiplex_sideband(const char *me, int status,
+			 char *buf, int len,
 			 int die_on_error,
 			 struct strbuf *scratch,
 			 enum sideband_type *sideband_type)
@@ -130,17 +132,30 @@ int demultiplex_sideband(const char *me, char *buf, int len,
 			suffix = DUMB_SUFFIX;
 	}
 
-	if (len == 0) {
-		*sideband_type = SIDEBAND_FLUSH;
-		goto cleanup;
-	}
-	if (len < 1) {
+	if (status == PACKET_READ_EOF) {
 		strbuf_addf(scratch,
-			    "%s%s: protocol error: no band designator",
+			    "%s%s: unexpected disconnect while reading sideband packet",
 			    scratch->len ? "\n" : "", me);
 		*sideband_type = SIDEBAND_PROTOCOL_ERROR;
 		goto cleanup;
 	}
+
+	if (len < 0)
+		BUG("negative length on non-eof packet read");
+
+	if (len == 0) {
+		if (status == PACKET_READ_NORMAL) {
+			strbuf_addf(scratch,
+				    "%s%s: protocol error: missing sideband designator",
+				    scratch->len ? "\n" : "", me);
+			*sideband_type = SIDEBAND_PROTOCOL_ERROR;
+		} else {
+			/* covers flush, delim, etc */
+			*sideband_type = SIDEBAND_FLUSH;
+		}
+		goto cleanup;
+	}
+
 	band = buf[0] & 0xff;
 	buf[len] = '\0';
 	len--;
diff --git a/sideband.h b/sideband.h
index 227740a58e..5a25331be5 100644
--- a/sideband.h
+++ b/sideband.h
@@ -18,8 +18,12 @@ enum sideband_type {
  *
  * scratch must be a struct strbuf allocated by the caller. It is used to store
  * progress messages split across multiple packets.
+ *
+ * The "status" parameter is a pkt-line response as returned by
+ * packet_read_with_status() (e.g., PACKET_READ_NORMAL).
  */
-int demultiplex_sideband(const char *me, char *buf, int len,
+int demultiplex_sideband(const char *me, int status,
+			 char *buf, int len,
 			 int die_on_error,
 			 struct strbuf *scratch,
 			 enum sideband_type *sideband_type);
diff --git a/t/t0070-fundamental.sh b/t/t0070-fundamental.sh
index 357201640a..8d59905ef0 100755
--- a/t/t0070-fundamental.sh
+++ b/t/t0070-fundamental.sh
@@ -40,4 +40,16 @@ test_expect_success 'incomplete sideband messages are reassembled' '
 	grep "Hello, world" err
 '
 
+test_expect_success 'eof on sideband message is reported' '
+	printf 1234 >input &&
+	test-tool pkt-line receive-sideband <input 2>err &&
+	test_i18ngrep "unexpected disconnect" err
+'
+
+test_expect_success 'missing sideband designator is reported' '
+	printf 0004 >input &&
+	test-tool pkt-line receive-sideband <input 2>err &&
+	test_i18ngrep "missing sideband" err
+'
+
 test_done

-- 
2.29.1.641.gb6287e56d2
Previous: Junio C HamanoNext: Jeff King
Message 31 of 33 in “t5500.43: make the check a bit more robust”
  1. t5500.43: make the check a bit more robustJohannes Schindelin via GitGitGadget, Oct 13, 2020
  2. Junio C HamanoOct 13, 2020
  3. Jeff KingOct 13, 2020
  4. Junio C HamanoOct 13, 2020
  5. Jeff KingOct 13, 2020
  6. Johannes SchindelinOct 17, 2020
  7. Johannes SchindelinOct 17, 2020
  8. 0/3 Work around flakiness in t5500.43Johannes Schindelin via GitGitGadget, Oct 19, 2020
  9. 1/3 sideband: avoid reporting incomplete sideband messagesJohannes Schindelin via GitGitGadget, Oct 19, 2020
  10. Junio C HamanoOct 20, 2020
  11. Johannes SchindelinOct 20, 2020
  12. Junio C HamanoOct 20, 2020
  13. 2/3 sideband: report unhandled incomplete sideband messages as bugsJohannes Schindelin via GitGitGadget, Oct 19, 2020
  14. 3/3 sideband: add defense against packets missing a band designatorJohannes Schindelin via GitGitGadget, Oct 19, 2020
  15. Junio C HamanoOct 20, 2020
  16. Jeff KingOct 23, 2020
  17. Junio C HamanoOct 23, 2020
  18. Jeff KingOct 23, 2020
  19. Johannes SchindelinOct 23, 2020
  20. Jeff KingOct 23, 2020
  21. Johannes SchindelinOct 26, 2020
  22. 0/2 Work around flakiness in t5500.43Johannes Schindelin via GitGitGadget, Oct 26, 2020
  23. 1/2 sideband: avoid reporting incomplete sideband messagesJohannes Schindelin via GitGitGadget, Oct 26, 2020
  24. 2/2 sideband: report unhandled incomplete sideband messages as bugsJohannes Schindelin via GitGitGadget, Oct 26, 2020
  25. Junio C HamanoOct 26, 2020
  26. sideband: diagnose more incoming packet anomaliesJeff King, Oct 27, 2020
  27. Jeff KingOct 27, 2020
  28. Junio C HamanoOct 27, 2020
  29. Jeff KingOct 27, 2020
  30. Junio C HamanoOct 27, 2020
  31. Jeff KingOct 28, 2020
  32. Jeff KingOct 27, 2020
  33. Junio C HamanoOct 27, 2020

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.