Account for packet queues in ssh_sendbuffer().

Ever since I reworked the SSH code to have multiple internal packet
queues, there's been a long-standing FIXME in ssh_sendbuffer() saying
that we ought to include the data buffered in those queues as part of
reporting how much data is buffered on standard input.

Recently a user reported that 'proftpd', or rather its 'mod_sftp'
add-on that implements an SFTP-only SSH server, exposes a bug related
to that missing piece of code. The xfer_upload system in sftp.c starts
by pushing SFTP write messages into the SSH code for as long as
sftp_sendbuffer() (which ends up at ssh_sendbuffer()) reports that not
too much data is buffered locally. In fact what happens is that all
those messages end up on the packet queues between SSH protocol
layers, so they're not counted by sftp_sendbuffer(), so we just keep
going until there's some other reason to stop.

Usually the reason we stop is because we've filled up the SFTP
channel's SSH-layer window, so we need the server to send us a
WINDOW_ADJUST before we're allowed to send any more data. So we return
to the main event loop and start waiting for reply packets. And when
the window is moderate (e.g. OpenSSH currently seems to present about
2MB), this isn't really noticeable.

But proftpd presents the maximum-size window of 2^32-1 bytes, and as a
result we just keep shovelling more and more packets into the internal
packet queues until PSFTP has grown to 4GB in size, and only then do
we even return to the event loop and start actually sending them down
the network. Moreover, this happens again at rekey time, because while
a rekey is in progress, ssh2transport stops emptying the queue of
outgoing packets sent by its higher layer - so, again, everything just
keeps buffering up somewhere that sftp_sendbuffer can't see it.

But this commit fixes it! Each PacketProtocolLayer now provides a
vtable method for asking how much data it currently has queued. Most
of them share a default implementation which just returns the newly
added total_size field from their pq_out; the exception is
ssh2transport, which also has to account for data queued in its higher
layer. And ssh_sendbuffer() adds that on to the quantity it already
knew about in other locations, to give a more realistic idea of the
currently buffered data.
This commit is contained in:
Simon Tatham 2020-02-05 19:34:29 +00:00
Родитель 0ff13ae773
Коммит cd97b7e7ea
10 изменённых файлов: 32 добавлений и 1 удалений

3
ssh.c
Просмотреть файл

@ -997,7 +997,8 @@ static size_t ssh_sendbuffer(Backend *be)
backlog = ssh_stdin_backlog(ssh->cl);
/* FIXME: also include sizes of pqs */
if (ssh->base_layer)
backlog += ssh_ppl_queued_data_size(ssh->base_layer);
/*
* If the SSH socket itself has backed up, add the total backup

Просмотреть файл

@ -43,6 +43,7 @@ static const struct PacketProtocolLayerVtable ssh1_connection_vtable = {
ssh1_connection_want_user_input,
ssh1_connection_got_user_input,
ssh1_connection_reconfigure,
ssh_ppl_default_queued_data_size,
NULL /* no layer names in SSH-1 */,
};

Просмотреть файл

@ -65,6 +65,7 @@ static const struct PacketProtocolLayerVtable ssh1_login_server_vtable = {
ssh1_login_server_want_user_input,
ssh1_login_server_got_user_input,
ssh1_login_server_reconfigure,
ssh_ppl_default_queued_data_size,
NULL /* no layer names in SSH-1 */,
};

Просмотреть файл

@ -80,6 +80,7 @@ static const struct PacketProtocolLayerVtable ssh1_login_vtable = {
ssh1_login_want_user_input,
ssh1_login_got_user_input,
ssh1_login_reconfigure,
ssh_ppl_default_queued_data_size,
NULL /* no layer names in SSH-1 */,
};

Просмотреть файл

@ -30,6 +30,7 @@ static const struct PacketProtocolLayerVtable ssh2_connection_vtable = {
ssh2_connection_want_user_input,
ssh2_connection_got_user_input,
ssh2_connection_reconfigure,
ssh_ppl_default_queued_data_size,
"ssh-connection",
};

Просмотреть файл

