Re: [PATCH v2 3/3] sideband: add defense against packets missing a band designator
- From
Johannes Schindelin <johannes.schindelin@gmx.de>
- Date
- Oct 23, 2020, 05:36 UTC
- Message-ID
- <nycvar.QRO.7.76.6.2010230732250.56@tvgsbejvaqbjf.bet>
- In-Reply-To
- <20201023084827.GE4012156@coredump.intra.peff.net>
Hi Peff,
On Fri, 23 Oct 2020, Jeff King wrote:
Show 21 quoted lines
> 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
Show 61 quoted lines
> 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--;
>