thegecko / webusb

Node.js implementation of the WebUSB Specification
https://thegecko.github.io/webusb/
MIT License
183 stars 27 forks source link

devicetoUSBDevice crash in device.allConfigDescriptors when non-WebUSB #21

Closed abeck70 closed 6 years ago

abeck70 commented 6 years ago

Hi Rob,

I've appreciated you making WebUSB/node available. This issue is most likely libusb or node-usb. In 1.0.4 you allowed loadDevice to return non-WebUSB devices. I find this crashes in the call to device.allConfigDescriptors in deviceToUSBDevice. The scenario was a stmf7 device jumping out to the stm system boot loader.

Any tips or guidance would be appreciated if you encountered the same issue.

Thanks Alex

thegecko commented 6 years ago

Hi Alex,

I've seen this problem and thought I had it fixed in https://github.com/thegecko/webusb/commit/28a5b53d8a069834c6d021406ae06e24b3575dda.

Could you confirm you are using version 1.0.8?

abeck70 commented 6 years ago

Hi Rob,

So I have reproduced the issued with 1.0.8 but I was using a my own version that is equivalent. I simply changed the vendorId in examples/selector.js and ran node ./examples/selector.js with node v8.11.2

This is all I get back from node: $ node ./examples/selector.js FATAL ERROR: v8::ToLocalChecked Empty MaybeLocal. 1: node_module_register 2: v8::V8::ToLocalEmpty 3: 00007FFE41206573 4: 00007FFE41207546 5: 00007FFE41201196 6: v8::internal::wasm::SignatureMap::Find 7: v8::internal::Builtins::CallableFor 8: v8::internal::Builtins::CallableFor 9: v8::internal::Builtins::CallableFor 10: 0000008F04B043C1

If I re-add the !capability check in loadDevice it works perfectly with my webusb device.

My own logging additions revealed this crash occurs on the call to device.allConfigDescriptors for this particular device as follows:

im4d-webusb:error getBosDescriptor: bcdUSB < 2.0.1 +2ms im4d-webusb:info loadDevice: no capability device Device { busNumber: 1, deviceAddress: 1, deviceDescriptor: { bLength: 18, bDescriptorType: 1, bcdUSB: 0, bDeviceClass: 9, bDeviceSubClass: 0, bDeviceProtocol: 0, bMaxPacketSize0: 0, idVendor: 32902, idProduct: 41647, bcdDevice: 0, iManufacturer: 0, iProduct: 0, iSerialNumber: 0, bNumConfigurations: 1 }, interfaces: null } +1ms im4d-webusb:info devicetoUSBDevice 1 +0ms im4d-webusb:info devicetoUSBDevice prior to allConfigDescriptors +2ms FATAL ERROR: v8::ToLocalChecked Empty MaybeLocal. 1: node_module_register 2: v8::V8::ToLocalEmpty 3: 00007FFE31DB65A3 4: 00007FFE31DB7576 5: 00007FFE31DB1196 6: v8::internal::wasm::SignatureMap::Find 7: v8::internal::Builtins::CallableFor 8: v8::internal::Builtins::CallableFor 9: v8::internal::Builtins::CallableFor 10: 0000027B99B843C1

I am not sure what this intel device is, and I suspect I need to filter this out somehow: 8086:a2af (bus 1, device 0) 2109:0812 (bus 1, device 5) path: 20 05ac:024f (bus 1, device 9) path: 10.3.2.2 05ac:1006 (bus 1, device 7) path: 10.3.2 PS D:\SoftwareInstalls\examples\bin64> .\xusb.exe 8086:a2af Using libusb v1.0.22.11312

Opening device 8086:A2AF... Failed.

The device is "Intel USB 3.0 eXtensible Host Controller - 1.0" which just so happens to return a bcdUSB value of zero, making it easy enough to filter out.

