[libusb] libusb_get_string_descriptor_ascii is crashing in windows 7

[Christopher Flordeliza]

Hi All,

libusb_get_string_descriptor_ascii throw an illegal access exception when called after I unplugged and plugged my USB device. I revised the listdev project in the libusb workspace to:

const libusb_device_handle* Open(const libusb_context* context)
{
struct libusb_device_descriptor deviceDescriptor;
libusb_device_handle* handle = NULL;
/* list devices */
libusb_device **devices = 0;

ssize_t deviceCount = libusb_get_device_list(context, &devices);
for (int index = 0; index < deviceCount; index++)
{
libusb_device *currentDevice = devices[index];
if ((libusb_get_device_descriptor(currentDevice, &deviceDescriptor) == 0)
&& (deviceDescriptor.idVendor == 0x0525)
&& (deviceDescriptor.idProduct == 0xA4A4))
{
if (0 == libusb_open(currentDevice, &handle))
{
unsigned char sn_ascii[14];
sn_ascii[13] = 0;
int snlen = 0;
// Match serial number if a serial number was specified
if (0 < (snlen = libusb_get_string_descriptor_ascii(handle, deviceDescriptor.iSerialNumber,
sn_ascii, sizeof(sn_ascii))))
{
printf(" SERIAL: %s ---", sn_ascii);
}
}
}
}

libusb_free_device_list(devices, 1);

return handle;
}
static boolean Close(const libusb_context* context, const libusb_device_handle* handle)
{
boolean result = TRUE;

libusb_close(handle);

return result;
}
int main(void)
{
int result = 0;
int testMax = 100000;
int testCounter = 1;
int r;
ssize_t cnt;
libusb_context* context = NULL;
const libusb_device_handle* handle = NULL;

if (libusb_init(&context) < 0)
{
return -1;
}
do
{
handle = Open(context);
printf("Test #%08d: %s!\n", testCounter, handle ? "SUCCESS" : "FAILED");
if (handle)
{
Close(context, handle);
}
testCounter++;
} while (testCounter <= testMax);

libusb_exit(context);
return result;
}

The crash happens frequently in Windows 7 (64bit) and seldom in Windows 10 (64bit). Please note, the code was compiled in windows 10 and was run in Windows 7. To reproduce the problem:
1. Compile the above code in Windows 10 (Win32)
2. Run the exe in Windows 7
3. While running, unplug the USB device
4. Plug again the USB device, crash happens.

I changed libusb_free_device_list(devices, 1); to libusb_free_device_list(devices, 0); to fix the crash. After running it again many times the crash only happened 1 time, I am not sure if the cause of the crash is already fixed by changing libusb_free_device_list(devices, 0).

Please help me in pointing out what I am doing wrong here.

Regards

Christopher

[Peter Stuge]

Hi Christopher,

Post by Christopher Flordeliza
libusb_get_string_descriptor_ascii throw an illegal access
exception when called after I unplugged and plugged my USB device.

That would be a bug in the library.
..

You're using the library the right way and it should not crash.


Unrelated to that I would like to suggest considering a different style.

rather than:

if (success_condition_one) {
if (success_condition_two) {
if (success_condition_three) {
/* do stuff */
}
}
}

try using this for a week:

if (!success_condition_one)
goto cleanup;

if (!success_condition_two)
goto cleanup;

if (!success_condition_three)
goto cleanup;

/* do stuff */

cleanup:

libusb_free_device_list(devices, 1);
return handle;


It makes the code significantly more readable; all the corner cases
are clearly spelled out before some stuff is done, and the stuff is
in the leftmost indent level of the function. If you look at the Linux
kernel source you'll find this style is used throughout.

goto in itself should not be considered harmful. That essay refers to
goto going out of scope, and I agree that that is harmful, but
forward goto for error handling is an excellent use.

Post by Christopher Flordeliza
I changed libusb_free_device_list(devices, 1); to
libusb_free_device_list(devices, 0); to fix the crash. After
running it again many times the crash only happened 1 time, I am
not sure if the cause of the crash is already fixed by changing
libusb_free_device_list(devices, 0).

Changing 1 to 0 in the libusb_free_device_list() call makes you leak
memory in libusb. This is probably OK as a workaround if your program
is short-lived, but for a long running program it is not a good solution.

If you want to continuously poll for new devices you could try using
a separate context for the polling, which you _init and _exit around
the calls to _get and _free_device_list(). You will need to keep track
of the context you used to open each device handle, so keep that in mind.


//Peter

[Christopher Flordeliza]

Hi Peter,

I tried using different contexts when doing discovery but it is still crashing. Right now we removed the call to libusb_get_string_descriptor_ascii to prevent the crash. Thank you very much for your suggestion to refactor the code.

Thanks again for the support, we appreciated it very much.

Regards,

Christopher

-----Original Message-----
From: Peter Stuge [mailto:***@stuge.se]
Sent: Monday, July 3, 2017 9:59 PM
To: libusb-***@lists.sourceforge.net
Subject: Re: [libusb] libusb_get_string_descriptor_ascii is crashing in windows 7

Hi Christopher,

Post by Christopher Flordeliza
libusb_get_string_descriptor_ascii throw an illegal access exception
when called after I unplugged and plugged my USB device.

That would be a bug in the library.
..

You're using the library the right way and it should not crash.


Unrelated to that I would like to suggest considering a different style.

rather than:

if (success_condition_one) {
if (success_condition_two) {
if (success_condition_three) {
/* do stuff */
}
}
}

try using this for a week:

if (!success_condition_one)
goto cleanup;

if (!success_condition_two)
goto cleanup;

if (!success_condition_three)
goto cleanup;

/* do stuff */

cleanup:

libusb_free_device_list(devices, 1);
return handle;


It makes the code significantly more readable; all the corner cases are clearly spelled out before some stuff is done, and the stuff is in the leftmost indent level of the function. If you look at the Linux kernel source you'll find this style is used throughout.

goto in itself should not be considered harmful. That essay refers to goto going out of scope, and I agree that that is harmful, but forward goto for error handling is an excellent use.

Post by Christopher Flordeliza
I changed libusb_free_device_list(devices, 1); to
libusb_free_device_list(devices, 0); to fix the crash. After running
it again many times the crash only happened 1 time, I am not sure if
the cause of the crash is already fixed by changing
libusb_free_device_list(devices, 0).

Changing 1 to 0 in the libusb_free_device_list() call makes you leak memory in libusb. This is probably OK as a workaround if your program is short-lived, but for a long running program it is not a good solution.

If you want to continuously poll for new devices you could try using a separate context for the polling, which you _init and _exit around the calls to _get and _free_device_list(). You will need to keep track of the context you used to open each device handle, so keep that in mind.


//Peter

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot _______________________________________________
libusb-devel mailing list
libusb-***@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/libusb-devel