Skip to content

hw_wallet: fix crash on exit if device unpairing fails#10737

Open
sashazykov wants to merge 1 commit into
spesmilo:masterfrom
sashazykov:hw-close-wallet-thread-leak
Open

hw_wallet: fix crash on exit if device unpairing fails#10737
sashazykov wants to merge 1 commit into
spesmilo:masterfrom
sashazykov:hw-close-wallet-thread-leak

Conversation

@sashazykov

Copy link
Copy Markdown
Contributor

Quitting Electrum with a hardware wallet open crashes the process (SIGABRT) if the paired device was unplugged during the session.

HW_PluginBase.close_wallet unpaired the device before stopping the keystore TaskThread. Unpairing closes the hardware client, which does device I/O and raises if the device is gone:

E | plugin | Plugin error. plugin: trezor, hook: close_wallet
Traceback (most recent call last):
  File "electrum/plugin.py", line 833, in run_hook
    r = f(*args)
  File "electrum/hw_wallet/plugin.py", line 89, in close_wallet
    self.device_manager().unpair_pairing_code(keystore.pairing_code())
  File "electrum/plugin.py", line 1118, in unpair_pairing_code
    self._close_client(_id)
  File "electrum/plugin.py", line 1134, in _close_client
    client.close()
  ...
  File "electrum/plugins/trezor/clientbase.py", line 286, in close
    self.client.lock()
  ...
trezorlib.transport.bridge.BridgeException: trezord: acquire/62/null failed with code 400: device not found

run_hook() swallows the exception, so keystore.thread.stop() never runs. The still-running TaskThread is a QObject child of the wallet window, so at interpreter shutdown PyQt's atexit cleanup destroys the window and Qt aborts:

QThread: Destroyed while thread '' is still running

Fix

  • hw_wallet/plugin.py: stop the keystore thread before unpairing, so a device-I/O failure cannot leak a running thread. This ordering is also safer in itself: previously the client could be closed while a queued task was still using it.
  • plugin.py: DeviceMgr._close_client treats client.close() as best-effort and logs failures — a missing device is a normal condition during cleanup, and this also covers the other unpair callers (disconnect events, session timeouts).

Testing

  • Regression tests added in tests/plugins/test_hw_wallet.py.
  • Reproduced and verified on macOS with a Trezor One (open hw wallet → pair → unplug → quit): before, the traceback above followed by the Qt fatal and abort; after, a clean shutdown with the bridge failure logged:
    plugin.DeviceMgr | failed to close hardware client cleanly: BridgeException('trezord: acquire/64/null failed with code 400: device not found')

On wallet close, the close_wallet hook unpaired the device before
stopping the keystore TaskThread. Unpairing does device I/O and can
raise, e.g. if the device was unplugged while the wallet was open:

    Plugin error. plugin: trezor, hook: close_wallet
    Traceback (most recent call last):
      File "electrum/plugin.py", line 833, in run_hook
        r = f(*args)
      File "electrum/hw_wallet/plugin.py", line 89, in close_wallet
        self.device_manager().unpair_pairing_code(keystore.pairing_code())
      File "electrum/plugin.py", line 1118, in unpair_pairing_code
        self._close_client(_id)
      File "electrum/plugin.py", line 1134, in _close_client
        client.close()
      ...
      File "electrum/plugins/trezor/clientbase.py", line 286, in close
        self.client.lock()
      ...
    trezorlib.transport.bridge.BridgeException: trezord: acquire/62/null failed with code 400: device not found

run_hook() swallows the exception, so the thread was never stopped.
A still-running QThread (child of the wallet window) at interpreter
shutdown then makes Qt abort the process:

    QThread: Destroyed while thread '' is still running

Stop the thread before unpairing, and make DeviceMgr._close_client
treat client.close() as best-effort, as closing a missing device is
a normal condition during cleanup.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant