A wrong `usb_config_index` (1) caused a segfault with libusb-0.1 builds (libusb1 is okay)

[jimklimov]

I rebased on master and force-pushed an identical change to libusb0.c.
This was compile and runtime tested, but not confirmed to fix the issue since I can't repro. I intentionally set a wrong usb_hid_rep_index value in ups.conf and confirmed that things still broke as expected, though. A wrong usb_config_index (1) caused a segfault, didn't look too much into it though:

   0.268093	[D2] Trying to match device
   0.268094	[D2] match_function_subdriver (non-SHUT mode): matching a device...
   0.268095	[D3] match_function_regex: matching a device...
   0.268110	[D2] Device matches
   0.268118	[D3] nut_usb_set_altinterface: skipped usb_set_altinterface(udev, 0)
   0.271084	[D2] Retrieved HID descriptor (expected 9, got 9)
   0.271089	[D3] HID descriptor, method 1: (9 bytes) => 09 21 10 01 21 01 22 ae 01
   0.271090	[D3] HID descriptor length (method 1) 430
Segmentation fault

Originally posted by @tofurky in #2611 (comment)

[jimklimov]

@tofurky : did this only happen with the libusb-0.1 build for you, or also libusb-1.0 (how much pressing does this issue seem)?

[tofurky]

It doesn't happen with libusb-1.0, just 0.1. Some more info (I didn't want to turn my PR into a bug report):

valgrind:

   1.237519	[D2] Trying to match device
   1.237536	[D2] match_function_subdriver (non-SHUT mode): matching a device...
   1.237561	[D2] match_function_subdriver (non-SHUT mode): failed to match a subdriver to vendor and/or product ID
   1.237572	[D2] Device does not match - skipping
   1.237685	[D2] Checking device (0764/0501) (003/002)
   1.254150	[D2] - VendorID: 0764
   1.254172	[D2] - ProductID: 0501
   1.254185	[D2] - Manufacturer: CP1500PFCLCD
   1.254197	[D2] - Product: CRCA102#BJ1
   1.254209	[D2] - Serial Number: CPS
   1.254219	[D2] - Bus: 003
   1.254229	[D2] - Device: 002
   1.254240	[D2] - Device release number: 0001
   1.254250	[D2] Trying to match device
   1.254264	[D2] match_function_subdriver (non-SHUT mode): matching a device...
   1.254682	[D3] match_function_regex: matching a device...
   1.261460	[D2] Device matches
   1.261861	[D3] nut_libusb_set_altinterface: skipped usb_set_altinterface(udev, 0)
   1.264225	[D2] Retrieved HID descriptor (expected 9, got 9)
   1.264760	[D3] HID descriptor, method 1: (9 bytes) => 09 21 10 01 21 01 22 ae 01
   1.264944	[D3] HID descriptor length (method 1) 430
