Fix race condition on device access

package.json

@@ -90,6 +90,7 @@
     "ripple-lib": "^1.0.0-beta.0",
     "rxjs": "^6.2.0",
     "rxjs-compat": "^6.1.0",
+    "semaphore": "^1.1.0",
     "smooth-scrollbar": "^8.2.7",
     "source-map": "0.7.2",
     "source-map-support": "^0.5.4",

src/commands/getAddress.js

@@ -2,7 +2,7 @@
 
 import { createCommand, Command } from 'helpers/ipc'
 import { fromPromise } from 'rxjs/observable/fromPromise'
-import CommNodeHid from '@ledgerhq/hw-transport-node-hid'
+import { withDevice } from 'helpers/deviceAccess'
 import getAddressForCurrency from 'helpers/getAddressForCurrency'
 
 type Input = {
@@ -24,7 +24,7 @@ const cmd: Command<Input, Result> = createCommand(
   'getAddress',
   ({ currencyId, devicePath, path, ...options }) =>
     fromPromise(
-      CommNodeHid.open(devicePath).then(transport =>
+      withDevice(devicePath)(transport =>
         getAddressForCurrency(currencyId)(transport, currencyId, path, options),
       ),
     ),

src/commands/signTransaction.js

@@ -2,7 +2,7 @@
 
 import { createCommand, Command } from 'helpers/ipc'
 import { fromPromise } from 'rxjs/observable/fromPromise'
-import CommNodeHid from '@ledgerhq/hw-transport-node-hid'
+import { withDevice } from 'helpers/deviceAccess'
 import signTransactionForCurrency from 'helpers/signTransactionForCurrency'
 
 type Input = {
@@ -19,7 +19,7 @@ const cmd: Command<Input, Result> = createCommand(
   'signTransaction',
   ({ currencyId, devicePath, path, transaction }) =>
     fromPromise(
-      CommNodeHid.open(devicePath).then(transport =>
+      withDevice(devicePath)(transport =>
         signTransactionForCurrency(currencyId)(transport, currencyId, path, transaction),
       ),
     ),

src/helpers/deviceAccess.js

@@ -0,0 +1,51 @@
+// @flow
+import createSemaphore from 'semaphore'
+import type Transport from '@ledgerhq/hw-transport'
+import CommNodeHid from '@ledgerhq/hw-transport-node-hid'
+
+// all open to device must use openDevice so we can prevent race conditions
+// and guarantee we do one device access at a time. It also will handle the .close()
+// NOTE optim: in the future we can debounce the close & reuse the same transport instance.
+
+type WithDevice = (devicePath: string) => <T>(job: (Transport<*>) => Promise<T>) => Promise<T>
+
+const semaphorePerDevice = {}
+
+export const withDevice: WithDevice = devicePath => {
+  const { FORK_TYPE } = process.env
+  if (FORK_TYPE !== 'devices') {
+    console.warn(
+      `deviceAccess is only expected to be used in process 'devices'. Any other usage may lead to race conditions. (Got: '${FORK_TYPE}')`,
+    )
+  }
+  const sem =
+    semaphorePerDevice[devicePath] || (semaphorePerDevice[devicePath] = createSemaphore(1))
+  return job =>
+    takeSemaphorePromise(sem, async () => {
+      const t = await CommNodeHid.open(devicePath)
+      try {
+        const res = await job(t)
+        // $FlowFixMe
+        return res
+      } finally {
+        t.close()
+      }
+    })
+}
+
+function takeSemaphorePromise<T>(sem, f: () => Promise<T>): Promise<T> {
+  return new Promise((resolve, reject) => {
+    sem.take(() => {
+      f().then(
+        r => {
+          sem.leave()
+          resolve(r)
+        },
+        e => {
+          sem.leave()
+          reject(e)
+        },
+      )
+    })
+  })
+}

src/internals/accounts/scanAccountsOnDevice.js

@@ -9,11 +9,9 @@
 //
 
 import Btc from '@ledgerhq/hw-app-btc'
-import CommNodeHid from '@ledgerhq/hw-transport-node-hid'
+import { withDevice } from 'helpers/deviceAccess'
 import { getCryptoCurrencyById } from '@ledgerhq/live-common/lib/helpers/currencies'
 
-import type Transport from '@ledgerhq/hw-transport'
-
 import type { AccountRaw, OperationRaw, OperationType } from '@ledgerhq/live-common/lib/types'
 import type { NJSAccount, NJSOperation } from '@ledgerhq/ledger-core/src/ledgercore_doc'
 
@@ -23,27 +21,30 @@ type Props = {
   onAccountScanned: Function,
 }
 
-export default async function scanAccountsOnDevice(props: Props): Promise<AccountRaw[]> {
+export default function scanAccountsOnDevice(props: Props): Promise<AccountRaw[]> {
   const { devicePath, currencyId, onAccountScanned } = props
 
-  // instanciate app on device
+  return withDevice(devicePath)(async transport => {
-  const transport: Transport<*> = await CommNodeHid.open(devicePath)
+    const hwApp = new Btc(transport)
-  const hwApp = new Btc(transport)
 
-  const commonParams = {
+    const commonParams = {
-    hwApp,
+      hwApp,
-    currencyId,
+      currencyId,
-    onAccountScanned,
+      onAccountScanned,
-    devicePath,
+      devicePath,
-  }
+    }
 
-  // scan segwit AND non-segwit accounts
+    // scan segwit AND non-segwit accounts
-  const segwitAccounts = await scanAccountsOnDeviceBySegwit({ ...commonParams, isSegwit: true })
+    const segwitAccounts = await scanAccountsOnDeviceBySegwit({ ...commonParams, isSegwit: true })
-  const nonSegwitAccounts = await scanAccountsOnDeviceBySegwit({ ...commonParams, isSegwit: false })
+    const nonSegwitAccounts = await scanAccountsOnDeviceBySegwit({
+      ...commonParams,
+      isSegwit: false,
+    })
 
-  const accounts = [...segwitAccounts, ...nonSegwitAccounts]
+    const accounts = [...segwitAccounts, ...nonSegwitAccounts]
 
-  return accounts
+    return accounts
+  })
 }
 
 export async function getWalletIdentifier({

src/internals/accounts/signAndBroadcastTransaction/btc.js

@@ -1,11 +1,9 @@
 // @flow
 
 import Btc from '@ledgerhq/hw-app-btc'
-import CommNodeHid from '@ledgerhq/hw-transport-node-hid'
+import { withDevice } from 'helpers/deviceAccess'
 import type { AccountRaw } from '@ledgerhq/live-common/lib/types'
 
-import type Transport from '@ledgerhq/hw-transport'
-
 import type { IPCSend } from 'types/electron'
 import { getWalletIdentifier } from '../scanAccountsOnDevice'
 
@@ -31,38 +29,40 @@ export default async function signAndBroadcastTransactionBTCLike(
     // TODO: investigate why importing it on file scope causes trouble
     const core = require('init-ledger-core')()
 
-    // instanciate app on device
+    const txHash = await withDevice(deviceId)(async transport => {
-    const transport: Transport<*> = await CommNodeHid.open(deviceId)
+      const hwApp = new Btc(transport)
-    const hwApp = new Btc(transport)
 
-    const WALLET_IDENTIFIER = await getWalletIdentifier({
+      const WALLET_IDENTIFIER = await getWalletIdentifier({
-      hwApp,
+        hwApp,
-      isSegwit: !!account.isSegwit,
+        isSegwit: !!account.isSegwit,
-      currencyId: account.currencyId,
+        currencyId: account.currencyId,
-      devicePath: deviceId,
+        devicePath: deviceId,
-    })
+      })
 
-    const njsWallet = await core.getWallet(WALLET_IDENTIFIER)
+      const njsWallet = await core.getWallet(WALLET_IDENTIFIER)
-    const njsAccount = await njsWallet.getAccount(account.index)
+      const njsAccount = await njsWallet.getAccount(account.index)
-    const bitcoinLikeAccount = njsAccount.asBitcoinLikeAccount()
+      const bitcoinLikeAccount = njsAccount.asBitcoinLikeAccount()
-    const njsWalletCurrency = njsWallet.getCurrency()
+      const njsWalletCurrency = njsWallet.getCurrency()
-    const amount = core.createAmount(njsWalletCurrency, transaction.amount)
+      const amount = core.createAmount(njsWalletCurrency, transaction.amount)
-    const fees = core.createAmount(njsWalletCurrency, transaction.feePerByte)
+      const fees = core.createAmount(njsWalletCurrency, transaction.feePerByte)
-    const transactionBuilder = bitcoinLikeAccount.buildTransaction()
+      const transactionBuilder = bitcoinLikeAccount.buildTransaction()
 
-    // TODO: check if is valid address. if not, it will fail silently on invalid
+      // TODO: check if is valid address. if not, it will fail silently on invalid
 
-    transactionBuilder.sendToAddress(amount, transaction.recipient)
+      transactionBuilder.sendToAddress(amount, transaction.recipient)
-    // TODO: don't use hardcoded value for sequence (and first also maybe)
+      // TODO: don't use hardcoded value for sequence (and first also maybe)
-    transactionBuilder.pickInputs(0, 0xffffff)
+      transactionBuilder.pickInputs(0, 0xffffff)
-    transactionBuilder.setFeesPerByte(fees)
+      transactionBuilder.setFeesPerByte(fees)
 
-    const builded = await transactionBuilder.build()
+      const builded = await transactionBuilder.build()
-    const signedTransaction = await core.signTransaction(hwApp, builded)
+      const signedTransaction = await core.signTransaction(hwApp, builded)
 
-    const txHash = await njsAccount
+      const txHash = await njsAccount
-      .asBitcoinLikeAccount()
+        .asBitcoinLikeAccount()
-      .broadcastRawTransaction(signedTransaction)
+        .broadcastRawTransaction(signedTransaction)
+
+      return txHash
+    })
 
     send('accounts.signAndBroadcastTransactionBTCLike.success', txHash)
   } catch (err) {

src/internals/manager/helpers.js

@@ -1,6 +1,6 @@
 // @flow
 
-import CommNodeHid from '@ledgerhq/hw-transport-node-hid'
+import { withDevice } from 'helpers/deviceAccess'
 import chalk from 'chalk'
 import Websocket from 'ws'
 import qs from 'qs'
@@ -44,6 +44,7 @@ export function createTransportHandler(
     errorResponse: string,
   },
 ) {
+  console.log('DEPRECATED: createTransportHandler use withDevice and commands/*')
   return async function transportHandler({
     devicePath,
     ...params
@@ -51,9 +52,7 @@ export function createTransportHandler(
     devicePath: string,
   }): Promise<void> {
     try {
-      const transport: Transport<*> = await CommNodeHid.open(devicePath)
+      const data = await withDevice(devicePath)(transport => action(transport, params))
-      // $FlowFixMe
-      const data = await action(transport, params)
       send(successResponse, data)
     } catch (err) {
       if (!err) {

yarn.lock

@@ -12203,6 +12203,10 @@ selfsigned@^1.9.1:
   dependencies:
     node-forge "0.7.5"
 
+semaphore@^1.1.0:
+  version "1.1.0"
+  resolved "https://registry.yarnpkg.com/semaphore/-/semaphore-1.1.0.tgz#aaad8b86b20fe8e9b32b16dc2ee682a8cd26a8aa"
+
 semver-diff@^2.0.0:
   version "2.1.0"
   resolved "https://registry.yarnpkg.com/semver-diff/-/semver-diff-2.1.0.tgz#4bbb8437c8d37e4b0cf1a68fd726ec6d645d6d36"