libusb: fix crash in hid_enumerate() caused by a stale device handle #526

Merged

imaami merged 1 commit from fix-stale-device-handle into master

2023-04-10 15:24:13 -05:00

imaami commented

2023-04-10 14:26:13 -05:00

(Migrated from github.com)

When hid_enumerate() iterates over the device list, it's possible that libusb_open() fails. If this occurs on the next round after a successful libusb_open() call, create_device_info_for_device() is passed the previous iteration's already closed device handle.

Fix the crash by setting the handle to NULL after libusb_close().

imaami commented

2023-04-10 14:36:45 -05:00

(Migrated from github.com)

FYI: I reworded the commit message slightly, hence the force-push.

imaami commented

2023-04-10 14:58:55 -05:00

(Migrated from github.com)

This may or may not be useful, but here's a backtrace from gdb. To be honest I didn't find this nearly as helpful as sprinkling a few printfs in hid_enumerate(), which is how I found this bug in the end.

The logic is pretty obvious when reading through hid_enumerate() and looking at what happens with the handle variable. The root cause is not cleaning up the value of handle after libusb_close(), and the more immediate cause is the fact that libusb_open() won't modify the destination variable on failure. These two combined lead to the situation where a stale handle may slip through the cracks.

#0  libusb_control_transfer (dev_handle=dev_handle@entry=0x7ffff002a010, bmRequestType=bmRequestType@entry=128 '\200', bRequest=bRequest@entry=6 '\006', wValue=wValue@entry=768, wIndex=wIndex@entry=0, data=data@entry=0x7ffff7d60690 "", wLength=64, timeout=1000) at /git/libusb/libusb/sync.c:106
        transfer = <optimized out>
        buffer = <optimized out>
        completed = 0
        r = <optimized out>
        __func__ = "libusb_control_transfer"
#1  0x00007ffff7fbe83b in libusb_get_string_descriptor (length=64, data=0x7ffff7d60690 "", langid=0, desc_index=0 '\000', dev_handle=0x7ffff002a010) at /usr/include/libusb-1.0/libusb.h:1857
No locals.
#2  is_language_supported (dev=dev@entry=0x7ffff002a010, lang=lang@entry=0) at /git/hidapi/libusb/hid.c:380
        buf = {0 <repeats 24 times>, 13904, 61440, 32767, 0, 26352, 61442, 32767, 0}
        len = <optimized out>
        i = <optimized out>
#3  0x00007ffff7fbf90c in get_usb_string (dev=dev@entry=0x7ffff002a010, idx=<optimized out>) at /git/hidapi/libusb/hid.c:423
        buf = "\000\000\000\000\000\000\000\000\nj\331\367\377\177\000\000 \000\000\0000\000\000\000\340\302\367\367\377\177\000\000\000\000\000\000\000\000\000\000 |\002\360\377\177\000\000\035\000\000\000\000\000\000\000\000\030\210\235-\021n\002urationV\300\001\000\000\000\000\000\000%\000\000\000\000\000\000\000\000\030\210\235-\021n\002\340y\002\360\377\177\000\000\003\000\000\000\000\000\000\000\001\000\000\000\000\000\000\000\320\f\326\367\377\177\000\000\001\000\000\000\000\000\000\000\001\000\000\000\000\000\000\000\a\000\000\000\000\000\000\000B[\340\367\377\177\000\000(\000\000\0000\000\000\000\200\f\326\367\377\177\000\000\300\v\326\367\377\177\000\000\000\030\210\235-\021n\002 \000\000\0000\000\000\000"...
        len = <optimized out>
        str = 0x0
        wbuf = L"G413 Carbon Mechanical Gaming Keyboard\000翿\xf7d608b8翿\x358fde8翿\xf7fc5788翿\006\000\000\000\xf7fbaf58翿\xf7fba4e8翿\xf7d60900翿\xf7d608f8翿\x3e5d118\000\xf7fc5788\000\000\000\000\000\xf7d93527翿\xf7fc4fb8翿\000\000\xf7fc4c30翿\xd63f7a02\000\xf7d60940翿\xf7fd4b38翿\003\000\000\000\005\000\xf7d608c0翿\xf7d60c1f翿\x9d881800\x26e112d\xf7d60b20翿\xf7d60b20翿\xf7d609b0翿\xf7da2374翿\xf7d60a40翿\000\000\xf7d608c0翿\xf7e2b5f8翿\xf7d608f8翿ÿ\000\xfbad8001翿\xf7d60b20翿\xf7d60b20翿\xf7d60b20翿\xf7d60b20翿\xf7d60b4c翿\xf7d60c1f翿\xf7d60b20翿\xf7d60c1f翿\000\000\000\000\000\000\000\000\000\000\005\200\000\000\xf7d90000翿\000\000\xf0003650翿\xf7d60d10翿\xffffffff\xffffffff\000\000\xf0000d18翿\xf7fdd29a翿\xffffffff\000\000\000\000\000\xf7f7c2e0翿\000\000\000\000\000\000\xf7d60a20翿\xf7d60cdb\x37007fff\x9d881800\x26e112d\xf7d60cd3翿\xf7d60cd3翿\xf7d60b10翿\xf7fc0006翿\xf7d60ba0翿\000\000\xf7d60a20翿\xf7e2b5f8翿\xf7d60bc0翿\b\000"...
        ic = <optimized out>
        inbytes = 0
        outbytes = 872
        res = <optimized out>
        inptr = 0x7ffff7d60b4e ""
        outptr = 0x7ffff7d60798 ""
        lang = 0
