Re: [PATCH v2 3/3] sideband: add defense against packets missing a band designator

[Jeff King <peff@xxxxxxxx>]

On Mon, Oct 19, 2020 at 07:35:42PM +0000, Johannes Schindelin via GitGitGadget wrote:

> diff --git a/pkt-line.c b/pkt-line.c
> index 657a702927..f72048f623 100644
> --- a/pkt-line.c
> +++ b/pkt-line.c
> @@ -461,8 +461,10 @@ 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);
> +		int status = packet_read_with_status(in_stream, NULL, NULL, buf,
> +						     LARGE_PACKET_MAX, &len, 0);
> +		if (!len && status == PACKET_READ_NORMAL)
> +			BUG("missing band designator");
>  		if (!demultiplex_sideband(me, buf, len, 0, &scratch,
>  					  &sideband_type))

I also wonder if this status-check should be pushed down into
demultiplex_sideband() by passing "status", for two reasons:

  1. So we don't have to repeat it (though it isn't that big)

  2. The other half of this weirdness is that if we get an early EOF,
     we'll hit the "missing sideband designator" die() message. But
     that's not really what happened; we probably got a network hangup.
     And we could distinguish that case by checking for status ==
     PACKET_READ_EOF and provide a better message.

Something like this (completely untested):

diff --git a/sideband.c b/sideband.c
index 0a60662fa6..6ad15ed581 100644
--- a/sideband.c
+++ b/sideband.c
@@ -115,6 +115,7 @@ static void maybe_colorize_sideband(struct strbuf *dest, const char *src, int n)
 #define DUMB_SUFFIX "        "
 
 int demultiplex_sideband(const char *me, char *buf, int len,
+			 enum packet_read_status status,
 			 int die_on_error,
 			 struct strbuf *scratch,
 			 enum sideband_type *sideband_type)
@@ -130,17 +131,29 @@ 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: protocol error: eof while reading 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: no band designator",
+				    scratch->len ? "\n" : "", me);
+			*sideband_type = SIDEBAND_PROTOCOL_ERROR;
+		} else {
+			*sideband_type = SIDEBAND_FLUSH;
+		}
+		goto cleanup;
+	}
+
 	band = buf[0] & 0xff;
 	buf[len] = '\0';
 	len--;