search

JeffLIrion / adb_shell

A Python implementation of ADB with shell and FileSync functionality.
Apache License 2.0
530 stars 60 forks source link

fix: Mismanaged `usb1.USBContext` lifecycle #195

Closed maybe-sybr closed 2 years ago

maybe-sybr commented 2 years ago

This change reverts e7d8b59 to fix the lifecycle issue it introduced (which is probably a UAF condition and security relevant), and proposes the use of an idempotent USBContext object in the USBTransport class to ensure we don't leak potentially many of those objects by repeatedly instantiating them.

This may address #194 if merged. It certainly fixes (for me) the crashes I noted in that issue.

maybe-sybr commented 2 years ago

Now what I don't know is whether it's safe for us to maintain a long lived usb1 context object. I don't know enough about the library and those objects to know if using one for an arbitrarily long period of time is the right thing to do. AFAICT sitting in a loop scanning for ADB devices with my own pairing management script seems to behave quite happily as I plug and unplug a single device, ie. finding the device reliably when it is plugged in and not finding it when it is not plugged in.

maybe-sybr commented 2 years ago

I'm just going to say this isn't a draft. It needs some though but I don't think it needs more work unless the premise is totally wrong.

JeffLIrion commented 2 years ago

I'm not knowledgeable about libusb1, so I'll defer to your judgment. Let me know if this is ready to be merged and if I should create a new release.

maybe-sybr commented 2 years ago

I'm not knowledgeable about libusb1, so I'll defer to your judgment. Let me know if this is ready to be merged and if I should create a new release.

I'd like to hear back from @botherder if possible, but otherwise from my perspective the revert is definitely worth merging and the other commit should be a cheap fix for the memory leak and the deprecation warning I was initially trying to make go away. Let's see if we get any input from MVT folks in the next day, but otherwise just go ahead and get this in.

Thanks for the quick reply!

Edit: For the last few hours, I've been blacklisting 0.4.1 to get my system running on 0.4.0 again and everything is working quite happily again. So my crashes are indeed fixed at the very least.

botherder commented 2 years ago

I can confirm that <0.4.1 this issue did not occur. The changes make sense to me and seem like a cleaner way. I'm gonna do some more testing in the next hours.

botherder commented 2 years ago

Yes, I can confirm that reverting to 0.4.0 removes the issue altogether. I can also confirm that using this patch the issue seems to be resolved.

JeffLIrion commented 2 years ago

I released v0.4.2 just now.