From 1074a9be4ce66ce4ca05d6698e563c42c62b5f10 Mon Sep 17 00:00:00 2001 From: Simon Tatham Date: Wed, 28 Nov 2018 20:16:41 +0000 Subject: [PATCH] Stop BPPs from handling EOF before reading all data. The BPP_READ macros in all four BPP implementations (including sshverstring) had the same bug: if EOF had been seen on the network input but there was _also_ enough data in the input queue to satisfy the current request, they would jump straight to complaining about the EOF rather than processing the available data first. I spotted this while trying to pipe in test data from a disk file, but it could easily also lead to us failing to handle the final message in the connection, e.g. losing the error message sent by the remote in a DISCONNECT message. --- ssh1bpp.c | 15 ++++++++------- ssh2bpp-bare.c | 15 ++++++++------- ssh2bpp.c | 15 ++++++++------- sshverstring.c | 15 ++++++++------- 4 files changed, 32 insertions(+), 28 deletions(-) diff --git a/ssh1bpp.c b/ssh1bpp.c index 2938e405..96f71aee 100644 --- a/ssh1bpp.c +++ b/ssh1bpp.c @@ -104,13 +104,14 @@ void ssh1_bpp_start_compression(BinaryPacketProtocol *bpp) bpp_logevent(("Started zlib (RFC1950) compression")); } -#define BPP_READ(ptr, len) do \ - { \ - crMaybeWaitUntilV(s->bpp.input_eof || \ - bufchain_try_fetch_consume( \ - s->bpp.in_raw, ptr, len)); \ - if (s->bpp.input_eof) \ - goto eof; \ +#define BPP_READ(ptr, len) do \ + { \ + bool success; \ + crMaybeWaitUntilV((success = bufchain_try_fetch_consume( \ + s->bpp.in_raw, ptr, len)) || \ + s->bpp.input_eof); \ + if (!success) \ + goto eof; \ } while (0) static void ssh1_bpp_handle_input(BinaryPacketProtocol *bpp) diff --git a/ssh2bpp-bare.c b/ssh2bpp-bare.c index cabb2cd1..8db2f7f7 100644 --- a/ssh2bpp-bare.c +++ b/ssh2bpp-bare.c @@ -51,13 +51,14 @@ static void ssh2_bare_bpp_free(BinaryPacketProtocol *bpp) sfree(s); } -#define BPP_READ(ptr, len) do \ - { \ - crMaybeWaitUntilV(s->bpp.input_eof || \ - bufchain_try_fetch_consume( \ - s->bpp.in_raw, ptr, len)); \ - if (s->bpp.input_eof) \ - goto eof; \ +#define BPP_READ(ptr, len) do \ + { \ + bool success; \ + crMaybeWaitUntilV((success = bufchain_try_fetch_consume( \ + s->bpp.in_raw, ptr, len)) || \ + s->bpp.input_eof); \ + if (!success) \ + goto eof; \ } while (0) static void ssh2_bare_bpp_handle_input(BinaryPacketProtocol *bpp) diff --git a/ssh2bpp.c b/ssh2bpp.c index 4d117878..b1d454c3 100644 --- a/ssh2bpp.c +++ b/ssh2bpp.c @@ -251,13 +251,14 @@ static void ssh2_bpp_enable_pending_compression(struct ssh2_bpp_state *s) } } -#define BPP_READ(ptr, len) do \ - { \ - crMaybeWaitUntilV(s->bpp.input_eof || \ - bufchain_try_fetch_consume( \ - s->bpp.in_raw, ptr, len)); \ - if (s->bpp.input_eof) \ - goto eof; \ +#define BPP_READ(ptr, len) do \ + { \ + bool success; \ + crMaybeWaitUntilV((success = bufchain_try_fetch_consume( \ + s->bpp.in_raw, ptr, len)) || \ + s->bpp.input_eof); \ + if (!success) \ + goto eof; \ } while (0) #define userauth_range(pkttype) ((unsigned)((pkttype) - 50) < 20) diff --git a/sshverstring.c b/sshverstring.c index 529b5964..438be8ef 100644 --- a/sshverstring.c +++ b/sshverstring.c @@ -205,12 +205,13 @@ static void ssh_verstring_send(struct ssh_verstring_state *s) } #define BPP_WAITFOR(minlen) do \ - { \ - crMaybeWaitUntilV( \ - s->bpp.input_eof || \ - bufchain_size(s->bpp.in_raw) >= (minlen)); \ - if (s->bpp.input_eof) \ - goto eof; \ + { \ + bool success; \ + crMaybeWaitUntilV( \ + (success = (bufchain_size(s->bpp.in_raw) >= (minlen))) || \ + s->bpp.input_eof); \ + if (!success) \ + goto eof; \ } while (0) void ssh_verstring_handle_input(BinaryPacketProtocol *bpp) @@ -284,7 +285,7 @@ void ssh_verstring_handle_input(BinaryPacketProtocol *bpp) void *data; char *nl; - crMaybeWaitUntilV(bufchain_size(s->bpp.in_raw) > 0); + BPP_WAITFOR(1); bufchain_prefix(s->bpp.in_raw, &data, &len); if ((nl = memchr(data, '\012', len)) != NULL) { len = nl - (char *)data + 1;