Re: [PATCH] winebus.sys: Handle device reports for hidraw devices

[Sebastian Lackner]

On 17.10.2016 06:41, Aric Stewart wrote:
> Signed-off-by: Aric Stewart <aric(a)codeweavers.com>
> ---
>  dlls/winebus.sys/bus.h      |  3 ++
>  dlls/winebus.sys/bus_udev.c | 63 +++++++++++++++++++++++++++++
>  dlls/winebus.sys/main.c     | 97 ++++++++++++++++++++++++++++++++++++++++++++-
>  3 files changed, 162 insertions(+), 1 deletion(-)
>
> 0001-winebus.sys-Handle-device-reports-for-hidraw-devices.txt
>
> diff --git a/dlls/winebus.sys/bus.h b/dlls/winebus.sys/bus.h
> index 7ce54bf..bcb1163 100644
> --- a/dlls/winebus.sys/bus.h
> +++ b/dlls/winebus.sys/bus.h
> @@ -25,6 +25,8 @@ typedef struct
>      int (*compare_platform_device)(DEVICE_OBJECT *device, void *platform_dev);
>      NTSTATUS (*get_reportdescriptor)(DEVICE_OBJECT *device, BYTE *buffer, DWORD length, DWORD *out_length);
>      NTSTATUS (*get_string)(DEVICE_OBJECT *device, DWORD index, WCHAR *buffer, DWORD length);
> +    NTSTATUS (*begin_report_processing)(DEVICE_OBJECT *device, BYTE **report_buffer, DWORD *buffer_length);
> +    void (*end_report_processing)(DEVICE_OBJECT *device, BYTE *report_buffer);
>  } platform_vtbl;
>
> void *get_platform_private(DEVICE_OBJECT *device) DECLSPEC_HIDDEN;
> @@ -37,3 +39,4 @@ DEVICE_OBJECT *bus_create_hid_device(DRIVER_OBJECT *driver, const WCHAR *busidW,
>  DEVICE_OBJECT *bus_find_hid_device(const platform_vtbl *vtbl, void *platform_dev) DECLSPEC_HIDDEN;
>  void bus_remove_hid_device(DEVICE_OBJECT *device) DECLSPEC_HIDDEN;
>  NTSTATUS WINAPI hid_internal_dispatch(DEVICE_OBJECT *device, IRP *irp) DECLSPEC_HIDDEN;
> +void process_hid_report(DEVICE_OBJECT *device, const WCHAR *busidW, BYTE *report, DWORD length) DECLSPEC_HIDDEN;
> diff --git a/dlls/winebus.sys/bus_udev.c b/dlls/winebus.sys/bus_udev.c
> index 243aabb..5307224 100644
> --- a/dlls/winebus.sys/bus_udev.c
> +++ b/dlls/winebus.sys/bus_udev.c
> @@ -61,6 +61,8 @@ WINE_DEFAULT_DEBUG_CHANNEL(plugplay);
>
> #ifdef HAVE_UDEV
>
> +WINE_DECLARE_DEBUG_CHANNEL(hid_report);
> +
>  static struct udev *udev_context = NULL;
>  static DRIVER_OBJECT *udev_driver_obj = NULL;
>
> @@ -73,6 +75,10 @@ struct platform_private
>  {
>      struct udev_device *udev_device;
>      int device_fd;
> +
> +    BYTE *report_buffer;
> +    int buffer_length;
> +    HANDLE report_thread;
>  };
>
> static inline struct platform_private *impl_from_DEVICE_OBJECT(DEVICE_OBJECT *device)
> @@ -222,11 +228,68 @@ static NTSTATUS hidraw_get_string(DEVICE_OBJECT *device, DWORD index, WCHAR *buf
>      return STATUS_SUCCESS;
>  }
>
> +static DWORD CALLBACK device_report_thread(void *args)
> +{
> +    DEVICE_OBJECT *device = (DEVICE_OBJECT*)args;
> +    struct platform_private *private = impl_from_DEVICE_OBJECT(device);
> +    struct pollfd plfds[1];
> +
> +    plfds[0].fd = private->device_fd;
> +    plfds[0].events = POLLIN;
> +
> +    while (1)
> +    {
> +        int size;
> +        if (poll(plfds, 1, -1) <= 0) continue;
> +        if (!private->report_buffer || private->buffer_length == 0)
> +            break;
> +        size = read(plfds[0].fd, private->report_buffer, private->buffer_length);
> +        if (size == -1)
> +            TRACE_(hid_report)("Read failed. Likely an unplugged device\n");
> +        else if (size == 0)
> +            TRACE_(hid_report)("Failed to read report\n");
> +        else
> +            process_hid_report(device, hidraw_busidW, private->report_buffer, size);
> +    }
> +    return 0;
> +}
> +
> +static NTSTATUS begin_report_processing(DEVICE_OBJECT *device, BYTE **buffer, DWORD *length)
> +{
> +    struct platform_private *private = impl_from_DEVICE_OBJECT(device);
> +    *length = private->buffer_length = 1024;
> +    *buffer = private->report_buffer = HeapAlloc(GetProcessHeap(), 0, *length);
> +
> +    private->report_thread = CreateThread(NULL, 0, device_report_thread, device, 0, NULL);
> +    if (!private->report_thread)
> +    {
> +        ERR("Unable to create device report thread\n");
> +        return STATUS_UNSUCCESSFUL;
> +    }
> +    else
> +        return STATUS_SUCCESS;
> +}
> +
> +static void end_report_processing(DEVICE_OBJECT *device, BYTE *buffer)
> +{
> +    struct platform_private *private = impl_from_DEVICE_OBJECT(device);
> +    if (private->report_thread)
> +    {
> +        TerminateThread(private->report_thread, 0);

TerminateThread is not really the best way to clean up this worker thread. Depending
on when exactly it is called, it can corrupt internal data and crash the whole process.
In this case, it would definitely be better to have some synchronization mechanism
(for example event pipe) to allow a clean shutdown.
> +        WaitForSingleObject(private->report_thread, INFINITE);
> +    }
> +    private->buffer_length = 0;
> +    private->report_buffer = NULL;
> +    HeapFree(GetProcessHeap(), 0, buffer);
> +}
> +
>  static const platform_vtbl hidraw_vtbl =
>  {
>      compare_platform_device,
>      hidraw_get_reportdescriptor,
>      hidraw_get_string,
> +    begin_report_processing,
> +    end_report_processing,
>  };
>
> static void try_add_device(struct udev_device *dev)
> diff --git a/dlls/winebus.sys/main.c b/dlls/winebus.sys/main.c
> index 86adbf5..f12cdf7 100644
> --- a/dlls/winebus.sys/main.c
> +++ b/dlls/winebus.sys/main.c
> @@ -40,6 +40,7 @@
>  #include "bus.h"
>
> WINE_DEFAULT_DEBUG_CHANNEL(plugplay);
> +WINE_DECLARE_DEBUG_CHANNEL(hid_report);
>
> struct pnp_device
>  {
> @@ -58,6 +59,13 @@ struct device_extension
>      const WCHAR *busid;  /* Expected to be a static constant */
>
> const platform_vtbl *vtbl;
> +
> +    BOOL processing_started;
> +    BYTE *last_report;
> +    DWORD last_report_size;
> +    DWORD buffer_size;
> +    LIST_ENTRY irp_queue;
> +
>      BYTE platform_private[1];
>  };
>
> @@ -214,6 +222,8 @@ DEVICE_OBJECT *bus_create_hid_device(DRIVER_OBJECT *driver, const WCHAR *busidW,
>      ext->busid      = busidW;
>      ext->vtbl       = vtbl;
>
> +    InitializeListHead(&ext->irp_queue);
> +
>      /* add to list of pnp devices */
>      pnp_dev->device = device;
>      list_add_tail(&pnp_devset, &pnp_dev->entry);
> @@ -271,6 +281,8 @@ void bus_remove_hid_device(DEVICE_OBJECT *device)
>  {
>      struct device_extension *ext = (struct device_extension *)device->DeviceExtension;
>      struct pnp_device *pnp_device = ext->pnp_device;
> +    LIST_ENTRY *entry;
> +    IRP *irp;
>
> TRACE("(%p)\n", device);
>
> @@ -279,6 +291,21 @@ void bus_remove_hid_device(DEVICE_OBJECT *device)
>      LeaveCriticalSection(&device_list_cs);
>
> IoInvalidateDeviceRelations(device, RemovalRelations);
> +
> +    /* Cancel pending IRPs */
> +    entry = RemoveHeadList(&ext->irp_queue);
> +    while(entry != &ext->irp_queue)
> +    {
> +        irp = CONTAINING_RECORD(entry, IRP, Tail.Overlay.ListEntry);
> +        irp->IoStatus.u.Status = STATUS_CANCELLED;
> +        irp->IoStatus.Information = 0;
> +        IoCompleteRequest(irp, IO_NO_INCREMENT);
> +        entry = RemoveHeadList(&ext->irp_queue);
> +    }
> +
> +    if (ext->processing_started)
> +        ext->vtbl->end_report_processing(device, ext->last_report);
> +
>      HeapFree(GetProcessHeap(), 0, ext->serial);
>      IoDeleteDevice(device);
>
> @@ -432,6 +459,49 @@ NTSTATUS WINAPI hid_internal_dispatch(DEVICE_OBJECT *device, IRP *irp)
>                  irp->IoStatus.Information = (strlenW((WCHAR *)irp->UserBuffer) + 1) * sizeof(WCHAR);
>              break;
>          }
> +        case IOCTL_HID_GET_INPUT_REPORT:
> +        {
> +            HID_XFER_PACKET *packet = (HID_XFER_PACKET*)(irp->UserBuffer);
> +            TRACE_(hid_report)("IOCTL_HID_GET_INPUT_REPORT\n");
> +            if (!ext->processing_started)
> +            {
> +                irp->IoStatus.u.Status = status = ext->vtbl->begin_report_processing(device, &ext->last_report, &ext->buffer_size);
> +                if (status == STATUS_SUCCESS)
> +                    ext->processing_started = TRUE;
> +                else
> +                    break;
> +            }

We might need synchronization here to ensure that only one worker thread is started
(if there are multiple IOCTL_HID_GET_INPUT_REPORT requests at the same time).
> +
> +            if (packet->reportBufferLen < ext->last_report_size)
> +            {
> +                irp->IoStatus.u.Status = status = STATUS_BUFFER_TOO_SMALL;
> +                break;
> +            }
> +
> +            ZeroMemory(packet->reportBuffer, packet->reportBufferLen);
> +            memcpy(packet->reportBuffer, ext->last_report, ext->last_report_size);

The worker thread always uses the same buffer. How do you ensure that the "last_report"
you are copying here is a complete record, and not corrupted (for example partially
old data)?
> +            packet->reportBufferLen = ext->last_report_size;
> +            irp->IoStatus.Information = ext->last_report_size;
> +            irp->IoStatus.u.Status = status = STATUS_SUCCESS;
> +            break;
> +        }
> +        case IOCTL_HID_READ_REPORT:
> +        {
> +            TRACE_(hid_report)("IOCTL_HID_READ_REPORT\n");
> +            if (!ext->processing_started)
> +            {
> +                if (ext->vtbl->begin_report_processing(device, &ext->last_report, &ext->buffer_size) == STATUS_SUCCESS)
> +                    ext->processing_started = TRUE;
> +                else
> +                {
> +                    irp->IoStatus.u.Status = status;
> +                    break;
> +                }
> +            }
> +            InsertTailList(&ext->irp_queue, &irp->Tail.Overlay.ListEntry);

The irp_queue will require locking when it is accessed from multiple threads.
> +            status = STATUS_PENDING;

Thats fine for now, but I'm planning to send some additional patches soon to fix STATUS_PENDING handling.
Drivers on Windows have to call IoMarkIrpPending() when they return STATUS_PENDING. Also see:

https://msdn.microsoft.com/en-us/library/windows/hardware/ff549422(v=vs.85)....
"""Otherwise, the I/O manager attempts to complete the IRP as soon as the dispatch routine returns control."""

and

"""If a driver queues incoming IRPs, it should call IoMarkIrpPending before it queues each IRP.
Otherwise, an IRP could be dequeued, completed by another driver routine, and freed by the system
before the call to IoMarkIrpPending occurs, thereby causing a crash. """
> +            break;
> +        }
>          default:
>          {
>              ULONG code = irpsp->Parameters.DeviceIoControl.IoControlCode;
> @@ -441,11 +511,36 @@ NTSTATUS WINAPI hid_internal_dispatch(DEVICE_OBJECT *device, IRP *irp)
>          }
>      }
>
> -    IoCompleteRequest(irp, IO_NO_INCREMENT);
> +    if (status != STATUS_PENDING)
> +        IoCompleteRequest(irp, IO_NO_INCREMENT);
>
> return status;
>  }
>
> +void process_hid_report(DEVICE_OBJECT *device, const WCHAR *busidW, BYTE *report, DWORD length)
> +{
> +    struct device_extension *ext = (struct device_extension*)device->DeviceExtension;
> +    IRP *irp;
> +    LIST_ENTRY *entry;
> +
> +    if (ext->last_report && report != ext->last_report && ext->buffer_size <= length)
> +    {
> +        memcpy(ext->last_report, report, length);
> +        ext->last_report_size = length;
> +    }
> +    entry = RemoveHeadList(&ext->irp_queue);
> +    while(entry != &ext->irp_queue)
> +    {
> +        TRACE_(hid_report)("Processing Request\n");
> +        irp = CONTAINING_RECORD(entry, IRP, Tail.Overlay.ListEntry);
> +        memcpy(irp->UserBuffer, report, length);

Shouldn't there also be some kind of length validation?
> +        irp->IoStatus.Information = length;
> +        irp->IoStatus.u.Status = STATUS_SUCCESS;
> +        IoCompleteRequest(irp, IO_NO_INCREMENT);
> +        entry = RemoveHeadList(&ext->irp_queue);
> +    }
> +}
> +
>  NTSTATUS WINAPI DriverEntry( DRIVER_OBJECT *driver, UNICODE_STRING *path )
>  {
>      static const WCHAR udevW[] = {'\\','D','r','i','v','e','r','\\','U','D','E','V',0};