@ -71,6 +71,7 @@ static void ssh2_transport_special_cmd(PacketProtocolLayer *ppl,
static bool ssh2_transport_want_user_input(PacketProtocolLayer *ppl);
static void ssh2_transport_got_user_input(PacketProtocolLayer *ppl);
static void ssh2_transport_reconfigure(PacketProtocolLayer *ppl, Conf *conf);
static size_t ssh2_transport_queued_data_size(PacketProtocolLayer *ppl);
static void ssh2_transport_set_max_data_size(struct ssh2_transport_state *s);
static unsigned long sanitise_rekey_time(int rekey_time, unsigned long def);
@ -84,6 +85,7 @@ static const struct PacketProtocolLayerVtable ssh2_transport_vtable = {
ssh2_transport_want_user_input,
ssh2_transport_got_user_input,
ssh2_transport_reconfigure,
ssh2_transport_queued_data_size,
NULL, /* no protocol name for this layer */
};
@ -2028,3 +2030,12 @@ static int ssh2_transport_confirm_weak_crypto_primitive(
return seat_confirm_weak_crypto_primitive(
s->ppl.seat, type, name, ssh2_transport_dialog_callback, s);
}
static size_t ssh2_transport_queued_data_size(PacketProtocolLayer *ppl)
{
struct ssh2_transport_state *s =
container_of(ppl, struct ssh2_transport_state, ppl);
return (ssh_ppl_default_queued_data_size(ppl) +
ssh_ppl_queued_data_size(s->higher_layer));
}

Просмотреть файл

@ -46,6 +46,7 @@ static const struct PacketProtocolLayerVtable ssh2_userauth_server_vtable = {
NULL /* want_user_input */,
NULL /* got_user_input */,
NULL /* reconfigure */,
ssh_ppl_default_queued_data_size,
"ssh-userauth",
};

Просмотреть файл

@ -120,6 +120,7 @@ static const struct PacketProtocolLayerVtable ssh2_userauth_vtable = {
ssh2_userauth_want_user_input,
ssh2_userauth_got_user_input,
ssh2_userauth_reconfigure,
ssh_ppl_default_queued_data_size,
"ssh-userauth",
};

Просмотреть файл

@ -835,6 +835,11 @@ void ssh_ppl_user_output_string_and_free(PacketProtocolLayer *ppl, char *text)
sfree(text);
}
size_t ssh_ppl_default_queued_data_size(PacketProtocolLayer *ppl)
{
return ppl->out_pq->pqb.total_size;
}
/* ----------------------------------------------------------------------
* Common helper functions for clients and implementations of
* BinaryPacketProtocol.

Просмотреть файл

@ -19,6 +19,7 @@ struct PacketProtocolLayerVtable {
bool (*want_user_input)(PacketProtocolLayer *ppl);
void (*got_user_input)(PacketProtocolLayer *ppl);
void (*reconfigure)(PacketProtocolLayer *ppl, Conf *conf);
size_t (*queued_data_size)(PacketProtocolLayer *ppl);
/* Protocol-level name of this layer. */
const char *name;
@ -73,6 +74,8 @@ static inline void ssh_ppl_got_user_input(PacketProtocolLayer *ppl)
{ ppl->vt->got_user_input(ppl); }
static inline void ssh_ppl_reconfigure(PacketProtocolLayer *ppl, Conf *conf)
{ ppl->vt->reconfigure(ppl, conf); }
static inline size_t ssh_ppl_queued_data_size(PacketProtocolLayer *ppl)
{ return ppl->vt->queued_data_size(ppl); }
/* ssh_ppl_free is more than just a macro wrapper on the vtable; it
* does centralised parts of the freeing too. */
@ -90,6 +93,11 @@ void ssh_ppl_setup_queues(PacketProtocolLayer *ppl,
* avoid dereferencing itself on return from this function! */
void ssh_ppl_replace(PacketProtocolLayer *old, PacketProtocolLayer *new);
/* Default implementation of queued_data_size, which just adds up the
* sizes of all the packets in pq_out. A layer can override this if it
* has other things to take into account as well. */
size_t ssh_ppl_default_queued_data_size(PacketProtocolLayer *ppl);
PacketProtocolLayer *ssh1_login_new(
Conf *conf, const char *host, int port,
PacketProtocolLayer *successor_layer);