From dde659004083b575547f2740aaa56fdf64523770 Mon Sep 17 00:00:00 2001 From: Simon Tatham Date: Thu, 30 Sep 2021 19:16:20 +0100 Subject: [PATCH] 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(-) diff --git a/windows/conpty.c b/windows/conpty.c index f2882316..322af7bf 100644 --- a/windows/conpty.c +++ b/windows/conpty.c @@ -120,7 +120,8 @@ static size_t conpty_gotdata( } } -static void conpty_sentdata(struct handle *h, size_t new_backlog, int err) +static void conpty_sentdata(struct handle *h, size_t new_backlog, int err, + bool close) { ConPTY *conpty = (ConPTY *)handle_get_privdata(h); if (err) { diff --git a/windows/handle-io.c b/windows/handle-io.c index 873eaf30..e012c8ed 100644 --- a/windows/handle-io.c +++ b/windows/handle-io.c @@ -284,6 +284,7 @@ struct handle_output { * drops. */ handle_outputfn_t sentdata; + struct handle *sentdata_param; }; static DWORD WINAPI handle_output_threadfunc(void *param) @@ -361,7 +362,7 @@ static void handle_try_output(struct handle_output *ctx) ctx->busy = true; } else if (!ctx->busy && bufchain_size(&ctx->queued_data) == 0 && ctx->outgoingeof == EOF_PENDING) { - CloseHandle(ctx->h); + ctx->sentdata(ctx->sentdata_param, 0, 0, true); ctx->h = INVALID_HANDLE_VALUE; ctx->outgoingeof = EOF_SENT; } @@ -507,6 +508,7 @@ struct handle *handle_output_new(HANDLE handle, handle_outputfn_t sentdata, bufchain_init(&h->u.o.queued_data); h->u.o.outgoingeof = EOF_NO; h->u.o.sentdata = sentdata; + h->u.o.sentdata_param = h; h->u.o.flags = flags; ensure_ready_event_setup(); @@ -644,11 +646,11 @@ static void handle_ready(struct handle *h) * thread is terminating by now). */ h->u.o.defunct = true; - h->u.o.sentdata(h, 0, h->u.o.writeerr); + h->u.o.sentdata(h, 0, h->u.o.writeerr, false); } else { bufchain_consume(&h->u.o.queued_data, h->u.o.lenwritten); noise_ultralight(NOISE_SOURCE_IOLEN, h->u.o.lenwritten); - h->u.o.sentdata(h, bufchain_size(&h->u.o.queued_data), 0); + h->u.o.sentdata(h, bufchain_size(&h->u.o.queued_data), 0, false); handle_try_output(&h->u.o); } break; diff --git a/windows/handle-socket.c b/windows/handle-socket.c index dbc4e0de..a024807e 100644 --- a/windows/handle-socket.c +++ b/windows/handle-socket.c @@ -93,10 +93,19 @@ static size_t handle_stderr( return 0; } -static void handle_sentdata(struct handle *h, size_t new_backlog, int err) +static void handle_sentdata(struct handle *h, size_t new_backlog, int err, + bool close) { HandleSocket *hs = (HandleSocket *)handle_get_privdata(h); + if (close) { + if (hs->send_H != INVALID_HANDLE_VALUE) + CloseHandle(hs->send_H); + if (hs->recv_H != INVALID_HANDLE_VALUE && hs->recv_H != hs->send_H) + CloseHandle(hs->recv_H); + hs->send_H = hs->recv_H = INVALID_HANDLE_VALUE; + } + if (err) { plug_closing(hs->plug, win_strerror(err), err, 0); return; @@ -125,8 +134,9 @@ static void sk_handle_close(Socket *s) handle_free(hs->send_h); handle_free(hs->recv_h); - CloseHandle(hs->send_H); - if (hs->recv_H != hs->send_H) + if (hs->send_H != INVALID_HANDLE_VALUE) + CloseHandle(hs->send_H); + if (hs->recv_H != INVALID_HANDLE_VALUE && hs->recv_H != hs->send_H) CloseHandle(hs->recv_H); bufchain_clear(&hs->inputdata); diff --git a/windows/platform.h b/windows/platform.h index ba07ea1a..65dacdc1 100644 --- a/windows/platform.h +++ b/windows/platform.h @@ -615,7 +615,7 @@ struct handle; typedef size_t (*handle_inputfn_t)( struct handle *h, const void *data, size_t len, int err); typedef void (*handle_outputfn_t)( - struct handle *h, size_t new_backlog, int err); + struct handle *h, size_t new_backlog, int err, bool close); struct handle *handle_input_new(HANDLE handle, handle_inputfn_t gotdata, void *privdata, int flags); struct handle *handle_output_new(HANDLE handle, handle_outputfn_t sentdata, diff --git a/windows/plink.c b/windows/plink.c index 8d3a75cf..503ae30b 100644 --- a/windows/plink.c +++ b/windows/plink.c @@ -216,8 +216,14 @@ size_t stdin_gotdata(struct handle *h, const void *data, size_t len, int err) return 0; } -void stdouterr_sent(struct handle *h, size_t new_backlog, int err) +void stdouterr_sent(struct handle *h, size_t new_backlog, int err, bool close) { + if (close) { + CloseHandle(outhandle); + CloseHandle(errhandle); + outhandle = errhandle = INVALID_HANDLE_VALUE; + } + if (err) { char buf[4096]; FormatMessage(FORMAT_MESSAGE_FROM_SYSTEM, NULL, err, 0, diff --git a/windows/serial.c b/windows/serial.c index 470c979f..d966145c 100644 --- a/windows/serial.c +++ b/windows/serial.c @@ -73,7 +73,8 @@ static size_t serial_gotdata( } } -static void serial_sentdata(struct handle *h, size_t new_backlog, int err) +static void serial_sentdata(struct handle *h, size_t new_backlog, int err, + bool close) { Serial *serial = (Serial *)handle_get_privdata(h); if (err) {