Once this device is filtered out it all works! select a device to see it's active configuration: 1: STM32 BOOTLOADER { "manufacturerName": "STMicroelectronics", "productName": "STM32 BOOTLOADER", "serialNumber": "STM32FxSTM32", "_configurations": [ { "configurationValue": 1, "configurationName": "Љ", "interfaces": "[1...]" } ], "_currentConfiguration": 1, "url": null, "_maxPacketSize": 64, "_handle": "18", "usbVersionMajor": 1, "usbVersionMinor": 0, "usbVersionSubminor": 0, "deviceClass": 0, "deviceSubclass": 0, "deviceProtocol": 0, "vendorId": 1155, "productId": 57105, "deviceVersionMajor": 22, "deviceVersionMinor": 0, "deviceVersionSubminor": 0

Regards, Alex.

thegecko commented 6 years ago

I think the problem boils down to there not being a guard in the node-usb package when the call to allConfigDescriptors is made:

https://github.com/tessel/node-usb/blob/master/usb.js#L55

It assumes all devices have that config and some clearly don't.

It's difficult to pin this down without a similar device, but I've added a few more guards in https://github.com/thegecko/webusb/commit/575e911e5c8590fad9cf5190d2bb8df5b0a0d426.

Could you try the latest master?

abeck70 commented 6 years ago

Hi Rob, It still crashes in the same place. $ node ./examples/selector.js FATAL ERROR: v8::ToLocalChecked Empty MaybeLocal. 1: node_module_register 2: v8::V8::ToLocalEmpty 3: 00007FFE40CF6573 4: 00007FFE40CF7546 5: 00007FFE40CF1196 6: v8::internal::wasm::SignatureMap::Find 7: v8::internal::Builtins::CallableFor 8: v8::internal::Builtins::CallableFor 9: v8::internal::Builtins::CallableFor 10: 00000145088043C1

Adding a try catch around allConfigDescriptors won't help for this issue. This offending device needs filtering out at an earlier stage. The only idea I have with my limited knowledge is the bcdUSB should be non-zero.

Regards Alex.

thegecko commented 6 years ago

Can you outline the exact line where webusb crashes?

abeck70 commented 6 years ago

Hi Rob,

Yes its the call to allConfigDescriptors

I have re-created the crash with node-usb's full-test. From this I observe the first fails in a controlled manner:

it 'should have a configDescriptor property', ->
    assert.ok(device.configDescriptor != undefined)

2) Device should have a configDescriptor property: Error: LIBUSB_ERROR_NOT_FOUND at Device.get (C:\Users\abeck\ngr\node-usb\usb.js:49:67) at Context. (C:\Users\abeck\ngr\node-usb\test\usb.coffee:85:31) at Test.Runnable.run (C:\Users\abeck\ngr\node-usb\node_modules\mocha\lib\runnable.js:213:32) at Runner.runTest (C:\

it 'should have a allConfigDescriptors property', ->
    assert.ok(device.allConfigDescriptors != undefined)

Where as this crashes

.........FATAL ERROR: v8::ToLocalChecked Empty MaybeLocal. 1: node_module_register 2: v8::V8::ToLocalEmpty 3: 00007FFE353265A3 4: 00007FFE35327AB6 5: 00007FFE35321196 6: v8::internal::wasm::SignatureMap::Find 7: v8::internal::Builtins::CallableFor 8: v8::internal::Builtins::CallableFor 9: v8::internal::Builtins::CallableFor 10: 00000267894843C1

So perhaps its a valid idea to access configDescriptor before accessing allConfigDescriptors. It will fail gracefully on configDescriptor.

Alex.

abeck70 commented 6 years ago

Rob As per above suggestion this code edit solves the problem

private devicetoUSBDevice(handle: string): Promise { return new Promise((resolve, _reject) => { const device = this.devices[handle].device; const url = this.devices[handle].url;

        let configs: Array<ConfigDescriptor> = null;
        let configDescriptor: ConfigDescriptor = null;
        let deviceDescriptor: DeviceDescriptor = null;

        try {
            configDescriptor = device.configDescriptor;
            deviceDescriptor = device.deviceDescriptor;
        } catch (_e) {
            return resolve(null);
        }

        try {
            configs = device.allConfigDescriptors;
        } catch (_e) {
            return resolve(null);
        }

Alex.

thegecko commented 6 years ago

Nice one Alex!

Do you want to open a PR and I'll merge and release it?

abeck70 commented 6 years ago

ok sure will do now

thegecko commented 6 years ago

Fixed in #22 and released in v1.0.9