Git fork

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>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

authored by

Jeff King and committed by
Junio C Hamano
af22a63c 712b0377

+47 -14
+8 -6
pkt-line.c
··· 461 enum sideband_type sideband_type; 462 463 while (1) { 464 - len = packet_read(in_stream, NULL, NULL, buf, LARGE_PACKET_MAX, 465 - 0); 466 - if (!demultiplex_sideband(me, buf, len, 0, &scratch, 467 &sideband_type)) 468 continue; 469 switch (sideband_type) { ··· 520 reader->options); 521 if (!reader->use_sideband) 522 break; 523 - if (demultiplex_sideband(reader->me, reader->buffer, 524 - reader->pktlen, 1, &scratch, 525 - &sideband_type)) 526 break; 527 } 528
··· 461 enum sideband_type sideband_type; 462 463 while (1) { 464 + int status = packet_read_with_status(in_stream, NULL, NULL, 465 + buf, LARGE_PACKET_MAX, 466 + &len, 467 + PACKET_READ_GENTLE_ON_EOF); 468 + if (!demultiplex_sideband(me, status, buf, len, 0, &scratch, 469 &sideband_type)) 470 continue; 471 switch (sideband_type) { ··· 522 reader->options); 523 if (!reader->use_sideband) 524 break; 525 + if (demultiplex_sideband(reader->me, reader->status, 526 + reader->buffer, reader->pktlen, 1, 527 + &scratch, &sideband_type)) 528 break; 529 } 530
+22 -7
sideband.c
··· 3 #include "config.h" 4 #include "sideband.h" 5 #include "help.h" 6 7 struct keyword_entry { 8 /* ··· 114 #define ANSI_SUFFIX "\033[K" 115 #define DUMB_SUFFIX " " 116 117 - int demultiplex_sideband(const char *me, char *buf, int len, 118 int die_on_error, 119 struct strbuf *scratch, 120 enum sideband_type *sideband_type) ··· 130 suffix = DUMB_SUFFIX; 131 } 132 133 - if (len == 0) { 134 - *sideband_type = SIDEBAND_FLUSH; 135 - goto cleanup; 136 - } 137 - if (len < 1) { 138 strbuf_addf(scratch, 139 - "%s%s: protocol error: no band designator", 140 scratch->len ? "\n" : "", me); 141 *sideband_type = SIDEBAND_PROTOCOL_ERROR; 142 goto cleanup; 143 } 144 band = buf[0] & 0xff; 145 buf[len] = '\0'; 146 len--;
··· 3 #include "config.h" 4 #include "sideband.h" 5 #include "help.h" 6 + #include "pkt-line.h" 7 8 struct keyword_entry { 9 /* ··· 115 #define ANSI_SUFFIX "\033[K" 116 #define DUMB_SUFFIX " " 117 118 + int demultiplex_sideband(const char *me, int status, 119 + char *buf, int len, 120 int die_on_error, 121 struct strbuf *scratch, 122 enum sideband_type *sideband_type) ··· 132 suffix = DUMB_SUFFIX; 133 } 134 135 + if (status == PACKET_READ_EOF) { 136 strbuf_addf(scratch, 137 + "%s%s: unexpected disconnect while reading sideband packet", 138 scratch->len ? "\n" : "", me); 139 *sideband_type = SIDEBAND_PROTOCOL_ERROR; 140 goto cleanup; 141 } 142 + 143 + if (len < 0) 144 + BUG("negative length on non-eof packet read"); 145 + 146 + if (len == 0) { 147 + if (status == PACKET_READ_NORMAL) { 148 + strbuf_addf(scratch, 149 + "%s%s: protocol error: missing sideband designator", 150 + scratch->len ? "\n" : "", me); 151 + *sideband_type = SIDEBAND_PROTOCOL_ERROR; 152 + } else { 153 + /* covers flush, delim, etc */ 154 + *sideband_type = SIDEBAND_FLUSH; 155 + } 156 + goto cleanup; 157 + } 158 + 159 band = buf[0] & 0xff; 160 buf[len] = '\0'; 161 len--;
+5 -1
sideband.h
··· 18 * 19 * scratch must be a struct strbuf allocated by the caller. It is used to store 20 * progress messages split across multiple packets. 21 */ 22 - int demultiplex_sideband(const char *me, char *buf, int len, 23 int die_on_error, 24 struct strbuf *scratch, 25 enum sideband_type *sideband_type);
··· 18 * 19 * scratch must be a struct strbuf allocated by the caller. It is used to store 20 * progress messages split across multiple packets. 21 + * 22 + * The "status" parameter is a pkt-line response as returned by 23 + * packet_read_with_status() (e.g., PACKET_READ_NORMAL). 24 */ 25 + int demultiplex_sideband(const char *me, int status, 26 + char *buf, int len, 27 int die_on_error, 28 struct strbuf *scratch, 29 enum sideband_type *sideband_type);
+12
t/t0070-fundamental.sh
··· 40 grep "Hello, world" err 41 ' 42 43 test_done
··· 40 grep "Hello, world" err 41 ' 42 43 + test_expect_success 'eof on sideband message is reported' ' 44 + printf 1234 >input && 45 + test-tool pkt-line receive-sideband <input 2>err && 46 + test_i18ngrep "unexpected disconnect" err 47 + ' 48 + 49 + test_expect_success 'missing sideband designator is reported' ' 50 + printf 0004 >input && 51 + test-tool pkt-line receive-sideband <input 2>err && 52 + test_i18ngrep "missing sideband" err 53 + ' 54 + 55 test_done