hughsie / libgusb

GUsb is a GObject wrapper for libusb1
GNU Lesser General Public License v2.1
25 stars 21 forks source link

context: Do not handle hotplug events after dispose #57

Closed benzea closed 2 years ago

benzea commented 2 years ago

Hotplug events may be queued at any time. They kept the object alive, but an explicit expose would mean that the event would be processed after dispose but before finalize.

Change the code to instead use a weak pointer. This causes the handling to be aborted if the object has been disposed in the meantime.

We could also simply check whether the internal state is acceptable at the time (e.g. check thread_event_run). However, using weak pointers has the advantage that we'll dispose/finalize earlier if the API user is just doing an unref, making the API more predictable.

This fixes commit 24934619a2ad ("Fix hotplug after threaded device removal") which in turn was a fix for commit 4b52b0cd27fd ("context: Fix hotplug handling and signal emission during enumerate")

benzea commented 2 years ago

I have not tested this yet beyond compiling it. But, I think this should be good (and I believe dispose clears the weak pointers already).

My current reproducer is a test run in the libfprint CI. No idea why it is triggering the issue now. Note that libfprint does an explicit dispose https://gitlab.freedesktop.org/libfprint/libfprint/-/jobs/23952384#L420. As such, I might be able to test it there on that runner by injecting another libusb build.

Maybe I can come up with a umockdev based reproducer for this.

benzea commented 2 years ago

So, the test verifies that the fix is correct.

But, it crashes in g_usb_context_find_by_bus_address as g_usb_context_add_device calls it. The funny thing is, the libfprint test case does not crash, which points to priv->done_enumerate being FALSE. I don't have a good explanation for that, but I suppose that might be a memory corruption, even if it is quite consistent.

So, not 100% certain this fixes the random libfprint failure. But, pretty certain there is a bugfix here.

hughsie commented 2 years ago

I'm a bit confused; libusb_hotplug_deregister_callback() gets called, we join the thread, and then the callback gets triggered?

benzea commented 2 years ago

I'm a bit confused; libusb_hotplug_deregister_callback() gets called, we join the thread, and then the callback gets triggered?

That code is fine (and libusb_hotplug_deregister_callback() wakes the thread, so the join is done immediately).

The problem is that the libusb callback pushes things into an idle handler (to run in the correct thread). And dispatching of the idler handler might happen after dispose runs. So, we get:

benzea commented 2 years ago

Obsoleted by #58.