null pointer dereference in usb.core.Device::_open_endpoint()

[samblenny]

I think I found a potential race condition + null pointer dereference in usb.core.Device.

I've been looking at shared-module/usb/core/Device.c with an eye towards improving error handling for quirky USB devices (PR in progress). Along the way, I found this code in _open_endpoint(...):

    if (self->configuration_descriptor == NULL) {
        mp_raise_usb_core_USBError(MP_ERROR_TEXT("No configuration set"));
    }

    tusb_desc_configuration_t *desc_cfg = (tusb_desc_configuration_t *)self->configuration_descriptor;

    uint32_t total_length = tu_le16toh(desc_cfg->wTotalLength);
    uint8_t const *desc_end = ((uint8_t const *)desc_cfg) + total_length;
    uint8_t const *p_desc = tu_desc_next(desc_cfg);

I may be misunderstanding how this works. But, by my reading, unless mp_raise_usb_core_USBError() is using some strange magic to adjust the call stack to interrupt the normal control flow, it looks like this code is:

1. Checking for a null pointer
2. Informing the MicroPython core of the exception condition if a null was found
3. Attempting to dereference the pointer regardless of whether or not it just determined the pointer was null

Why I think this is worth attention

Short version: Lots of USB devices have weird quirks, so to get to a point of "It Just Works" for using USB devices on CircuitPython, the USB code might need to be a bit more defensive than other areas of the codebase.

Long version: Most the time, I can get my CircuitPython code to recover from an unplugged USB device by catching the exception, calling usb.core.find() again, and plugging the device back in. But, if I unplug and replug enough times, with just the right timing, eventually I can get the code to lock up. When that happens, it never finds a new device until I reset my board. The symptoms kinda look like what might happen if a null pointer was dereferenced.

At first glance, it seems like the obvious solution is to just not do that... like, just plug in the USB device normally and then leave it alone. But, the catch is, certain USB devices (e.g. 8BitDo Wireless Bluetooth USB Adapter 2 which I really want to get working), have firmware that changes the vendor and product ID depending on what mode the device is in. So, in normal use, such devices act like someone has rapidly unplugged one device and plugged in a different one.

Plans for a PR

I plan to make a PR to fix this. It should just take a one-liner with a return 0;. But, I'd be interested in feedback on how people feel about spending the code size increase that it would take to have robust null pointer checks for everything in usb.core.Device. There are many functions with a usb_core_device_obj_t *self parameter that gets accessed with stuff like self->whatever without any null pointer check. I'm new to TinyUSB, so maybe my intuition is off. But, I'd expect that there might be circumstances when structs, or fields within a struct, might become null due to a device being unplugged, or the like.