==685833== Invalid read of size 8
==685833==    at 0x136170: nut_libusb_open (libusb0.c:599)
==685833==    by 0x12868D: UnknownInlinedFun (usbhid-ups.c:1354)
==685833==    by 0x12868D: main (main.c:2457)
==685833==  Address 0x4bd1b10 is 16 bytes before a block of size 34 free'd
==685833==    at 0x484988F: free (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==685833==    by 0x4881FCD: usb_os_find_devices (in /usr/lib/x86_64-linux-gnu/libusb-0.1.so.4.4.4)
==685833==    by 0x488277C: usb_find_devices (in /usr/lib/x86_64-linux-gnu/libusb-0.1.so.4.4.4)
==685833==    by 0x13546E: nut_libusb_open (libusb0.c:317)
==685833==    by 0x12868D: UnknownInlinedFun (usbhid-ups.c:1354)
==685833==    by 0x12868D: main (main.c:2457)
==685833==  Block was alloc'd at
==685833==    at 0x4846828: malloc (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==685833==    by 0x4882030: usb_os_find_devices (in /usr/lib/x86_64-linux-gnu/libusb-0.1.so.4.4.4)
==685833==    by 0x488277C: usb_find_devices (in /usr/lib/x86_64-linux-gnu/libusb-0.1.so.4.4.4)
==685833==    by 0x13546E: nut_libusb_open (libusb0.c:317)
==685833==    by 0x12868D: UnknownInlinedFun (usbhid-ups.c:1354)
==685833==    by 0x12868D: main (main.c:2457)
==685833== 
==685833== Invalid read of size 8
==685833==    at 0x136174: nut_libusb_open (libusb0.c:599)
==685833==    by 0x12868D: UnknownInlinedFun (usbhid-ups.c:1354)
==685833==    by 0x12868D: main (main.c:2457)
==685833==  Address 0x0 is not stack'd, malloc'd or (recently) free'd
==685833== 
==685833== 
==685833== Process terminating with default action of signal 11 (SIGSEGV)
==685833==  Access not within mapped region at address 0x0
==685833==    at 0x136174: nut_libusb_open (libusb0.c:599)
==685833==    by 0x12868D: UnknownInlinedFun (usbhid-ups.c:1354)
==685833==    by 0x12868D: main (main.c:2457)
==685833==  If you believe this happened as a result of a stack
==685833==  overflow in your program's main thread (unlikely but
==685833==  possible), you can try to increase the size of the
==685833==  main thread stack using the --main-stacksize= flag.
==685833==  The main thread stack size used in this run was 8388608.
==685833== 
==685833== HEAP SUMMARY:
==685833==     in use at exit: 167,522 bytes in 371 blocks
==685833==   total heap usage: 578 allocs, 207 frees, 769,255 bytes allocated
==685833== 
==685833== LEAK SUMMARY:
==685833==    definitely lost: 0 bytes in 0 blocks
==685833==    indirectly lost: 0 bytes in 0 blocks
==685833==      possibly lost: 0 bytes in 0 blocks
==685833==    still reachable: 167,522 bytes in 371 blocks
==685833==         suppressed: 0 bytes in 0 blocks
==685833== Rerun with --leak-check=full to see details of leaked memory
==685833== 
==685833== For lists of detected and suppressed errors, rerun with: -s
==685833== ERROR SUMMARY: 2 errors from 2 contexts (suppressed: 0 from 0)
==685833== could not unlink /tmp/vgdb-pipe-from-vgdb-to-685833-by-root-on-???
==685833== could not unlink /tmp/vgdb-pipe-to-vgdb-from-685833-by-root-on-???
==685833== could not unlink /tmp/vgdb-pipe-shared-mem-vgdb-685833-by-root-on-???
Segmentation fault

gdb:

Program received signal SIGSEGV, Segmentation fault.
Downloading source file /usr/src/nut-2.8.2+git20240908-aquos/drivers/libusb0.c
nut_libusb_open (udevp=0x5555555b9c60 <udev>, curDevice=0x5555555b9c00 <curDevice.lto_priv.0>, matcher=0x5555555b98b0 <subdriver_matcher_struct>, callback=0x55555557fe30 <callback>)                                                         
    at /usr/src/nut-2.8.2+git20240908-aquos/drivers/libusb0.c:600
warning: 600	/usr/src/nut-2.8.2+git20240908-aquos/drivers/libusb0.c: No such file or directory
(gdb) bt full
#0  nut_libusb_open (udevp=0x5555555b9c60 <udev>, curDevice=0x5555555b9c00 <curDevice.lto_priv.0>, matcher=0x5555555b98b0 <subdriver_matcher_struct>, callback=0x55555557fe30 <callback>)
    at /usr/src/nut-2.8.2+git20240908-aquos/drivers/libusb0.c:600
        retries = <optimized out>
        rdlen1 = <optimized out>
        rdlen2 = -1
        m = <optimized out>
        dev = <optimized out>
        bus = 0x5555555cb730
        udev = <optimized out>
        iface = 0xc000403810507
        ret = <optimized out>
        res = <optimized out>
        buf = "\t!\020\001!\001\"\256\001\000\000\000\000\000\000\000\000\000\000"
        p = <optimized out>
        string = "CPS\000102#BJ1", '\000' <repeats 173 times>, "x\r\306\367\377\177\000\000 \311\377\377\377\177\000\000"...
        i = 0
        count_open_EACCESS = 0
        count_open_errors = 0
        count_open_attempts = <optimized out>
        rdbuf = '\000' <repeats 136 times>, "\026\031\306\367\377\177\000\000\000\000\000\000\001\000\000\000\020\302\377\377\377\177\000\000 \311\377\377\377\177\000\000\004\000\000\000\000\000\000\000f\000\000\000\004\000\000\000\002\000\000\000\000\000\000\000\300C\340\367\377\177\000\000"...
        rdlen = <optimized out>
        busses = <optimized out>
        usb_hid_number_opts_parsed = 1
        __func__ = "nut_libusb_open"
#1  0x000055555557468e in upsdrv_initups () at /usr/src/nut-2.8.2+git20240908-aquos/drivers/usbhid-ups.c:1354
        ret = 0
        val = <optimized out>
        regex_array = {0x5555555c01b0 "0764", 0x5555555c02a0 "0501", 0x0, 0x0, 0x0, 0x0, 0x0}
        ret = <optimized out>
        val = <optimized out>
        regex_array = <optimized out>
        ipv = <optimized out>
        ipv = <optimized out>
        ipv = <optimized out>
        productLen = <optimized out>
        got_lbrb_log_delay_without_calibrating = <optimized out>
        got_onlinedischarge_calibration = <optimized out>
        got_onlinedischarge_log_throttle_sec = <optimized out>
        ipv = <optimized out>
#2  main (argc=<optimized out>, argv=<optimized out>) at /usr/src/nut-2.8.2+git20240908-aquos/drivers/main.c:2457
        new_uid = <optimized out>
        i = <optimized out>
        do_forceshutdown = <optimized out>
        update_count = 0
        cmd = <optimized out>
        oldpid = <optimized out>
        optstring = "+a:s:kDFBd:hx:Lqr:u:g:Vi:c:P:"

Seems line 600 of libusb0.c is this:

                        iface = &dev->config[usb_subdriver.usb_config_index].interface[usb_subdriver.hid_rep_index].altsetting[0];
                        for (i=0; i<iface->extralen; i+=iface->extra[i]) {
                                upsdebugx(4, "i=%d, extra[i]=%02x, extra[i+1]=%02x", i,
                                        iface->extra[i], iface->extra[i+1]);

                                if (i+9 <= iface->extralen
                                &&  iface->extra[i] >= 9
                                &&  iface->extra[i+1] == 0x21
                                ) {
                                        p = (usb_ctrl_char *)&iface->extra[i];
                                        upsdebug_hex(3, "HID descriptor, method 2", p, 9);
                                        rdlen2 = ((uint8_t)p[7]) | (((uint8_t)p[8]) << 8);
                                        break;
                                }
                        }

[tofurky]

Updated valgrind output above; didn't have dbgsym installed in the original comment.

[tofurky]

And also, to clarify - it only occurred because I intentionally specified an invalid usb_config_index to ensure that the changes in the PR weren't stepping on the override. So, not pressing at all - was just a bit surprised to see a segfault rather than it being gracefully handled.

[jimklimov]

So, comparing these lines for two implementations:

• nut/drivers/libusb0.c

  Line 599 in 6da2fd9

  iface = &dev->config[usb_subdriver.usb_config_index].interface[usb_subdriver.hid_rep_index].altsetting[0];

• nut/drivers/libusb1.c

  Line 666 in 6da2fd9

  if_desc = &(conf_desc->interface[usb_subdriver.hid_rep_index].altsetting[0]);

I can only guess the issue is about looking into array elements which are not there?..

In libusb1.c variant, it is fetched here:

• nut/drivers/libusb1.c

  Lines 498 to 500 in 6da2fd9

  	ret = libusb_get_config_descriptor(device,
  	(uint8_t)usb_subdriver.usb_config_index,
  	&conf_desc);

I do not see any practical equivalent in libusb0.c :\ I guess it is supposed to be populated as part of main loop which iterates dev instances

nut/drivers/libusb0.c

Lines 336 to 337 in 6da2fd9

	for (bus = busses; bus; bus = bus->next) {
	for (dev = bus->devices; dev; dev = dev->next) {

Maybe those have a field for max value of these arrays?

[jimklimov]

Seems to be this struct usb_device : https://www.kernel.org/doc/html/v6.1/driver-api/usb/usb.html

...
  struct usb_host_config *config;
  struct usb_host_config *actconfig;
...

• config
  all of the device’s configs

• actconfig
  the active configuration

Very helpful :\ no counter. Maybe gotta loop until a null entry?