Merge pull request #3801 from karalabe/ledger-linux-confirm

accounts/usbwallet: fix Ledger hidapi/libusb protocol violation

accounts/usbwallet/ledger_hub.go

@@ -22,6 +22,7 @@ package usbwallet
 
 import (
 	"errors"
+	"runtime"
 	"sync"
 	"time"
 
@@ -55,7 +56,12 @@ 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
+
+	// TODO(karalabe): remove if hotplug lands on Windows
+	commsPend int        // Number of operations blocking enumeration
+	commsLock sync.Mutex // Lock protecting the pending counter and enumeration
 }
 
 // NewLedgerHub creates a new hardware wallet manager for Ledger devices.
@@ -76,8 +82,8 @@ func (hub *LedgerHub) Wallets() []accounts.Wallet {
 	// Make sure the list of wallets is up to date
 	hub.refreshWallets()
 
-	hub.lock.RLock()
+	hub.stateLock.RLock()
-	defer hub.lock.RUnlock()
+	defer hub.stateLock.RUnlock()
 
 	cpy := make([]accounts.Wallet, len(hub.wallets))
 	copy(cpy, hub.wallets)
@@ -88,15 +94,29 @@ 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()
+		if hub.commsPend > 0 { // A confirmation is pending, don't refresh
+			hub.commsLock.Unlock()
+			return
+		}
+	}
 	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 +125,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 +145,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 +164,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 +176,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()
+	hub.stateLock.Lock()
-	defer hub.lock.Unlock()
+	defer hub.stateLock.Unlock()
 
 	// Subscribe the caller and track the subscriber count
 	sub := hub.updateScope.Track(hub.updateFeed.Subscribe(sink))
@@ -182,12 +206,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()
 	}
 }

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,17 @@ 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.Lock()
+	w.hub.commsPend++
+	w.hub.commsLock.Unlock()
+
+	defer func() {
+		w.hub.commsLock.Lock()
+		w.hub.commsPend--
+		w.hub.commsLock.Unlock()
+	}()
 	return w.ledgerSign(path, account.Address, tx, chainID)
 }