From 26da6daaa96a6d45187e5a2ff519676e9e772cb2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?P=C3=A9ter=20Szil=C3=A1gyi?= Date: Mon, 20 Mar 2017 13:35:56 +0200 Subject: [PATCH 1/2] accounts/usbwallet: fix Ledger hidapi/libusb protocol violation --- accounts/usbwallet/ledger_hub.go | 43 ++++++++++++++++++++--------- accounts/usbwallet/ledger_wallet.go | 6 ++++ 2 files changed, 36 insertions(+), 13 deletions(-) diff --git a/accounts/usbwallet/ledger_hub.go b/accounts/usbwallet/ledger_hub.go index db874f4cce..ed992a82dd 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 698e85f48b..97434ed3bd 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) } From 8ff7e55ab547e6c915a74971126f64b7582f2b77 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?P=C3=A9ter=20Szil=C3=A1gyi?= Date: Thu, 23 Mar 2017 17:04:39 +0200 Subject: [PATCH 2/2] accounts/usbwallet: if a confirmation is pending, skip refresh --- accounts/usbwallet/ledger_hub.go | 9 ++++++++- accounts/usbwallet/ledger_wallet.go | 10 ++++++++-- 2 files changed, 16 insertions(+), 3 deletions(-) diff --git a/accounts/usbwallet/ledger_hub.go b/accounts/usbwallet/ledger_hub.go index ed992a82dd..fcbc24c0f5 100644 --- a/accounts/usbwallet/ledger_hub.go +++ b/accounts/usbwallet/ledger_hub.go @@ -58,7 +58,10 @@ type LedgerHub struct { quit chan chan error 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) + + // 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. @@ -109,6 +112,10 @@ func (hub *LedgerHub) refreshWallets() { // 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 { diff --git a/accounts/usbwallet/ledger_wallet.go b/accounts/usbwallet/ledger_wallet.go index 97434ed3bd..f1beebb2c9 100644 --- a/accounts/usbwallet/ledger_wallet.go +++ b/accounts/usbwallet/ledger_wallet.go @@ -579,9 +579,15 @@ func (w *ledgerWallet) SignTx(account accounts.Account, tx *types.Transaction, c // 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() + 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) }