simon-git: putty (master): Simon Tatham

Wed Feb 5 19:58:51 GMT 2020

TL;DR:
  0ff13ae7 Track the total size of every PacketQueue.
  cd97b7e7 Account for packet queues in ssh_sendbuffer().
  bf0f323f Fix minor memory leak in psftp batch mode.

Repository:     https://git.tartarus.org/simon/putty.git
On the web:     https://git.tartarus.org/?p=simon/putty.git
Branch updated: master
Committer:      Simon Tatham <anakin at pobox.com>
Date:           2020-02-05 19:58:51

commit 0ff13ae77329a906d5ce5dbcf534a071eedd69f3
web diff https://git.tartarus.org/?p=simon/putty.git;a=commitdiff;h=0ff13ae77329a906d5ce5dbcf534a071eedd69f3;hp=563cb062b801fd447f5a5b04b9161890d8966e4a
Author: Simon Tatham <anakin at pobox.com>
Date:   Wed Feb 5 19:32:22 2020 +0000

    Track the total size of every PacketQueue.

    The queue-node structure shared between PktIn and PktOut now has a
    'formal_size' field, which is initialised appropriately by the various
    packet constructors. And the PacketQueue structure has a 'total_size'
    field which tracks the sum of the formal sizes of all the packets on
    the queue, and is automatically updated by the push, pop and
    concatenate functions.

    No functional change, and nothing uses the new fields yet: this is
    infrastructure that will be used in the next commit.

 ssh.h          |  2 ++
 ssh1bpp.c      |  1 +
 ssh2bpp-bare.c |  1 +
 ssh2bpp.c      |  1 +
 sshcommon.c    | 35 +++++++++++++++++++++++++++++++----
 5 files changed, 36 insertions(+), 4 deletions(-)

commit cd97b7e7ea37ee146a2797350f61d77f66dd3d8b
web diff https://git.tartarus.org/?p=simon/putty.git;a=commitdiff;h=cd97b7e7ea37ee146a2797350f61d77f66dd3d8b;hp=0ff13ae77329a906d5ce5dbcf534a071eedd69f3
Author: Simon Tatham <anakin at pobox.com>
Date:   Wed Feb 5 19:34:29 2020 +0000

    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.

 ssh.c                 |  3 ++-
 ssh1connection.c      |  1 +
 ssh1login-server.c    |  1 +
 ssh1login.c           |  1 +
 ssh2connection.c      |  1 +
 ssh2transport.c       | 11 +++++++++++
 ssh2userauth-server.c |  1 +
 ssh2userauth.c        |  1 +
 sshcommon.c           |  5 +++++
 sshppl.h              |  8 ++++++++
 10 files changed, 32 insertions(+), 1 deletion(-)

commit bf0f323fb47c34f392a09c3bc1359a82e8339959
web diff https://git.tartarus.org/?p=simon/putty.git;a=commitdiff;h=bf0f323fb47c34f392a09c3bc1359a82e8339959;hp=cd97b7e7ea37ee146a2797350f61d77f66dd3d8b
Author: Simon Tatham <anakin at pobox.com>
Date:   Wed Feb 5 19:45:27 2020 +0000

    Fix minor memory leak in psftp batch mode.

    Spotted by Leak Sanitiser, while I was investigating the PSFTP /
    proftpd issue mentioned in the previous commit (with ASan on as
    usual).

    The two very similar loops that read PSFTP commands from the
    interactive prompt and a batch file differed in one respect: only one
    of them remembered to free the command afterwards. Now I've moved the
    freeing code out into a subroutine that both loops can use.

 psftp.c | 19 ++++++++++++-------
 1 file changed, 12 insertions(+), 7 deletions(-)