#4  0x00007ffff7fbfac3 in create_device_info_for_device (device=device@entry=0x7ffff0002300, handle=0x7ffff002a010, desc=desc@entry=0x7ffff7d60d90, config_number=1, interface_num=7) at /git/hidapi/libusb/hid.c:682
        cur_dev = 0x7ffff00266f0
#5  0x00007ffff7fbfbb0 in hid_enumerate (vendor_id=vendor_id@entry=0, product_id=product_id@entry=0) at /git/hidapi/libusb/hid.c:800
        tmp = <optimized out>
        intf_desc = 0x7ffff0001ec0
        intf = 0x7ffff002a080
        k = 0
        dev_vid = <optimized out>
        dev_pid = <optimized out>
        desc = {bLength = 18 '\022', bDescriptorType = 1 '\001', bcdUSB = 512, bDeviceClass = 239 '\357', bDeviceSubClass = 2 '\002', bDeviceProtocol = 1 '\001', bMaxPacketSize0 = 64 '@', idVendor = 3504, idProduct = 16796, bcdDevice = 3, iManufacturer = 3 '\003', iProduct = 1 '\001', 
          iSerialNumber = 0 '\000', bNumConfigurations = 1 '\001'}
        conf_desc = 0x7ffff0003a30
        j = 7
        res = -3
        devs = 0x7ffff0025cc0
        dev = 0x7ffff0002300
        handle = 0x7ffff002a010
        num_devs = <optimized out>
        i = 15
        root = 0x7ffff0033010
        cur_dev = 0x7ffff0028a50

