[PATCH] winebus.sys: Implement IOCTL_HID_GET_STRING for hidraw

[Aric Stewart]

On 10/12/16 4:02 PM, Sebastian Lackner wrote:
> On 12.10.2016 12:12, Aric Stewart wrote:
>> Signed-off-by: Aric Stewart <aric at codeweavers.com>
>> ---
>>  dlls/winebus.sys/bus.h      |  1 +
>>  dlls/winebus.sys/bus_udev.c | 70 +++++++++++++++++++++++++++++++++++++++++++++
>>  dlls/winebus.sys/main.c     | 11 +++++++
>>  3 files changed, 82 insertions(+)
>>
>>
>>
>> 0001-winebus.sys-Implement-IOCTL_HID_GET_STRING-for-hidraw.txt
>>
>>
>> diff --git a/dlls/winebus.sys/bus.h b/dlls/winebus.sys/bus.h
>> index 9ab242c..9094e17 100644
>> --- a/dlls/winebus.sys/bus.h
>> +++ b/dlls/winebus.sys/bus.h
>> @@ -24,6 +24,7 @@ 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, int index, WCHAR *buffer, DWORD length);
>>  } platform_vtbl;
>>  
>>  void *get_platform_private(DEVICE_OBJECT *device) DECLSPEC_HIDDEN;
>> diff --git a/dlls/winebus.sys/bus_udev.c b/dlls/winebus.sys/bus_udev.c
>> index 22ea07d..4ded568 100644
>> --- a/dlls/winebus.sys/bus_udev.c
>> +++ b/dlls/winebus.sys/bus_udev.c
>> @@ -51,7 +51,9 @@
>>  #include "winnls.h"
>>  #include "winternl.h"
>>  #include "ddk/wdm.h"
>> +#include "ddk/hidtypes.h"
>>  #include "wine/debug.h"
>> +#include "wine/unicode.h"
>>  
>>  #include "bus.h"
>>  
>> @@ -144,10 +146,78 @@ static NTSTATUS hidraw_get_reportdescriptor(DEVICE_OBJECT *device, BYTE *buffer,
>>  #endif
>>  }
>>  
>> +static NTSTATUS hidraw_get_string(DEVICE_OBJECT *device, int index, WCHAR *buffer, DWORD length)
>> +{
>> +    struct udev_device *usbdev;
>> +    struct platform_private *private = impl_from_DEVICE_OBJECT(device);
>> +
>> +    usbdev = udev_device_get_parent_with_subsystem_devtype(private->udev_device, "usb", "usb_device");
>> +    if (usbdev)
>> +    {
>> +        WCHAR *str = NULL;
>> +        switch (index)
>> +        {
>> +            case HID_STRING_ID_IPRODUCT:
>> +                str = get_sysattr_string(usbdev, "product");
>> +                break;
>> +            case HID_STRING_ID_IMANUFACTURER:
>> +                str = get_sysattr_string(usbdev, "manufacturer");
>> +                break;
>> +            case HID_STRING_ID_ISERIALNUMBER:
>> +                str = get_sysattr_string(usbdev, "serial");
>> +                break;
>> +            default:
>> +                ERR("Unhandled string index %i\n", index);
>> +                return STATUS_NOT_IMPLEMENTED;
>> +        }
>> +        if (str)
>> +        {
>> +            if (strlenW(str) >= length)
>> +            {
>> +                HeapFree(GetProcessHeap(), 0, str);
>> +                return STATUS_BUFFER_TOO_SMALL;
>> +            }
>> +            strcpyW(buffer, str);
>> +            HeapFree(GetProcessHeap(), 0, str);
>> +            return STATUS_SUCCESS;
>> +        }
>> +        else if (length)
>> +            buffer[0] = 0;
>> +        return STATUS_SUCCESS;
> 
> Shouldn't it still return STATUS_BUFFER_TOO_SMALL when length == 0?
> 
>> +    }
>> +    else
>> +    {
>> +#ifdef HAVE_LINUX_HIDRAW_H
>> +        static char str[MAX_PATH] = {0};
> 
> Isn't there a risk of race-conditions that multiple HID devices attempt to use
> this static buffer at the same time?
> 
>> +
>> +        switch (index)
>> +        {
>> +            case HID_STRING_ID_IPRODUCT:
>> +                ioctl(private->device_fd, HIDIOCGRAWNAME(MAX_PATH), &str);
>> +                break;
>> +            case HID_STRING_ID_IMANUFACTURER:
>> +                break;
>> +            case HID_STRING_ID_ISERIALNUMBER:
>> +                break;
>> +            default:
>> +                ERR("Unhandled string index %i\n", index);
>> +                return STATUS_NOT_IMPLEMENTED;
>> +        }
>> +#endif
>> +        if (str[0])
> 
> If HAVE_LINUX_HIDRAW_H is not available this will break compilation (str undeclared).
> 
>> +            MultiByteToWideChar(CP_ACP, 0, str, -1, buffer, length);
>> +        else if (length)
>> +            buffer[0] = 0;
> 
> It would be better to do length checking also in this case.
> 
>> +
>> +        return STATUS_SUCCESS;
>> +    }
>> +}
>> +
>>  static const platform_vtbl hidraw_vtbl =
>>  {
>>      compare_platform_device,
>>      hidraw_get_reportdescriptor,
>> +    hidraw_get_string,
>>  };
>>  
>>  static void try_add_device(struct udev_device *dev)
>> diff --git a/dlls/winebus.sys/main.c b/dlls/winebus.sys/main.c
>> index ad09ec1..a7b76df 100644
>> --- a/dlls/winebus.sys/main.c
>> +++ b/dlls/winebus.sys/main.c
>> @@ -421,6 +421,17 @@ NTSTATUS WINAPI hid_internal_dispatch(DEVICE_OBJECT *device, IRP *irp)
>>              irp->IoStatus.Information = length;
>>              break;
>>          }
>> +        case IOCTL_HID_GET_STRING:
>> +        {
>> +            DWORD index = (DWORD)irpsp->Parameters.DeviceIoControl.Type3InputBuffer;
> 
> This will trigger a compilation warning on 64-bit. You have to cast to a ptr-size integer
> type first, for example:
> 
> DWORD index = (ULONG_PTR)irpsp->Parameters.DeviceIoControl.Type3InputBuffer;
> 
> Also, accoding to MSDN, the content of the variable is a composite value:
> 
> """Parameters.DeviceIoControl.Type3InputBuffer in the I/O stack location of the IRP contains
> a composite value. The two most significant bytes contain the language ID of the string to
> be retrieved. The two least significant bytes contain one of the following three constant values..."""
> 

True, However a language ID of 00 means vendor default. Which is what I am pretty sure we get out of the underlying calls. Anything of another language will be unimplemented and should return that code. 

-aric

>> +            TRACE("IOCTL_HID_GET_STRING[%i]\n", index);
>> +            irp->IoStatus.u.Status = status = ext->vtbl->get_string(device,
>> +                index, (WCHAR*)(irp->UserBuffer),
>> +                irpsp->Parameters.DeviceIoControl.OutputBufferLength/sizeof(WCHAR));
>> +            if (status == STATUS_SUCCESS)
>> +                irp->IoStatus.Information = (strlenW((WCHAR*)(irp->UserBuffer)) + 1) * sizeof(WCHAR);
>> +            break;
>> +        }
>>          default:
>>          {
>>              ULONG code = irpsp->Parameters.DeviceIoControl.IoControlCode;
>>
>>
>>
>