Re: [win-pv-devel] [PATCH 4/4] Handle DBT_DEVICEQUERYREMOVEFAILED

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: win-pv-devel [mailto:win-pv-devel-bounces@xxxxxxxxxxxxxxxxxxxx] On
> Behalf Of Owen Smith
> Sent: 05 November 2018 16:30
> To: win-pv-devel@xxxxxxxxxxxxxxxxxxxx
> Cc: Owen Smith <owen.smith@xxxxxxxxxx>
> Subject: [win-pv-devel] [PATCH 4/4] Handle DBT_DEVICEQUERYREMOVEFAILED
> 
> Split ConsoleCreate/ConsoleDestroy so that a query remove can close all
> device handles, and re-open them is a query remove failed is received.

s/is/if

> This allows the named pipes to keep operating, discarding any data
> whilst the device is being query removed.
> Also fixes several issues with the threading model and leaking handles.
> 
> Signed-off-by: Owen Smith <owen.smith@xxxxxxxxxx>
> ---
>  src/monitor/monitor.c | 418 +++++++++++++++++++++++++++++++++++++--------
> -----
>  1 file changed, 308 insertions(+), 110 deletions(-)
> 
> diff --git a/src/monitor/monitor.c b/src/monitor/monitor.c
> index 1912047..d871af7 100644
> --- a/src/monitor/monitor.c
> +++ b/src/monitor/monitor.c
> @@ -70,6 +70,7 @@ typedef struct _MONITOR_CONSOLE {
>      LIST_ENTRY              ListEntry;
>      PWCHAR                  DevicePath;
>      HANDLE                  DeviceHandle;
> +    BOOL                    DeviceHandleOpen;
>      HDEVNOTIFY              DeviceNotification;
>      PCHAR                   DeviceName; // protocol and instance?
>      HANDLE                  ExecutableThread;
> @@ -387,9 +388,14 @@ ConnectionThread(
> 
>          ResetEvent(Overlapped.hEvent);
> 
> -        PutString(Console->DeviceHandle,
> -                  Buffer,
> -                  Length);
> +        EnterCriticalSection(&Console->CriticalSection);
> +        if (Console->DeviceHandleOpen)
> +            PutString(Console->DeviceHandle,
> +                      Buffer,
> +                      Length);
> +        else
> +            Log("[NO_DEVICE] %.*hs", Length, Buffer);
> +        LeaveCriticalSection(&Console->CriticalSection);
>      }
> 
>      EnterCriticalSection(&Console->CriticalSection);
> @@ -460,6 +466,8 @@ ServerThread(
>      Log("%s", PipeName);
> 
>      for (;;) {
> +        DWORD           ThreadId;
> +
>          Pipe = CreateNamedPipe(PipeName,
>                                 PIPE_ACCESS_DUPLEX | FILE_FLAG_OVERLAPPED,
>                                 PIPE_TYPE_MESSAGE | PIPE_READMODE_MESSAGE,
> @@ -496,10 +504,12 @@ ServerThread(
>                                            0,
>                                            ConnectionThread,
>                                            Connection,
> -                                          0,
> -                                          NULL);
> +                                          CREATE_SUSPENDED,
> +                                          &ThreadId);

What is ThreadId used for?

>          if (Connection->Thread == NULL)
>              goto fail5;
> +
> +        ResumeThread(Connection->Thread);
>      }
> 
>      CloseHandle(Overlapped.hEvent);
> @@ -839,6 +849,138 @@ fail1:
>      return 1;
>  }
> 
> +static BOOL
> +ConsoleOpen(
> +    IN  PMONITOR_CONSOLE    Console
> +    )
> +{
> +    HANDLE                  DeviceHandle;
> +    CHAR                    DeviceName[MAX_PATH];
> +    DWORD                   Bytes;
> +    DWORD                   ThreadId;
> +    BOOL                    Success;
> +
> +    Log("====> %ws", Console->DevicePath);
> +
> +    if (Console->DeviceHandleOpen)
> +        goto fail1;
> +
> +    DeviceHandle = CreateFileW(Console->DevicePath,
> +                               GENERIC_READ | GENERIC_WRITE,
> +                               FILE_SHARE_READ | FILE_SHARE_WRITE,
> +                               NULL,
> +                               OPEN_EXISTING,
> +                               FILE_ATTRIBUTE_NORMAL,
> +                               NULL);
> +    if (DeviceHandle == INVALID_HANDLE_VALUE)
> +        goto fail2;
> +
> +    // check re-open reuses a handle
> +    if ((Console->DeviceHandle != INVALID_HANDLE_VALUE) &&
> +        (Console->DeviceHandle != DeviceHandle))
> +        Log("HANDLE changed! %p -> %p",
> +            (VOID*)Console->DeviceHandle,
> +            (VOID*)DeviceHandle);

Should the above check be within the critical section too? I realize it is only 
there to generate a log message but I guess checking without the lock could 
result in false positives.

> +
> +    EnterCriticalSection(&Console->CriticalSection);
> +    Console->DeviceHandle = DeviceHandle;
> +    Console->DeviceHandleOpen = TRUE;
> +    LeaveCriticalSection(&Console->CriticalSection);
> +
> +    if (Console->DeviceName == NULL) {
> +        Success = DeviceIoControl(Console->DeviceHandle,
> +                                  IOCTL_XENCONS_GET_NAME,
> +                                  NULL,
> +                                  0,
> +                                  DeviceName,
> +                                  sizeof(DeviceName),
> +                                  &Bytes,
> +                                  NULL);
> +        if (!Success)
> +            goto fail3;
> +
> +        DeviceName[MAX_PATH - 1] = '\0';
> +
> +        Console->DeviceName = _strdup(DeviceName);
> +        if (Console->DeviceName == NULL)
> +            goto fail4;
> +    }
> +
> +    ECHO(Console->DeviceHandle, "\r\n[ATTACHED]\r\n");
> +
> +    Console->DeviceThread = CreateThread(NULL,
> +                                         0,
> +                                         DeviceThread,
> +                                         Console,
> +                                         CREATE_SUSPENDED,
> +                                         &ThreadId);

Again, do we need the ThreadId?

> +    if (Console->DeviceThread == NULL)
> +        goto fail5;
> +
> +    ResumeThread(Console->DeviceThread);
> +
> +    Log("<==== %s", Console->DeviceName);
> +
> +    return TRUE;
> +
> +fail5:
> +    Log("fail5");
> +
> +    ECHO(Console->DeviceHandle, "\r\n[DETATCHED]\r\n");
> +
> +    free(Console->DeviceName);
> +    Console->DeviceName = NULL;
> +
> +fail4:
> +    Log("fail4");
> +
> +fail3:
> +    Log("fail3");
> +
> +    EnterCriticalSection(&Console->CriticalSection);
> +    Console->DeviceHandleOpen = FALSE;
> +    CloseHandle(Console->DeviceHandle);
> +    LeaveCriticalSection(&Console->CriticalSection);
> +
> +fail2:
> +    Log("fail2");
> +
> +fail1:
> +    Log("fail1");
> +
> +    return FALSE;
> +}
> +
> +static VOID
> +ConsoleClose(
> +    IN  PMONITOR_CONSOLE    Console
> +    )
> +{
> +    Log("====> %s", Console->DeviceName);
> +
> +    if (!Console->DeviceHandleOpen)
> +        goto fail1;
> +
> +    SetEvent(Console->DeviceEvent);
> +    WaitForSingleObject(Console->DeviceThread, INFINITE);
> +
> +    CloseHandle(Console->DeviceThread);
> +    Console->DeviceThread = NULL;
> +
> +    ECHO(Console->DeviceHandle, "\r\n[DETACHED]\r\n");
> +
> +    EnterCriticalSection(&Console->CriticalSection);
> +    Console->DeviceHandleOpen = FALSE;
> +    CloseHandle(Console->DeviceHandle);
> +    LeaveCriticalSection(&Console->CriticalSection);
> +
> +    Log("<==== %s", Console->DeviceName);
> +    return;
> +
> +fail1:
> +    Log("fail1");
> +}
> +
>  static PMONITOR_CONSOLE
>  ConsoleCreate(
>      IN  PWCHAR              DevicePath
> @@ -847,10 +989,8 @@ ConsoleCreate(
>      PMONITOR_CONTEXT        Context = &MonitorContext;
>      PMONITOR_CONSOLE        Console;
>      DEV_BROADCAST_HANDLE    Handle;
> -    CHAR                    DeviceName[MAX_PATH];
> -    DWORD                   Bytes;
> -    BOOL                    Success;
>      HRESULT                 Error;
> +    DWORD                   ThreadId;

And again.

  Paul

> 
>      Log("====> %ws", DevicePath);
> 
> @@ -862,39 +1002,36 @@ ConsoleCreate(
>      __InitializeListHead(&Console->ListHead);
>      __InitializeListHead(&Console->ListEntry);
>      InitializeCriticalSection(&Console->CriticalSection);
> +    Console->DeviceHandleOpen = FALSE;
> +    Console->DeviceHandle = INVALID_HANDLE_VALUE;
> 
> -    Console->DevicePath = _wcsdup(DevicePath);
> -    if (Console->DevicePath == NULL)
> +    Console->DeviceEvent = CreateEvent(NULL,
> +                                       TRUE,
> +                                       FALSE,
> +                                       NULL);
> +    if (Console->DeviceEvent == NULL)
>          goto fail2;
> 
> -    Console->DeviceHandle = CreateFileW(DevicePath,
> -                                        GENERIC_READ | GENERIC_WRITE,
> -                                        FILE_SHARE_READ |
> FILE_SHARE_WRITE,
> -                                        NULL,
> -                                        OPEN_EXISTING,
> -                                        FILE_ATTRIBUTE_NORMAL,
> -                                        NULL);
> -    if (Console->DeviceHandle == INVALID_HANDLE_VALUE)
> +    Console->ServerEvent = CreateEvent(NULL,
> +                                       TRUE,
> +                                       FALSE,
> +                                       NULL);
> +    if (Console->ServerEvent == NULL)
>          goto fail3;
> 
> -    Success = DeviceIoControl(Console->DeviceHandle,
> -                              IOCTL_XENCONS_GET_NAME,
> -                              NULL,
> -                              0,
> -                              DeviceName,
> -                              sizeof(DeviceName),
> -                              &Bytes,
> -                              NULL);
> -    if (!Success)
> +    Console->ExecutableEvent = CreateEvent(NULL,
> +                                           TRUE,
> +                                           FALSE,
> +                                           NULL);
> +    if (Console->ExecutableEvent == NULL)
>          goto fail4;
> 
> -    DeviceName[MAX_PATH - 1] = '\0';
> -
> -    Console->DeviceName = _strdup(DeviceName);
> -    if (Console->DeviceName == NULL)
> +    Console->DevicePath = _wcsdup(DevicePath);
> +    if (Console->DevicePath == NULL)
>          goto fail5;
> 
> -    ECHO(Console->DeviceHandle, "\r\n[ATTACHED]\r\n");
> +    if (!ConsoleOpen(Console))
> +        goto fail6;
> 
>      ZeroMemory(&Handle, sizeof (Handle));
>      Handle.dbch_size = sizeof (Handle);
> @@ -906,118 +1043,80 @@ ConsoleCreate(
>                                      &Handle,
>                                      DEVICE_NOTIFY_SERVICE_HANDLE);
>      if (Console->DeviceNotification == NULL)
> -        goto fail6;
> -
> -    Console->DeviceEvent = CreateEvent(NULL,
> -                                       TRUE,
> -                                       FALSE,
> -                                       NULL);
> -    if (Console->DeviceEvent == NULL)
>          goto fail7;
> 
> -    Console->DeviceThread = CreateThread(NULL,
> -                                         0,
> -                                         DeviceThread,
> -                                         Console,
> -                                         0,
> -                                         NULL);
> -    if (Console->DeviceThread == NULL)
> -        goto fail8;
> -
> -    Console->ServerEvent = CreateEvent(NULL,
> -                                       TRUE,
> -                                       FALSE,
> -                                       NULL);
> -    if (Console->ServerEvent == NULL)
> -        goto fail9;
> -
>      Console->ServerThread = CreateThread(NULL,
>                                           0,
>                                           ServerThread,
>                                           Console,
> -                                         0,
> -                                         NULL);
> +                                         CREATE_SUSPENDED,
> +                                         &ThreadId);
>      if (Console->ServerThread == NULL)
> -        goto fail10;
> +        goto fail8;
> 
> -    Console->ExecutableEvent = CreateEvent(NULL,
> -                                           TRUE,
> -                                           FALSE,
> -                                           NULL);
> -    if (Console->ExecutableEvent == NULL)
> -        goto fail11;
> +    ResumeThread(Console->ServerThread);
> 
>      Console->ExecutableThread = CreateThread(NULL,
>                                               0,
>                                               ExecutableThread,
>                                               Console,
> -                                             0,
> -                                             NULL);
> +                                             CREATE_SUSPENDED,
> +                                             &ThreadId);
>      if (Console->ExecutableThread == NULL)
> -        goto fail12;
> +        goto fail9;
> +
> +    ResumeThread(Console->ExecutableThread);
> 
>      Log("<==== %s", Console->DeviceName);
> 
>      return Console;
> 
> -fail12:
> -    Log("fail12");
> -
> -    CloseHandle(Console->ExecutableEvent);
> -    Console->ExecutableEvent = NULL;
> -
> -fail11:
> -    Log("fail11");
> +fail9:
> +    Log("fail9");
> 
>      SetEvent(Console->ServerEvent);
>      WaitForSingleObject(Console->ServerThread, INFINITE);
> 
> -fail10:
> -    Log("fail10");
> -
> -    CloseHandle(Console->ServerEvent);
> -    Console->ServerEvent = NULL;
> -
> -fail9:
> -    Log("fail9");
> -
> -    SetEvent(Console->DeviceEvent);
> -    WaitForSingleObject(Console->DeviceThread, INFINITE);
> +    CloseHandle(Console->ServerThread);
> +    Console->ServerThread = NULL;
> 
>  fail8:
>      Log("fail8");
> 
> -    CloseHandle(Console->DeviceEvent);
> -    Console->DeviceEvent = NULL;
> +    UnregisterDeviceNotification(Console->DeviceNotification);
> +    Console->DeviceNotification = NULL;
> 
>  fail7:
>      Log("fail7");
> 
> -    UnregisterDeviceNotification(Console->DeviceNotification);
> -    Console->DeviceNotification = NULL;
> +    EnterCriticalSection(&Console->CriticalSection);
> +    ConsoleClose(Console);
> +    Console->DeviceHandle = INVALID_HANDLE_VALUE;
> +
> 
>  fail6:
>      Log("fail6");
> 
> -    ECHO(Console->DeviceHandle, "\r\n[DETACHED]\r\n");
> -
>      free(Console->DevicePath);
>      Console->DevicePath = NULL;
> 
>  fail5:
>      Log("fail5");
> 
> +    CloseHandle(Console->ExecutableEvent);
> +    Console->ExecutableEvent = NULL;
> +
>  fail4:
>      Log("fail4");
> 
> -    CloseHandle(Console->DeviceHandle);
> -    Console->DeviceHandle = INVALID_HANDLE_VALUE;
> +    CloseHandle(Console->ServerEvent);
> +    Console->ServerEvent = NULL;
> 
>  fail3:
>      Log("fail3");
> 
> -    free(Console->DevicePath);
> -    Console->DevicePath = NULL;
> +    CloseHandle(Console->DeviceEvent);
> +    Console->DeviceEvent = NULL;
> 
>  fail2:
>      Log("fail2");
> @@ -1099,33 +1198,33 @@ ConsoleDestroy(
>      SetEvent(Console->ExecutableEvent);
>      WaitForSingleObject(Console->ExecutableThread, INFINITE);
> 
> -    CloseHandle(Console->ExecutableEvent);
> -    Console->ExecutableEvent = NULL;
> +    CloseHandle(Console->ExecutableThread);
> +    Console->ExecutableThread = NULL;
> 
>      ConsoleWaitForPipes(Console);
> 
> -    CloseHandle(Console->ServerEvent);
> -    Console->ServerEvent = NULL;
> -
> -    SetEvent(Console->DeviceEvent);
> -    WaitForSingleObject(Console->DeviceThread, INFINITE);
> -
> -    CloseHandle(Console->DeviceEvent);
> -    Console->DeviceEvent = NULL;
> +    CloseHandle(Console->ServerThread);
> +    Console->ServerThread = NULL;
> 
>      UnregisterDeviceNotification(Console->DeviceNotification);
>      Console->DeviceNotification = NULL;
> 
> -    ECHO(Console->DeviceHandle, "\r\n[DETACHED]\r\n");
> +    free(Console->DeviceName);
> +    Console->DeviceName = NULL;
> 
>      free(Console->DevicePath);
>      Console->DevicePath = NULL;
> 
> -    CloseHandle(Console->DeviceHandle);
>      Console->DeviceHandle = INVALID_HANDLE_VALUE;
> 
> -    free(Console->DevicePath);
> -    Console->DevicePath = NULL;
> +    CloseHandle(Console->ExecutableEvent);
> +    Console->ExecutableEvent = NULL;
> +
> +    CloseHandle(Console->ServerEvent);
> +    Console->ServerEvent = NULL;
> +
> +    CloseHandle(Console->DeviceEvent);
> +    Console->DeviceEvent = NULL;
> 
>      DeleteCriticalSection(&Console->CriticalSection);
>      ZeroMemory(&Console->ListHead, sizeof(LIST_ENTRY));
> @@ -1136,6 +1235,88 @@ ConsoleDestroy(
>      Log("<====");
>  }
> 
> +static BOOL
> +MonitorOpen(
> +    IN  HANDLE          DeviceHandle
> +    )
> +{
> +    PMONITOR_CONTEXT    Context = &MonitorContext;
> +    PMONITOR_CONSOLE    Console;
> +    PLIST_ENTRY         ListEntry;
> +
> +    Log("=====> 0x%p", DeviceHandle);
> +
> +    EnterCriticalSection(&Context->CriticalSection);
> +    for (ListEntry = Context->ListHead.Flink;
> +        ListEntry != &Context->ListHead;
> +        ListEntry = ListEntry->Flink) {
> +        Console = CONTAINING_RECORD(ListEntry,
> +            MONITOR_CONSOLE,
> +            ListEntry);
> +
> +        if (Console->DeviceHandle == DeviceHandle)
> +            goto found;
> +    }
> +    LeaveCriticalSection(&Context->CriticalSection);
> +
> +    Log("DeviceHandle 0x%p not found", DeviceHandle);
> +
> +    return FALSE;
> +
> +found:
> +    LeaveCriticalSection(&Context->CriticalSection);
> +
> +    if (!ConsoleOpen(Console))
> +        goto fail1;
> +
> +    Log("<=====");
> +
> +    return TRUE;
> +
> +fail1:
> +    Log("fail1");
> +
> +    return FALSE;
> +}
> +
> +static BOOL
> +MonitorClose(
> +    IN  HANDLE          DeviceHandle
> +    )
> +{
> +    PMONITOR_CONTEXT    Context = &MonitorContext;
> +    PMONITOR_CONSOLE    Console;
> +    PLIST_ENTRY         ListEntry;
> +
> +    Log("=====> 0x%p", DeviceHandle);
> +
> +    EnterCriticalSection(&Context->CriticalSection);
> +    for (ListEntry = Context->ListHead.Flink;
> +        ListEntry != &Context->ListHead;
> +        ListEntry = ListEntry->Flink) {
> +        Console = CONTAINING_RECORD(ListEntry,
> +            MONITOR_CONSOLE,
> +            ListEntry);
> +
> +        if (Console->DeviceHandle == DeviceHandle)
> +            goto found;
> +    }
> +    LeaveCriticalSection(&Context->CriticalSection);
> +
> +    Log("DeviceHandle 0x%p not found", DeviceHandle);
> +
> +    return FALSE;
> +
> +found:
> +    LeaveCriticalSection(&Context->CriticalSection);
> +
> +    ConsoleClose(Console);
> +
> +    Log("<=====");
> +
> +    return TRUE;
> +}
> +
>  static BOOL
>  MonitorAdd(
>      IN  PWCHAR          DevicePath
> @@ -1342,6 +1523,7 @@ MonitorRemoveAll(
> 
>          LeaveCriticalSection(&Context->CriticalSection);
> 
> +        ConsoleClose(Console);
>          ConsoleDestroy(Console);
>      }
>      LeaveCriticalSection(&Context->CriticalSection);
> @@ -1386,7 +1568,23 @@ MonitorCtrlHandlerEx(
>              }
>              break;
> 
> +        case DBT_DEVICEQUERYREMOVEFAILED:
> +            if (Header->dbch_devicetype == DBT_DEVTYP_HANDLE) {
> +                PDEV_BROADCAST_HANDLE Device = EventData;
> +
> +                MonitorOpen(Device->dbch_handle);
> +                // should check return - Open can fail!
> +            }
> +            break;
> +
>          case DBT_DEVICEQUERYREMOVE:
> +            if (Header->dbch_devicetype == DBT_DEVTYP_HANDLE) {
> +                PDEV_BROADCAST_HANDLE Device = EventData;
> +
> +                MonitorClose(Device->dbch_handle);
> +            }
> +            break;
> +
>          case DBT_DEVICEREMOVEPENDING:
>          case DBT_DEVICEREMOVECOMPLETE:
>              if (Header->dbch_devicetype == DBT_DEVTYP_HANDLE) {
> --
> 2.16.2.windows.1
> 
> 
> _______________________________________________
> win-pv-devel mailing list
> win-pv-devel@xxxxxxxxxxxxxxxxxxxx
> https://lists.xenproject.org/mailman/listinfo/win-pv-devel
_______________________________________________
win-pv-devel mailing list
win-pv-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/win-pv-devel