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

Commit Message

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

From: Johannes Schindelin <[e-mail]>

While there is currently no instance of code producing this type of
packet, if the `demultiplex_sideband()` would receive a packet whose
payload is not only empty but even misses the band designator, it would
mistake it for a flush packet.

Let's defend against such a bug in the future.

Note: `demultiplex_sideband()` will treat empty flush, delim, eof and
response-end packets all alike: it will treat them as flush packets.
Since empty packets by definition are missing a band designator, this is
the best that function can do. Therefore, it would make little sense to
pass the `status` on to `demultiplex_sideband()`. Besides, fbd76cd450
(sideband: reverse its dependency on pkt-line, 2019-01-16) went out of
its way to _stop_ the code inside `demultiplex_sideband()` from relying
on anything in `pkt-line.h`; that includes the data type `enum
packet_read_status` that we would need if we were to pass `status` as a
parameter to that function.

Based on a suggestion by Jeff King.

Signed-off-by: Johannes Schindelin <[e-mail]>
---
 pkt-line.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

Comments

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

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

> From: Johannes Schindelin <[e-mail]>
>
> While there is currently no instance of code producing this type of
> packet, if the `demultiplex_sideband()` would receive a packet whose
> payload is not only empty but even misses the band designator, it would
> mistake it for a flush packet.

Quite sensible.  Will queue all three.  Thanks.

Johannes Schindelin Oct. 23, 2020, 5:36 a.m. UTC | #2

Hi Peff,

On Fri, 23 Oct 2020, Jeff King wrote:

> 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",

I tried that, but as mentioned in the commit message, fbd76cd450
(sideband: reverse its dependency on pkt-line, 2019-01-16) went out of its
way to _stop_ the code inside `demultiplex_sideband()` from relying on
anything in `pkt-line.h`. And that `PACKET_READ_NORMAL` and
`PACKET_READ_EOF` _is_ from `pkt-line.h`.

Ciao,
Dscho

> 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--;
>

Jeff King Oct. 23, 2020, 8:34 a.m. UTC | #3

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

> From: Johannes Schindelin <[e-mail]>
> 
> While there is currently no instance of code producing this type of
> packet, if the `demultiplex_sideband()` would receive a packet whose
> payload is not only empty but even misses the band designator, it would
> mistake it for a flush packet.
> 
> Let's defend against such a bug in the future.

That seems reasonable, but I'm not sure if these ought to be BUG()s.
Isn't it an indication that the other side sent us bogus input? That
likely is a bug on the other end, but I think it should be a die(), just
as we would produce for any other malformed protocol input.

-Peff

Jeff King Oct. 23, 2020, 8:48 a.m. UTC | #4

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--;

Junio C Hamano Oct. 23, 2020, 2:44 p.m. UTC | #5

Jeff King <[e-mail]> writes:

> On Mon, Oct 19, 2020 at 07:35:42PM +0000, Johannes Schindelin via GitGitGadget wrote:
>
>> From: Johannes Schindelin <[e-mail]>
>> 
>> While there is currently no instance of code producing this type of
>> packet, if the `demultiplex_sideband()` would receive a packet whose
>> payload is not only empty but even misses the band designator, it would
>> mistake it for a flush packet.
>> 
>> Let's defend against such a bug in the future.
>
> That seems reasonable, but I'm not sure if these ought to be BUG()s.
> Isn't it an indication that the other side sent us bogus input? That
> likely is a bug on the other end, but I think it should be a die(), just
> as we would produce for any other malformed protocol input.

Thanks for spotting.  I also think this was a good change, but at
this point in the code we found a problem in the data the other side
created (i.e. we diagnosed a bug on the other side), which is a
usual input error, so it should not be a BUG().  

Would this be something we can warn and ignore if the connection is
still active, I wonder, though.

Patch

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))
 			continue;
@@ -520,6 +522,8 @@  enum packet_read_status packet_reader_read(struct packet_reader *reader)
 							 reader->options);
 		if (!reader->use_sideband)
 			break;
+		if (!reader->pktlen && reader->status == PACKET_READ_NORMAL)
+			BUG("missing band designator");
 		if (demultiplex_sideband(reader->me, reader->buffer,
 					 reader->pktlen, 1, &scratch,
 					 &sideband_type))