From cde2648c2250eb373d1e245160e2aca26dc0555c Mon Sep 17 00:00:00 2001 From: Corinna Vinschen Date: Wed, 7 Mar 2018 16:19:32 +0100 Subject: [PATCH] Cygwin: AF_UNIX: make sure connect wait thread is cleanly interruptible Using TerminateThread potentially leaks resources. In our case, the connect wait thread may be forcefully terminated after having successfully opened a client side pipe handle. If this occurs, we have a stale pipe server instance, so the pipe will never be closed as long as the process lives. Avoid this by changing the npfs handle to non-blocking, so we can wait on a termination event object from inside the thread itself and cleanly exit from the thread instead of terminating. Signed-off-by: Corinna Vinschen --- winsup/cygwin/fhandler.h | 1 + winsup/cygwin/fhandler_socket_unix.cc | 56 +++++++++++++++++++++++---- 2 files changed, 49 insertions(+), 8 deletions(-) diff --git a/winsup/cygwin/fhandler.h b/winsup/cygwin/fhandler.h index 4166126fc..13f406821 100644 --- a/winsup/cygwin/fhandler.h +++ b/winsup/cygwin/fhandler.h @@ -860,6 +860,7 @@ class fhandler_socket_unix : public fhandler_socket if the socket is backed by a file in the file system (actually a reparse point) */ HANDLE connect_wait_thr; + HANDLE cwt_termination_evt; PVOID cwt_param; LONG so_error; sun_name_t *sun_path; diff --git a/winsup/cygwin/fhandler_socket_unix.cc b/winsup/cygwin/fhandler_socket_unix.cc index 099000d0d..dcd493856 100644 --- a/winsup/cygwin/fhandler_socket_unix.cc +++ b/winsup/cygwin/fhandler_socket_unix.cc @@ -667,7 +667,7 @@ fhandler_socket_unix::npfs_handle (HANDLE &nph) InitializeObjectAttributes (&attr, &ro_u_npfs, 0, NULL, NULL); status = NtOpenFile (&npfs_dirh, FILE_READ_ATTRIBUTES | SYNCHRONIZE, &attr, &io, FILE_SHARE_READ | FILE_SHARE_WRITE, - FILE_SYNCHRONOUS_IO_NONALERT); + 0); } ReleaseSRWLockExclusive (&npfs_lock); if (NT_SUCCESS (status)) @@ -808,7 +808,11 @@ fhandler_socket_unix::wait_pipe (PUNICODE_STRING pipe_name) conn_wait_info_t *wait_info; DWORD waitret, err; int ret = -1; + HANDLE thr, evt; + PVOID param; + if (!(cwt_termination_evt = create_event ())) + return -1; wait_info = (conn_wait_info_t *) cmalloc_abort (HEAP_FHANDLER, sizeof *wait_info); wait_info->fh = this; @@ -823,23 +827,28 @@ fhandler_socket_unix::wait_pipe (PUNICODE_STRING pipe_name) { cfree (wait_info); __seterrno (); - return -1; + goto out; } if (is_nonblocking ()) { set_errno (EINPROGRESS); - return -1; + goto out; } waitret = cygwait (connect_wait_thr, cw_infinite, cw_cancel | cw_sig_eintr); if (waitret == WAIT_OBJECT_0) GetExitCodeThread (connect_wait_thr, &err); else - TerminateThread (connect_wait_thr, 0); - HANDLE thr = InterlockedExchangePointer (&connect_wait_thr, NULL); + { + SetEvent (cwt_termination_evt); + WaitForSingleObject (connect_wait_thr, INFINITE); + GetExitCodeThread (connect_wait_thr, &err); + waitret = WAIT_SIGNALED; + } + thr = InterlockedExchangePointer (&connect_wait_thr, NULL); if (thr) CloseHandle (thr); - PVOID param = InterlockedExchangePointer (&cwt_param, NULL); + param = InterlockedExchangePointer (&cwt_param, NULL); if (param) cfree (param); switch (waitret) @@ -858,6 +867,10 @@ fhandler_socket_unix::wait_pipe (PUNICODE_STRING pipe_name) ret = 0; break; } +out: + evt = InterlockedExchangePointer (&cwt_termination_evt, NULL); + if (evt) + NtClose (evt); return ret; } @@ -965,6 +978,7 @@ fhandler_socket_unix::fixup_after_fork (HANDLE parent) InitializeSRWLock (&bind_lock); InitializeSRWLock (&io_lock); connect_wait_thr = NULL; + cwt_termination_evt = NULL; cwt_param = NULL; } @@ -1001,6 +1015,7 @@ fhandler_socket_unix::dup (fhandler_base *child, int flags) InitializeSRWLock (&fhs->bind_lock); InitializeSRWLock (&fhs->io_lock); fhs->connect_wait_thr = NULL; + fhs->cwt_termination_evt = NULL; fhs->cwt_param = NULL; return fhandler_socket::dup (child, flags); } @@ -1019,6 +1034,7 @@ DWORD fhandler_socket_unix::wait_pipe_thread (PUNICODE_STRING pipe_name) { HANDLE npfsh; + HANDLE evt; LONG error = 0; NTSTATUS status; IO_STATUS_BLOCK io; @@ -1033,6 +1049,8 @@ fhandler_socket_unix::wait_pipe_thread (PUNICODE_STRING pipe_name) error = geterrno_from_nt_status (status); goto out; } + if (!(evt = create_event ())) + goto out; pwbuf_size = offsetof (FILE_PIPE_WAIT_FOR_BUFFER, Name) + pipe_name->Length; pwbuf = (PFILE_PIPE_WAIT_FOR_BUFFER) alloca (pwbuf_size); pwbuf->Timeout.QuadPart = -20 * NS100PERSEC; /* 20 secs */ @@ -1042,8 +1060,22 @@ fhandler_socket_unix::wait_pipe_thread (PUNICODE_STRING pipe_name) stamp = ntod.nsecs (); do { - status = NtFsControlFile (npfsh, NULL, NULL, NULL, &io, FSCTL_PIPE_WAIT, + status = NtFsControlFile (npfsh, evt, NULL, NULL, &io, FSCTL_PIPE_WAIT, pwbuf, pwbuf_size, NULL, 0); + if (status == STATUS_PENDING) + { + HANDLE w[2] = { evt, cwt_termination_evt }; + switch (WaitForMultipleObjects (2, w, FALSE, INFINITE)) + { + case WAIT_OBJECT_0: + status = io.Status; + break; + case WAIT_OBJECT_0 + 1: + default: + status = STATUS_THREAD_IS_TERMINATING; + break; + } + } switch (status) { case STATUS_SUCCESS: @@ -1074,6 +1106,9 @@ fhandler_socket_unix::wait_pipe_thread (PUNICODE_STRING pipe_name) case STATUS_INSUFFICIENT_RESOURCES: error = ENOBUFS; break; + case STATUS_THREAD_IS_TERMINATING: + error = EINTR; + break; case STATUS_INVALID_DEVICE_REQUEST: default: error = EIO; @@ -1434,12 +1469,17 @@ fhandler_socket_unix::shutdown (int how) int fhandler_socket_unix::close () { + HANDLE evt = InterlockedExchangePointer (&cwt_termination_evt, NULL); HANDLE thr = InterlockedExchangePointer (&connect_wait_thr, NULL); if (thr) { - TerminateThread (thr, 0); + if (evt) + SetEvent (evt); + WaitForSingleObject (thr, INFINITE); CloseHandle (thr); } + if (evt) + NtClose (evt); PVOID param = InterlockedExchangePointer (&cwt_param, NULL); if (param) cfree (param);