accounts/usbwallet: fix Ledger hidapi/libusb protocol violation

2 files changed, 36 insertions, 13 deletions

diff --git a/accounts/usbwallet/ledger_hub.go b/accounts/usbwallet/ledger_hub.go
index db874f4cc..ed992a82d 100644
--- a/accounts/usbwallet/ledger_hub.go
+++ b/accounts/usbwallet/ledger_hub.go
@@ -22,6 +22,7 @@ package usbwallet
 
 import (
 	"errors"
+	"runtime"
 	"sync"
 	"time"
 
@@ -55,7 +56,9 @@ type LedgerHub struct {
 	updating    bool                    // Whether the event notification loop is running
 
 	quit chan chan error
-	lock sync.RWMutex
+
+	stateLock sync.RWMutex // Protects the internals of the hub from racey access
+	commsLock sync.RWMutex // Allows wallets to lock enumeration (TODO(karalabe): remove if hotplug lands on Windows)
 }
 
 // NewLedgerHub creates a new hardware wallet manager for Ledger devices.
@@ -76,8 +79,8 @@ func (hub *LedgerHub) Wallets() []accounts.Wallet {
 	// Make sure the list of wallets is up to date
 	hub.refreshWallets()
 
-	hub.lock.RLock()
-	defer hub.lock.RUnlock()
+	hub.stateLock.RLock()
+	defer hub.stateLock.RUnlock()
 
 	cpy := make([]accounts.Wallet, len(hub.wallets))
 	copy(cpy, hub.wallets)
@@ -88,15 +91,25 @@ func (hub *LedgerHub) Wallets() []accounts.Wallet {
 // list of wallets based on the found devices.
 func (hub *LedgerHub) refreshWallets() {
 	// Don't scan the USB like crazy it the user fetches wallets in a loop
-	hub.lock.RLock()
+	hub.stateLock.RLock()
 	elapsed := time.Since(hub.refreshed)
-	hub.lock.RUnlock()
+	hub.stateLock.RUnlock()
 
 	if elapsed < ledgerRefreshThrottling {
 		return
 	}
 	// Retrieve the current list of Ledger devices
 	var ledgers []hid.DeviceInfo
+
+	if runtime.GOOS == "linux" {
+		// hidapi on Linux opens the device during enumeration to retrieve some infos,
+		// breaking the Ledger protocol if that is waiting for user confirmation. This
+		// is a bug acknowledged at Ledger, but it won't be fixed on old devices so we
+		// need to prevent concurrent comms ourselves. The more elegant solution would
+		// be to ditch enumeration in favor of hutplug events, but that don't work yet
+		// on Windows so if we need to hack it anyway, this is more elegant for now.
+		hub.commsLock.Lock()
+	}
 	for _, info := range hid.Enumerate(0, 0) { // Can't enumerate directly, one valid ID is the 0 wildcard
 		for _, id := range ledgerDeviceIDs {
 			if info.VendorID == id.Vendor && info.ProductID == id.Product {
@@ -105,8 +118,12 @@ func (hub *LedgerHub) refreshWallets() {
 			}
 		}
 	}
+	if runtime.GOOS == "linux" {
+		// See rationale before the enumeration why this is needed and only on Linux.
+		hub.commsLock.Unlock()
+	}
 	// Transform the current list of wallets into the new one
-	hub.lock.Lock()
+	hub.stateLock.Lock()
 
 	wallets := make([]accounts.Wallet, 0, len(ledgers))
 	events := []accounts.WalletEvent{}
@@ -121,7 +138,7 @@ func (hub *LedgerHub) refreshWallets() {
 		}
 		// If there are no more wallets or the device is before the next, wrap new wallet
 		if len(hub.wallets) == 0 || hub.wallets[0].URL().Cmp(url) > 0 {
-			wallet := &ledgerWallet{url: &url, info: ledger, log: log.New("url", url)}
+			wallet := &ledgerWallet{hub: hub, url: &url, info: ledger, log: log.New("url", url)}
 
 			events = append(events, accounts.WalletEvent{Wallet: wallet, Arrive: true})
 			wallets = append(wallets, wallet)
@@ -140,7 +157,7 @@ func (hub *LedgerHub) refreshWallets() {
 	}
 	hub.refreshed = time.Now()
 	hub.wallets = wallets
-	hub.lock.Unlock()
+	hub.stateLock.Unlock()
 
 	// Fire all wallet events and return
 	for _, event := range events {
@@ -152,8 +169,8 @@ func (hub *LedgerHub) refreshWallets() {
 // receive notifications on the addition or removal of Ledger wallets.
 func (hub *LedgerHub) Subscribe(sink chan<- accounts.WalletEvent) event.Subscription {
 	// We need the mutex to reliably start/stop the update loop
-	hub.lock.Lock()
-	defer hub.lock.Unlock()
+	hub.stateLock.Lock()
+	defer hub.stateLock.Unlock()
 
 	// Subscribe the caller and track the subscriber count
 	sub := hub.updateScope.Track(hub.updateFeed.Subscribe(sink))
@@ -182,12 +199,12 @@ func (hub *LedgerHub) updater() {
 		hub.refreshWallets()
 
 		// If all our subscribers left, stop the updater
-		hub.lock.Lock()
+		hub.stateLock.Lock()
 		if hub.updateScope.Count() == 0 {
 			hub.updating = false
-			hub.lock.Unlock()
+			hub.stateLock.Unlock()
 			return
 		}
-		hub.lock.Unlock()
+		hub.stateLock.Unlock()
 	}
 }
diff --git a/accounts/usbwallet/ledger_wallet.go b/accounts/usbwallet/ledger_wallet.go
index 698e85f48..97434ed3b 100644
--- a/accounts/usbwallet/ledger_wallet.go
+++ b/accounts/usbwallet/ledger_wallet.go
@@ -83,6 +83,7 @@ var errInvalidVersionReply = errors.New("invalid version reply")
 
 // ledgerWallet represents a live USB Ledger hardware wallet.
 type ledgerWallet struct {
+	hub *LedgerHub    // USB hub the device originates from (TODO(karalabe): remove if hotplug lands on Windows)
 	url *accounts.URL // Textual URL uniquely identifying this wallet
 
 	info    hid.DeviceInfo // Known USB device infos about the wallet
@@ -576,6 +577,11 @@ func (w *ledgerWallet) SignTx(account accounts.Account, tx *types.Transaction, c
 	<-w.commsLock
 	defer func() { w.commsLock <- struct{}{} }()
 
+	// Ensure the device isn't screwed with while user confirmation is pending
+	// TODO(karalabe): remove if hotplug lands on Windows
+	w.hub.commsLock.RLock()
+	defer w.hub.commsLock.RUnlock()
+
 	return w.ledgerSign(path, account.Address, tx, chainID)
 }