windows: change MAX_STRING_WCHARS to 126 #627

vovkos · 2023-09-25T06:09:23Z

Win32 HID API doc says: For USB devices, the maximum string length is 126 wide characters (not including the terminating NULL character).

For certain USB devices, using a buffer larger or equal to 127 wchars results in successful completion of HID API functions, but a broken string is stored in the output buffer. This behaviour persists even if HID API is bypassed and HID IOCTLs are passed to the HID driver directly (IOCTL_HID_GET_MANUFACTURER_STRING, IOCTL_HID_GET_PRODUCT_STRING, etc).

So, the buffer MUST NOT exceed 126 wchars.

Win32 HID API doc says: For USB devices, the maximum string length is 126 wide characters (not including the terminating NULL character). For certain USB devices, using a buffer larger or equal to 127 wchars results in successful completion of HID API functions, but a broken string is stored in the output buffer. This behaviour persists even if HID API is bypassed and HID IOCTLs are passed to the HID driver directly (IOCTL_HID_GET_MANUFACTURER_STRING, IOCTL_HID_GET_PRODUCT_STRING, etc). So, the buffer MUST NOT exceed 126 wchars.

mcuee · 2023-09-25T08:56:26Z

What kind of USB HID device is showing such problem?

Will this PR cause side effects for other HID device like Bluetooth?

/* MAXIMUM_USB_STRING_LENGTH from usbspec.h is 255 */
/* BLUETOOTH_DEVICE_NAME_SIZE from bluetoothapis.h is 256 */
#define MAX_STRING_WCHARS 256

Reference: it seems to me Windows API allows the buffer size to be up to 4093 Bytes,
https://learn.microsoft.com/en-us/windows-hardware/drivers/ddi/hidsdi/nf-hidsdi-hidd_getindexedstring

Youw · 2023-09-25T09:45:31Z

a broken string is stored in the output buffer

Is this causing un undefined behavior (like reading uninitialized memory) or just is it more a security consern (getting data from some buffer in the memory which we wheren't supposed to get normally.

For certain USB devices

Is it specific to driver or specific to device descriptor?

For certain USB devices
the buffer MUST NOT exceed 126 wchars

Taking into account size limitation for other buses (e.g. Bluetooth) this parameter probably need to be bus-specific, e.g. have it as is (256) by default, and have it 126 for device bus is known and is USB.

vovkos · 2023-09-26T02:34:37Z

What kind of USB HID device is showing such problem?

I can't quite put my finger on what exactly triggers this behaviour, but here's an example (Windows10 Pro 21H2, Version 10.0.19044 Build 19044)

Before the patch:

---------------------------------------
Enumerating HIDAPI devices
---------------------------------------
Loading HID DB...
Enumerating HID devices...

... (skipped)

\\?\HID#VID_14A5&PID_A011#6&1e2f4adc&0&0000#{4d1e55b2-f16f-11cf-88cb-001111000030}
    VID:          14a5
    PID:          a011
    Manufacturer: U
    Product:      U
    Serial:       
    Release:      74
    Usage page:   Generic Desktop
    Usage:        Mouse
    Bus:          USB
    Interface:    0
    Instance:     HID\VID_14A5&PID_A011\6&1e2f4adc&0&0000
    DEVINST:      0x0004

... (skipped)

After the patch:

---------------------------------------
Enumerating HIDAPI devices
---------------------------------------
Loading HID DB...
Enumerating HID devices...

... (skipped)

\\?\HID#VID_14A5&PID_A011#6&1e2f4adc&0&0000#{4d1e55b2-f16f-11cf-88cb-001111000030}
    VID:          14a5
    PID:          a011
    Manufacturer: USB Gaming Mouse
    Product:      USB Gaming Mouse
    Serial:       
    Release:      74
    Usage page:   Generic Desktop
    Usage:        Mouse
    Bus:          USB
    Interface:    0
    Instance:     HID\VID_14A5&PID_A011\6&1e2f4adc&0&0000
    DEVINST:      0x0004

... (skipped)

Symptoms are as follows:

On return, the output buffer is zeroed except for the first letter; the first letter is pretty much random on every run (sometimes, it matches the first letter of the actual string descriptor).

Tried calling HidD_GetManufacturerString or even sending IOCTL_HID_GET_MANUFACTURER_STRING directly with the same result.

I was able to find similar reports from other users, for example: https://stackoverflow.com/questions/2329572/trouble-reading-manufacturer-string-from-hid-device-using-hid-dll-api.

In the case of my device, passing a 127 wchars long buffer still results in a broken output (126 wchars work).

Please note that all of the above only applies to this particular USB mouse (other HID devices work fine with the original 256 wchars long buffer).

Will this PR cause side effects for other HID device like Bluetooth?

That's a good question. HidD_GetManufacturerString and friends fail if the buffer is not big enough, so the patch could affect devices with string descriptor exceeding 126 wchars (are there even devices like that?).

To minimize possible impact, it probably makes sense to make this patch specific to the USB bus (i.e., only limit the buffer size for HID_API_BUS_USB.)

Reference: it seems to me Windows API allows the buffer size to be up to 4093 Bytes, https://learn.microsoft.com/en-us/windows-hardware/drivers/ddi/hidsdi/nf-hidsdi-hidd_getindexedstring

Well, the same page also states that USB string descriptors are limited to 126 wchars:

For USB devices, the maximum string length is 126 wide characters (not including the terminating NULL character).

The doc is a bit vague on whether it's OK to pass a 127 wchars long buffer to accommodate the NULL-terminator, but my experiments showed that it's not (fails for 127 wchars, works for 126 wchars).

vovkos · 2023-09-26T02:43:54Z

Is this causing un undefined behavior (like reading uninitialized memory) or just is it more a security consern (getting data from some buffer in the memory which we wheren't supposed to get normally.

It doesn't look like it's actually breaking anything or leading to a crash; just the output buffer is wrong (a random letter, then zeroes).

Is it specific to driver or specific to device descriptor?

Can't really tell whether it's caused by a particular device, USB driver, or HID driver (but not the Win32 HID library, because I tried passing HID IOCTLs directly). But again, it's not unique to my mouse -- there are similar reports on the web.

Taking into account size limitation for other buses (e.g. Bluetooth) this parameter probably need to be bus-specific, e.g. have it as is (256) by default, and have it 126 for device bus is known and is USB.

Good point. I'll make the changes and make this limitation specific to the USB bus.

windows/hid.c

Youw

other then my last commnet - looks good

…_internal_detect_bus_type(...) hid_internal_get_usb_info is now only responsible for detection of the bus type; rename it accordingly. Also, mixing an internal flag and DEV_INST into an ULONGLONG retval feels kinda hackish; use a cleaner approach instead (add an internal flag to help distinguishing between BLUETOOTH and BLE devices, then clear it once we are done).

…t now)

vovkos · 2023-09-29T06:54:27Z

The windows-cmake check still fails, but it has nothing to do with this PR (compilation errors in hid_report_reconstructor_test)

Youw · 2023-09-29T07:02:13Z

No worries. I'll override that check when it is time to merge.
I always give a few days for community to have a chance to review the PR even after it has been approved.

DJm00n · 2023-11-10T20:38:39Z

According to the USB 3.2 Revision 1.1 Specification. Section 9.6.9.:

The UNICODE string descriptor (shown in Table 9-31) is not NULL-terminated. The string length is computed by subtracting two from the value of the first byte of the descriptor.

So maximum size in BYTES is 255-2=253. Or 127 WCHARS with NULL-termination.

I like the idea of this workaround for possible Windows bug, but do not like this hid_internal_detect_bus_type_result in implementation. I think it will be better to add DEVINST hid_internal_get_devnode(const wchar_t* interface_path) instead.

Youw · 2023-11-10T20:51:56Z

better to add DEVINST hid_internal_get_devnode(const wchar_t* interface_path)

I agree this would do good from Single Responsibility principle,
but from implementation POV I believe it will be non-trivial to have a separate hid_internal_get_devnode and a function that would detect if the device is a BLE device w/o some code duplication.
I believe current aproach is a good balance in the overall design.

mcuee added the Windows Related to Windows backend label Sep 25, 2023

windows: only apply the 126 wchars limit to USB HID devices

3e9a836

vovkos force-pushed the master branch from 8dce42a to 3e9a836 Compare September 26, 2023 06:53

Youw reviewed Sep 26, 2023

View reviewed changes

windows/hid.c Outdated Show resolved Hide resolved

Youw reviewed Sep 26, 2023

View reviewed changes

vovkos added 2 commits September 27, 2023 16:32

windows: refactor hid_internal_detect_bus_type (bus flags are explici…

518e365

…t now)

Youw approved these changes Sep 28, 2023

View reviewed changes

windows: add a 'default' label to shut down -Wswitch warnings/errors

3c2d502

vovkos force-pushed the master branch from 62d9e76 to 3c2d502 Compare September 28, 2023 15:47

Youw added 2 commits March 4, 2024 12:52

code review

380434b

code review

ef87f71

Youw approved these changes Mar 4, 2024

View reviewed changes

Youw merged commit 4f2e91b into libusb:master Mar 4, 2024
14 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

windows: change MAX_STRING_WCHARS to 126 #627

windows: change MAX_STRING_WCHARS to 126 #627

vovkos commented Sep 25, 2023

mcuee commented Sep 25, 2023 •

edited

Loading

Youw commented Sep 25, 2023

vovkos commented Sep 26, 2023 •

edited

Loading

vovkos commented Sep 26, 2023

Youw left a comment

vovkos commented Sep 29, 2023

Youw commented Sep 29, 2023

DJm00n commented Nov 10, 2023

Youw commented Nov 10, 2023

windows: change MAX_STRING_WCHARS to 126 #627

windows: change MAX_STRING_WCHARS to 126 #627

Conversation

vovkos commented Sep 25, 2023

mcuee commented Sep 25, 2023 • edited Loading

Youw commented Sep 25, 2023

vovkos commented Sep 26, 2023 • edited Loading

vovkos commented Sep 26, 2023

Youw left a comment

Choose a reason for hiding this comment

vovkos commented Sep 29, 2023

Youw commented Sep 29, 2023

DJm00n commented Nov 10, 2023

Youw commented Nov 10, 2023

mcuee commented Sep 25, 2023 •

edited

Loading

vovkos commented Sep 26, 2023 •

edited

Loading