This may or may not be useful, but here's a backtrace from gdb. To be honest I didn't find this nearly as helpful as sprinkling a few printfs in `hid_enumerate()`, which is how I found this bug in the end. The logic is pretty obvious when reading through `hid_enumerate()` and looking at what happens with the `handle` variable. The root cause is not cleaning up the value of `handle` after `libusb_close()`, and the more immediate cause is the fact that `libusb_open()` won't modify the destination variable on failure. These two combined lead to the situation where a stale handle may slip through the cracks. ``` #0 libusb_control_transfer (dev_handle=dev_handle@entry=0x7ffff002a010, bmRequestType=bmRequestType@entry=128 '\200', bRequest=bRequest@entry=6 '\006', wValue=wValue@entry=768, wIndex=wIndex@entry=0, data=data@entry=0x7ffff7d60690 "", wLength=64, timeout=1000) at /git/libusb/libusb/sync.c:106 transfer = <optimized out> buffer = <optimized out> completed = 0 r = <optimized out> __func__ = "libusb_control_transfer" #1 0x00007ffff7fbe83b in libusb_get_string_descriptor (length=64, data=0x7ffff7d60690 "", langid=0, desc_index=0 '\000', dev_handle=0x7ffff002a010) at /usr/include/libusb-1.0/libusb.h:1857 No locals. #2 is_language_supported (dev=dev@entry=0x7ffff002a010, lang=lang@entry=0) at /git/hidapi/libusb/hid.c:380 buf = {0 <repeats 24 times>, 13904, 61440, 32767, 0, 26352, 61442, 32767, 0} len = <optimized out> i = <optimized out> #3 0x00007ffff7fbf90c in get_usb_string (dev=dev@entry=0x7ffff002a010, idx=<optimized out>) at /git/hidapi/libusb/hid.c:423 buf = "\000\000\000\000\000\000\000\000\nj\331\367\377\177\000\000 \000\000\0000\000\000\000\340\302\367\367\377\177\000\000\000\000\000\000\000\000\000\000 |\002\360\377\177\000\000\035\000\000\000\000\000\000\000\000\030\210\235-\021n\002urationV\300\001\000\000\000\000\000\000%\000\000\000\000\000\000\000\000\030\210\235-\021n\002\340y\002\360\377\177\000\000\003\000\000\000\000\000\000\000\001\000\000\000\000\000\000\000\320\f\326\367\377\177\000\000\001\000\000\000\000\000\000\000\001\000\000\000\000\000\000\000\a\000\000\000\000\000\000\000B[\340\367\377\177\000\000(\000\000\0000\000\000\000\200\f\326\367\377\177\000\000\300\v\326\367\377\177\000\000\000\030\210\235-\021n\002 \000\000\0000\000\000\000"... len = <optimized out> str = 0x0 wbuf = L"G413 Carbon Mechanical Gaming Keyboard\000翿\xf7d608b8翿\x358fde8翿\xf7fc5788翿\006\000\000\000\xf7fbaf58翿\xf7fba4e8翿\xf7d60900翿\xf7d608f8翿\x3e5d118\000\xf7fc5788\000\000\000\000\000\xf7d93527翿\xf7fc4fb8翿\000\000\xf7fc4c30翿\xd63f7a02\000\xf7d60940翿\xf7fd4b38翿\003\000\000\000\005\000\xf7d608c0翿\xf7d60c1f翿\x9d881800\x26e112d\xf7d60b20翿\xf7d60b20翿\xf7d609b0翿\xf7da2374翿\xf7d60a40翿\000\000\xf7d608c0翿\xf7e2b5f8翿\xf7d608f8翿ÿ\000\xfbad8001翿\xf7d60b20翿\xf7d60b20翿\xf7d60b20翿\xf7d60b20翿\xf7d60b4c翿\xf7d60c1f翿\xf7d60b20翿\xf7d60c1f翿\000\000\000\000\000\000\000\000\000\000\005\200\000\000\xf7d90000翿\000\000\xf0003650翿\xf7d60d10翿\xffffffff\xffffffff\000\000\xf0000d18翿\xf7fdd29a翿\xffffffff\000\000\000\000\000\xf7f7c2e0翿\000\000\000\000\000\000\xf7d60a20翿\xf7d60cdb\x37007fff\x9d881800\x26e112d\xf7d60cd3翿\xf7d60cd3翿\xf7d60b10翿\xf7fc0006翿\xf7d60ba0翿\000\000\xf7d60a20翿\xf7e2b5f8翿\xf7d60bc0翿\b\000"... ic = <optimized out> inbytes = 0 outbytes = 872 res = <optimized out> inptr = 0x7ffff7d60b4e "" outptr = 0x7ffff7d60798 "" lang = 0 #4 0x00007ffff7fbfac3 in create_device_info_for_device (device=device@entry=0x7ffff0002300, handle=0x7ffff002a010, desc=desc@entry=0x7ffff7d60d90, config_number=1, interface_num=7) at /git/hidapi/libusb/hid.c:682 cur_dev = 0x7ffff00266f0 #5 0x00007ffff7fbfbb0 in hid_enumerate (vendor_id=vendor_id@entry=0, product_id=product_id@entry=0) at /git/hidapi/libusb/hid.c:800 tmp = <optimized out> intf_desc = 0x7ffff0001ec0 intf = 0x7ffff002a080 k = 0 dev_vid = <optimized out> dev_pid = <optimized out> desc = {bLength = 18 '\022', bDescriptorType = 1 '\001', bcdUSB = 512, bDeviceClass = 239 '\357', bDeviceSubClass = 2 '\002', bDeviceProtocol = 1 '\001', bMaxPacketSize0 = 64 '@', idVendor = 3504, idProduct = 16796, bcdDevice = 3, iManufacturer = 3 '\003', iProduct = 1 '\001', iSerialNumber = 0 '\000', bNumConfigurations = 1 '\001'} conf_desc = 0x7ffff0003a30 j = 7 res = -3 devs = 0x7ffff0025cc0 dev = 0x7ffff0002300 handle = 0x7ffff002a010 num_devs = <optimized out> i = 15 root = 0x7ffff0033010 cur_dev = 0x7ffff0028a50 ```

Youw (Migrated from github.com) approved these changes

2023-04-10 15:02:37 -05:00

Youw (Migrated from github.com) left a comment

Now when I look at the fix, my though - how come no one had hit this one until now?

Thanks!

Now when I look at the fix, my though - how come no one had hit this one until now? Thanks!

imaami commented

2023-04-10 15:07:59 -05:00

(Migrated from github.com)

Now when I look at the fix, my though - how come no one had hit this one until now?

Thanks!

Human brains are to blame, they're always causing stuff like this. :)

I think it went unnoticed because it seems to only show up when

not running as root - this makes it more likely that libusb_open() fails,
requiring specifically that first libusb_open() succeeds on iteration N, then on iteration N+1 it fails. If you only get failures on every round the pointer stays NULL until the end, and if the last N rounds are a streak of successful libusb_open() calls it also won't crash even if some of the first ones failed.

> Now when I look at the fix, my though - how come no one had hit this one until now? > > Thanks! Human brains are to blame, they're always causing stuff like this. :) I think it went unnoticed because it seems to only show up when 1. not running as root - this makes it more likely that `libusb_open()` fails, 2. requiring specifically that first `libusb_open()` succeeds on iteration N, then on iteration N+1 it fails. If you only get failures on every round the pointer stays `NULL` until the end, and if the last N rounds are a streak of successful `libusb_open()` calls it also won't crash even if some of the first ones failed.

👍 2