XLink crashes app if any exceptions occur in XLink cpp files

[diablodale]

Xlink can easily crash due to the newer C++ code within it yet having no exception handling.

For example, the file usb_host.cpp has a lot of C++ code in it and most of it can throw exceptions. However, there is no exception handling in functions, e.g. getUSBDevices(), to prevent their doomed cascade upward into standard-C code...which causes an app crash/segfault. 💥

Since exceptions can be throw by trivial uses like std::string(...) or container::at(), I recommend that the top-level functions in all cpp files be marked noexcept. Then add try/catch to all needed places to catch any exceptions and return relevant error codes instead. All modern editors like vscode and analyzers provide clear feedback of functions marked noexcept yet calling things which can throw.

Setup

• all os, platforms, compilers
• XLink in v2.21.2, master, develop, and for many past versions

Repro

1. Edit usb_host.cpp and insert as the first line of the function getUSBDevices() the following: throw std::runtime_error("crash me");
2. config and build. I used VS2019.
3. attach a USB oak device
4. run tests/color_camera_node_test

Result

App immediately crashes.

Expected

App somehow gracefully fails, in this case it should report it could find no usb devices.

[themarpe]

This will likely best be addressed (for now) by doing function wide try/catch, until either the lib is C++ified or if more fine grained handling is added (though I assume most cases will be general issues, as libusb ret value handling is already fine grained)

[diablodale]

Got it. I will look into this late this week or next.

I intend to mark functions noexcept at the C/C++ boundaries. Also, use normal try/catch where needed around large blocks of code.

I hesitate in using the function-level try/catch as it is intended to be used on constructors since that's the only way to catch exceptions in their initializer lists. It is a rare language feature that dev's might overlook or not understand.

[diablodale]

This is straightforward though needs supporting code. In XLink today, I saw how I need to handle libusb while managing exceptions. For example...

1. refLibusbDeviceByName()
2. libusb_get_device_list(context, &devs) populates the device list
3. getLibusbDevicePath()
4. getLibusbDevicePath() throws exception

The exception handling has to be in refLibusbDeviceByName() because the handler needs to libusb_free_device_list(devs, 1)

Tracing all the code to find where to insert try/catch/freeResources is time consuming and error-prone. I recommend using a RAII wrapper; similar to a unique_ptr with custom deleter. I would prefer:

• https://github.com/microsoft/wil/wiki/RAII-resource-wrappers#wilunique_any works great...on Windows. It is feature rich and well tested but unique_any needs to be cut/isolated from the files that contain Windows-only derived classes
• https://en.cppreference.com/w/cpp/experimental/unique_resource (https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2019/p0052r10.pdf) will not be in std for years. Early impls https://github.com/Lastique/scope/blob/develop/include/boost/scope/unique_resource.hpp, https://github.com/offa/scope-guard/blob/master/include/unique_resource.h, https://github.com/okdshin/unique_resource
• Write/use a limited C++ class for parts of libusb that need it for exception handling. Several at https://www.google.com/search?q=libusb+c%2B%2B+wrapper

[diablodale]

At https://github.com/diablodale/XLink/commit/2cfde5b58ebce9cc5769dc84ce54f3eb632a901b is a wrapper class LibusbDeviceList for libusb_get_device_list/free. It is an STL container and can be used with language/library features. Changed XLink to use it and is good in casual testing.

TODO

1. Need wrapper for libusb_open/close, libusb_get_config_descriptor/free, maybe libusb_claim_interface/release.
2. Add the try/catch at the C/C++ boundaries as a minimum. Might have them in nested functions where there is value in having more specific error code/returns.
3. Choose if wrapper(s) use the factory approach like above commit...or use a constructor approach?

Factory approach (implemented in above commit)

• Caller makes a wrapper instance via static factory LibusbDeviceList::create(context).
• It returns a std::unique_ptr<LibusbDeviceList>
• C++ code in the wrapper can throw in extreme cases (e.g. no memory). Caller needs to catch these.
• If libusb code encounters an error, libusb generates error codes (it does not throw). Error codes are detected within the wrapper. An mvLog() entry is made and an empty unique_ptr is returned to the Caller. Caller can then check for an empty unique_ptr with if (!myuniqueptr) { handle_error() }. The libusb error code is not available to the Caller.

Constructor approach (alternative)

• Caller creates a wrapper instance via a constructor, e.g. LibusbDeviceList{context}, std::make_unique<LibusbDeviceList>(context), etc.
• All errors from C++ and libusb cause an exception to be thrown. The wrapper will check for libusb error codes and transform the libusb error code into an exception having the libusb error text and (optionally) the libusb error code. Then throw that exception. Caller needs to catch all exceptions.
• Given this, the LibusbDeviceList instance is either constructed and valid...or an exception was thrown.
• I'm unsure what code should make the mvLog() entry. Should that be in the wrapper? Or should the Caller do it withcatch() { mvLog("had an error: " + myexception.what()); }

[diablodale]

https://github.com/diablodale/XLink/commit/461e30ad4fbb437e6ec9dfd7df1237b326a22ea3 adds the Constructor-approach. I think I like this Constructor-approach more. It feels more "STL" where containers are created with constructors (not factories).

However, I also updated the Factory-approach to be non-throwing. The Caller does not need try/catch and can always test the result with if (!myuniqueptr) { handle_error() }

Any feedback on which approach is preferred?

[diablodale]

added PR to fix this issue. I need a few questions answered in the PR to move further. :-)

[diablodale]

I've pushed the last commit to draft PR above. It includes markdown and mermaid documentation for usage and object model for the libusb wrapper. Its really quite simple. 🔢 This wrapper replaces much of the old Xlink C-style code for its usb transport and fixes this issue.

Moving forward, this issue's fixes and more will usually be in my fork at https://github.com/diablodale/XLink/ I'll leave this issue open as the Luxonis fork does not have any fixes for this issue's crashes.