thww hidapi usage: try to mitigate some thread-safety issues - electrum - Electrum Bitcoin wallet
HTML git clone https://git.parazyd.org/electrum
DIR Log
DIR Files
DIR Refs
DIR Submodules
---
DIR commit 2cfa3bd6c82b5f2df0332307240fd202523cb522
DIR parent 98d2ab5bd6f47961316c607f2a7ccd01fe38c472
HTML Author: SomberNight <somber.night@protonmail.com>
Date: Fri, 17 Apr 2020 19:05:56 +0200
hww hidapi usage: try to mitigate some thread-safety issues
related: #6097
Diffstat:
M electrum/plugin.py | 32 ++++++++++++++++++++++++++++++-
M electrum/plugins/bitbox02/bitbox02… | 19 +++++++++++--------
M electrum/plugins/coldcard/coldcard… | 9 +++++----
M electrum/plugins/digitalbitbox/dig… | 14 ++++++++------
M electrum/plugins/hw_wallet/plugin.… | 3 +++
M electrum/plugins/ledger/ledger.py | 14 ++++++++------
6 files changed, 66 insertions(+), 25 deletions(-)
---
DIR diff --git a/electrum/plugin.py b/electrum/plugin.py
t@@ -30,6 +30,8 @@ import threading
import sys
from typing import (NamedTuple, Any, Union, TYPE_CHECKING, Optional, Tuple,
Dict, Iterable, List, Sequence)
+import concurrent
+from concurrent import futures
from .i18n import _
from .util import (profiler, DaemonThread, UserCancelled, ThreadJob, UserFacingException)
t@@ -321,6 +323,20 @@ class HardwarePluginToScan(NamedTuple):
PLACEHOLDER_HW_CLIENT_LABELS = {None, "", " "}
+# hidapi is not thread-safe
+# see https://github.com/signal11/hidapi/issues/205#issuecomment-527654560
+# https://github.com/libusb/hidapi/issues/45
+# https://github.com/signal11/hidapi/issues/45#issuecomment-4434598
+# https://github.com/signal11/hidapi/pull/414#issuecomment-445164238
+# It is not entirely clear to me, exactly what is safe and what isn't, when
+# using multiple threads...
+# For now, we use a dedicated thread to enumerate devices (_hid_executor),
+# and we synchronize all device opens/closes/enumeration (_hid_lock).
+# FIXME there are still probably threading issues with how we use hidapi...
+_hid_executor = None # type: Optional[concurrent.futures.Executor]
+_hid_lock = threading.Lock()
+
+
class DeviceMgr(ThreadJob):
'''Manages hardware clients. A client communicates over a hardware
channel with the device.
t@@ -367,9 +383,15 @@ class DeviceMgr(ThreadJob):
# locks: if you need to take multiple ones, acquire them in the order they are defined here!
self._scan_lock = threading.RLock()
self.lock = threading.RLock()
+ self.hid_lock = _hid_lock
self.config = config
+ global _hid_executor
+ if _hid_executor is None:
+ _hid_executor = concurrent.futures.ThreadPoolExecutor(max_workers=1,
+ thread_name_prefix='hid_enumerate_thread')
+
def with_scan_lock(func):
def func_wrapper(self: 'DeviceMgr', *args, **kwargs):
with self._scan_lock:
t@@ -636,7 +658,15 @@ class DeviceMgr(ThreadJob):
except ImportError:
return []
- hid_list = hid.enumerate(0, 0)
+ def hid_enumerate():
+ with self.hid_lock:
+ return hid.enumerate(0, 0)
+
+ hid_list_fut = _hid_executor.submit(hid_enumerate)
+ try:
+ hid_list = hid_list_fut.result()
+ except (concurrent.futures.CancelledError, concurrent.futures.TimeoutError) as e:
+ return []
devices = []
for d in hid_list:
DIR diff --git a/electrum/plugins/bitbox02/bitbox02.py b/electrum/plugins/bitbox02/bitbox02.py
t@@ -46,7 +46,7 @@ class BitBox02Client(HardwareClientBase):
# handler is a BitBox02_Handler, importing it would lead to a circular dependency
def __init__(self, handler: Any, device: Device, config: SimpleConfig, *, plugin: HW_PluginBase):
HardwareClientBase.__init__(self, plugin=plugin)
- self.bitbox02_device = None
+ self.bitbox02_device = None # type: Optional[bitbox02.BitBox02]
self.handler = handler
self.device_descriptor = device
self.config = config
t@@ -73,10 +73,11 @@ class BitBox02Client(HardwareClientBase):
return True
def close(self):
- try:
- self.bitbox02_device.close()
- except:
- pass
+ with self.device_manager().hid_lock:
+ try:
+ self.bitbox02_device.close()
+ except:
+ pass
def has_usable_connection_with_device(self) -> bool:
if self.bitbox_hid_info is None:
t@@ -91,7 +92,8 @@ class BitBox02Client(HardwareClientBase):
res = device_response()
except:
# Close the hid device on exception
- hid_device.close()
+ with self.device_manager().hid_lock:
+ hid_device.close()
raise
finally:
self.handler.finished()
t@@ -155,8 +157,9 @@ class BitBox02Client(HardwareClientBase):
return set_noise_privkey(privkey)
if self.bitbox02_device is None:
- hid_device = hid.device()
- hid_device.open_path(self.bitbox_hid_info["path"])
+ with self.device_manager().hid_lock:
+ hid_device = hid.device()
+ hid_device.open_path(self.bitbox_hid_info["path"])
self.bitbox02_device = bitbox02.BitBox02(
transport=u2fhid.U2FHid(hid_device),
DIR diff --git a/electrum/plugins/coldcard/coldcard.py b/electrum/plugins/coldcard/coldcard.py
t@@ -72,9 +72,9 @@ class CKCCClient(HardwareClientBase):
self.dev = ElectrumColdcardDevice(dev_path, encrypt=True)
else:
# open the real HID device
- import hid
- hd = hid.device(path=dev_path)
- hd.open_path(dev_path)
+ with self.device_manager().hid_lock:
+ hd = hid.device(path=dev_path)
+ hd.open_path(dev_path)
self.dev = ElectrumColdcardDevice(dev=hd, encrypt=True)
t@@ -127,7 +127,8 @@ class CKCCClient(HardwareClientBase):
def close(self):
# close the HID device (so can be reused)
- self.dev.close()
+ with self.device_manager().hid_lock:
+ self.dev.close()
self.dev = None
def is_initialized(self):
DIR diff --git a/electrum/plugins/digitalbitbox/digitalbitbox.py b/electrum/plugins/digitalbitbox/digitalbitbox.py
t@@ -77,10 +77,11 @@ class DigitalBitbox_Client(HardwareClientBase):
def close(self):
if self.opened:
- try:
- self.dbb_hid.close()
- except:
- pass
+ with self.device_manager().hid_lock:
+ try:
+ self.dbb_hid.close()
+ except:
+ pass
self.opened = False
t@@ -681,8 +682,9 @@ class DigitalBitboxPlugin(HW_PluginBase):
def get_dbb_device(self, device):
- dev = hid.device()
- dev.open_path(device.path)
+ with self.device_manager().hid_lock:
+ dev = hid.device()
+ dev.open_path(device.path)
return dev
DIR diff --git a/electrum/plugins/hw_wallet/plugin.py b/electrum/plugins/hw_wallet/plugin.py
t@@ -196,6 +196,9 @@ class HardwareClientBase:
def __init__(self, *, plugin: 'HW_PluginBase'):
self.plugin = plugin
+ def device_manager(self) -> 'DeviceMgr':
+ return self.plugin.device_manager()
+
def is_pairable(self) -> bool:
raise NotImplementedError()
DIR diff --git a/electrum/plugins/ledger/ledger.py b/electrum/plugins/ledger/ledger.py
t@@ -74,7 +74,8 @@ class Ledger_Client(HardwareClientBase):
return True
def close(self):
- self.dongleObject.dongle.close()
+ with self.device_manager().hid_lock:
+ self.dongleObject.dongle.close()
def timeout(self, cutoff):
pass
t@@ -184,13 +185,13 @@ class Ledger_Client(HardwareClientBase):
self.segwitSupported = self.nativeSegwitSupported or (firmwareInfo['specialVersion'] == 0x20 and versiontuple(firmware) >= versiontuple(SEGWIT_SUPPORT_SPECIAL))
if not checkFirmware(firmwareInfo):
- self.dongleObject.dongle.close()
+ self.close()
raise UserFacingException(MSG_NEEDS_FW_UPDATE_GENERIC)
try:
self.dongleObject.getOperationMode()
except BTChipException as e:
if (e.sw == 0x6985):
- self.dongleObject.dongle.close()
+ self.close()
self.handler.get_setup( )
# Acquire the new client on the next run
else:
t@@ -593,9 +594,10 @@ class LedgerPlugin(HW_PluginBase):
ledger = True
else:
return None # non-compatible interface of a Nano S or Blue
- dev = hid.device()
- dev.open_path(device.path)
- dev.set_nonblocking(True)
+ with self.device_manager().hid_lock:
+ dev = hid.device()
+ dev.open_path(device.path)
+ dev.set_nonblocking(True)
return HIDDongleHIDAPI(dev, ledger, BTCHIP_DEBUG)
def create_client(self, device, handler):