simon-git: putty (main): Simon Tatham

Thu Sep 30 19:22:50 BST 2021

TL;DR:
  15419745 Windows dputs: use WriteFile to avoid stdio buffering.
  dde65900 handle_write_eof: delegate CloseHandle back to the client.
  6dfe941a Windows Pageant: fix hang due to queued callbacks.
  e7dd2421 Pageant: actually link requests on to their queues.

Repository:     https://git.tartarus.org/simon/putty.git
On the web:     https://git.tartarus.org/?p=simon/putty.git
Branch updated: main
Committer:      Simon Tatham <anakin at pobox.com>
Date:           2021-09-30 19:22:50

commit 1541974564a65b6edbf09447e8cf54355fe10805
web diff https://git.tartarus.org/?p=simon/putty.git;a=commitdiff;h=1541974564a65b6edbf09447e8cf54355fe10805;hp=a73aaf945728a15159a274193c6505a04111d2f8
Author: Simon Tatham <anakin at pobox.com>
Date:   Thu Sep 30 18:33:16 2021 +0100

    Windows dputs: use WriteFile to avoid stdio buffering.

    Trying to debug a problem involving threads just now, it turned out
    that the version of the diagnostics going to my debug.log was getting
    data in a different order from the version going to the debug console.
    Now I open and write to debug_fp by going directly to the Win32 API
    instead of via a buffering userland stdio, and that seems to have
    solved the problem.

 windows/utils/dputs.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

commit dde659004083b575547f2740aaa56fdf64523770
web diff https://git.tartarus.org/?p=simon/putty.git;a=commitdiff;h=dde659004083b575547f2740aaa56fdf64523770;hp=1541974564a65b6edbf09447e8cf54355fe10805
Author: Simon Tatham <anakin at pobox.com>
Date:   Thu Sep 30 19:16:20 2021 +0100

    handle_write_eof: delegate CloseHandle back to the client.

    When a writable HANDLE is managed by the handle-io.c system, you ask
    to send EOF on the handle by calling handle_write_eof. That waits
    until all buffered data has been written, and then sends an EOF event
    by simply closing the handle.

    That is, of course, the only way to send an EOF signal on a handle at
    all. And yet, it's a bug, because the handle_output system does not
    take ownership of the handle you give it: the client of handle_output
    retains ownership, keeps its own copy of the handle, and will expect
    to close it itself.

    In most cases, the extra close will harmlessly fail, and return
    ERROR_INVALID_HANDLE (which the caller didn't notice anyway). But if
    you're unlucky, in conditions of frantic handle opening and closing
    (e.g. with a lot of separate named-pipe-style agent forwarding
    connections being constantly set up and torn down), the handle value
    might have been reused between the two closes, so that the second
    CloseHandle closes an unrelated handle belonging to some other part of
    the program.

    We can't fix this by giving handle_output permanent ownership of the
    handle, because it really _is_ necessary for copies of it to survive
    elsewhere: in particular, for a bidirectional file such as a serial
    port or named pipe, the reading side also needs a copy of the same
    handle! And yet, we can't replace the handle_write_eof call in the
    client with a direct CloseHandle, because that won't wait until
    buffered output has been drained.

    The solution is that the client still calls handle_write_eof to
    register that it _wants_ an EOF sent; the handle_output system will
    wait until it's ready, but then, instead of calling CloseHandle, it
    will ask its _client_ to close the handle, by calling the provided
    'sentdata' callback with the new 'close' flag set to true. And then
    the client can not only close the handle, but do whatever else it
    needs to do to record that that has been done.

 windows/conpty.c        |  3 ++-
 windows/handle-io.c     |  8 +++++---
 windows/handle-socket.c | 16 +++++++++++++---
 windows/platform.h      |  2 +-
 windows/plink.c         |  8 +++++++-
 windows/serial.c        |  3 ++-
 6 files changed, 30 insertions(+), 10 deletions(-)

commit 6dfe941a7309a5af2088061ab0a745555f31b879
web diff https://git.tartarus.org/?p=simon/putty.git;a=commitdiff;h=6dfe941a7309a5af2088061ab0a745555f31b879;hp=dde659004083b575547f2740aaa56fdf64523770
Author: Simon Tatham <anakin at pobox.com>
Date:   Thu Sep 30 18:31:39 2021 +0100

    Windows Pageant: fix hang due to queued callbacks.

    In the Windows Pageant message loop, we were alternating between
    MsgWaitForMultipleObjects with timeout=INFINITE, and
    run_toplevel_callbacks. But run_toplevel_callbacks doesn't loop until
    the callback queue is empty: it just runs at least one of the
    currently queued callbacks, so that some progress was made. So if two
    or more callbacks were queued, we'd leave the rest in the queue and go
    back into MsgWaitForMultipleObjects, which could hang indefinitely.

    (A very silly workaround was available: move the mouse over the
    Pageant systray icon! Every mouse event would terminate the wait and
    let you get one more iteration of run_toplevel_callbacks.)

    Now we do the same thing as in other main loops: if any further
    callbacks are pending, then we still run MsgWaitForMultipleObjects,
    but we do it with timeout=0 instead of timeout=INFINITE, so that we
    won't go back to a _blocking_ sleep until all callbacks have been
    serviced.

 windows/pageant.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

commit e7dd2421cfaf6dd75b316c011e4b10843d2007a6
web diff https://git.tartarus.org/?p=simon/putty.git;a=commitdiff;h=e7dd2421cfaf6dd75b316c011e4b10843d2007a6;hp=6dfe941a7309a5af2088061ab0a745555f31b879
Author: Simon Tatham <anakin at pobox.com>
Date:   Thu Sep 30 18:56:26 2021 +0100

    Pageant: actually link requests on to their queues.

    When I create any class that implements PageantAsyncOp, I had intended
    to link its pao.cr list node on to the list in the appropriate
    PageantClientInfo, so that if that PageantClientInfo was destroyed
    prematurely (e.g. an agent connection was abruptly closed), we could
    destroy all the pending requests containing a pointer to it.

    I did this linking by setting the linked-list fields in pao.cr to
    point to the appropriate places in the existing list - but I didn't
    modify the pointer fields _in_ the existing list to point to the new
    operation at all. So nothing ever actually got linked on to any of
    those queues!

 pageant.c | 2 ++
 1 file changed, 2 insertions(+)