From ad2d8c6218fff9696d5a45f85d9cd695d096950f Mon Sep 17 00:00:00 2001 From: niklasad1 Date: Sat, 10 Feb 2018 00:21:35 +0100 Subject: [PATCH 1/6] Hardware-wallet fix * More fine-grained initilization of callbacks by vendorID, productID and usb class * Each device manufacturer gets a seperate handle thread each * Replaced "dummy for loop" with a delay to wait for the device to boot-up properly * Haven't been very carefully with checking dependencies cycles etc * Inline comments explaining where shortcuts have been taken * Need to test this on Windows machine and with Ledger (both models) Signed-off-by: niklasad1 --- hw/src/ledger.rs | 55 ++++++++++++++++++------ hw/src/lib.rs | 107 +++++++++++++++++++++++------------------------ hw/src/trezor.rs | 54 ++++++++++++++++++------ 3 files changed, 137 insertions(+), 79 deletions(-) diff --git a/hw/src/ledger.rs b/hw/src/ledger.rs index f2c2c0ec53b..e274a830de9 100644 --- a/hw/src/ledger.rs +++ b/hw/src/ledger.rs @@ -20,18 +20,21 @@ use std::cmp::min; use std::fmt; use std::str::FromStr; -use std::sync::Arc; +use std::sync::{Arc, Weak}; use std::time::Duration; +use std::thread; use ethereum_types::{H256, Address}; use ethkey::Signature; use hidapi; +use libusb; use parking_lot::{Mutex, RwLock}; use super::{WalletInfo, KeyPath}; -const LEDGER_VID: u16 = 0x2c97; -const LEDGER_PIDS: [u16; 2] = [0x0000, 0x0001]; // Nano S and Blue +// Expose these to the callback configuration +pub const LEDGER_VID: u16 = 0x2c97; +pub const LEDGER_PIDS: [u16; 2] = [0x0000, 0x0001]; // Nano S and Blue const ETH_DERIVATION_PATH_BE: [u8; 17] = [4, 0x80, 0, 0, 44, 0x80, 0, 0, 60, 0x80, 0, 0, 0, 0, 0, 0, 0]; // 44'/60'/0'/0 const ETC_DERIVATION_PATH_BE: [u8; 21] = [5, 0x80, 0, 0, 44, 0x80, 0, 0, 60, 0x80, 0x02, 0x73, 0xd0, 0x80, 0, 0, 0, 0, 0, 0, 0]; // 44'/60'/160720'/0'/0 @@ -234,16 +237,10 @@ impl Manager { fn open_path(&self, f: F) -> Result where F: Fn() -> Result { - let mut err = Error::KeyNotFound; - // Try to open device a few times. - for _ in 0..10 { - match f() { - Ok(handle) => return Ok(handle), - Err(e) => err = From::from(e), - } - ::std::thread::sleep(Duration::from_millis(200)); + match f() { + Ok(handle) => Ok(handle), + Err(e) => Err(From::from(e)), } - Err(err) } fn send_apdu(handle: &hidapi::HidDevice, command: u8, p1: u8, p2: u8, data: &[u8]) -> Result, Error> { @@ -335,6 +332,40 @@ impl Manager { } } +pub struct EventHandler { + ledger: Weak, +} + +impl EventHandler { + pub fn new(ledger: Weak) -> Self { + Self { ledger: ledger } + } +} + + +impl libusb::Hotplug for EventHandler { + fn device_arrived(&mut self, _device: libusb::Device) { + println!("ledger arrived"); + Duration::from_millis(1000); + if let Some(t) = self.ledger.upgrade() { + // Wait for the device to bootup + thread::sleep(Duration::from_millis(1000)); + if let Err(e) = t.update_devices() { + debug!("Ledger Connect Error: {:?}", e); + } + } + } + + fn device_left(&mut self, _device: libusb::Device) { + println!("ledger left"); + if let Some(l) = self.ledger.upgrade() { + if let Err(e) = l.update_devices() { + debug!("Ledger Disconnect Error: {:?}", e); + } + } + } +} + #[test] fn smoke() { use rustc_hex::FromHex; diff --git a/hw/src/lib.rs b/hw/src/lib.rs index a1e90ea3e8a..87f13abcb1e 100644 --- a/hw/src/lib.rs +++ b/hw/src/lib.rs @@ -33,13 +33,16 @@ use ethkey::{Address, Signature}; use parking_lot::Mutex; use std::fmt; -use std::sync::{Arc, Weak}; +use std::sync::Arc; use std::sync::atomic; use std::sync::atomic::AtomicBool; use std::thread; use std::time::Duration; use ethereum_types::U256; +const USB_DEVICE_CLASS_DEVICE: u8 = 0; + + /// Hardware wallet error. #[derive(Debug)] pub enum Error { @@ -128,84 +131,76 @@ impl From for Error { /// Hardware wallet management interface. pub struct HardwareWalletManager { - update_thread: Option>, + active_threads: Vec>>, exiting: Arc, ledger: Arc, trezor: Arc, } -struct EventHandler { - ledger: Weak, - trezor: Weak, -} - -impl libusb::Hotplug for EventHandler { - fn device_arrived(&mut self, _device: libusb::Device) { - debug!("USB Device arrived"); - if let (Some(l), Some(t)) = (self.ledger.upgrade(), self.trezor.upgrade()) { - for _ in 0..10 { - let l_devices = l.update_devices().unwrap_or_else(|e| { - debug!("Error enumerating Ledger devices: {}", e); - 0 - }); - let t_devices = t.update_devices().unwrap_or_else(|e| { - debug!("Error enumerating Trezor devices: {}", e); - 0 - }); - if l_devices + t_devices > 0 { - break; - } - thread::sleep(Duration::from_millis(200)); - } - } - } - - fn device_left(&mut self, _device: libusb::Device) { - debug!("USB Device lost"); - if let (Some(l), Some(t)) = (self.ledger.upgrade(), self.trezor.upgrade()) { - l.update_devices().unwrap_or_else(|e| {debug!("Error enumerating Ledger devices: {}", e); 0}); - t.update_devices().unwrap_or_else(|e| {debug!("Error enumerating Trezor devices: {}", e); 0}); - } - } -} impl HardwareWalletManager { pub fn new() -> Result { - let usb_context = Arc::new(libusb::Context::new()?); + let usb_context_trezor = Arc::new(libusb::Context::new()?); + let usb_context_ledger = Arc::new(libusb::Context::new()?); + let mut active_threads: Vec>> = Vec::with_capacity(10); let hidapi = Arc::new(Mutex::new(hidapi::HidApi::new().map_err(|e| Error::Hid(e.to_string().clone()))?)); let ledger = Arc::new(ledger::Manager::new(hidapi.clone())); let trezor = Arc::new(trezor::Manager::new(hidapi.clone())); - usb_context.register_callback( - None, None, None, - Box::new(EventHandler { - ledger: Arc::downgrade(&ledger), - trezor: Arc::downgrade(&trezor), - }), - )?; + + // Note this support only TREZOR V1 becasue TREZOR V2 has both different vendorID and // productID + usb_context_trezor.register_callback( + Some(trezor::TREZOR_VID), Some(trezor::TREZOR_PIDS[0]), Some(USB_DEVICE_CLASS_DEVICE), + Box::new(trezor::EventHandler::new(Arc::downgrade(&trezor))))?; + + // Only specify vendorID and usb device class to start with (unsure if 1 listens for both 0 and 1 or only 1) + usb_context_ledger.register_callback( + Some(ledger::LEDGER_VID), None, Some(USB_DEVICE_CLASS_DEVICE), + Box::new(ledger::EventHandler::new(Arc::downgrade(&ledger))))?; + let exiting = Arc::new(AtomicBool::new(false)); - let thread_exiting = exiting.clone(); + let thread_exiting_ledger = exiting.clone(); + let thread_exiting_trezor = exiting.clone(); let l = ledger.clone(); let t = trezor.clone(); - let thread = thread::Builder::new() - .name("hw_wallet".to_string()) + + // Ledger event thread + // FIXME: check if we can move it to ledger.rs (lifetime issue) + active_threads.push(thread::Builder::new() + .name("hw_wallet_ledger".to_string()) .spawn(move || { if let Err(e) = l.update_devices() { debug!("Error updating ledger devices: {}", e); } + loop { + usb_context_ledger.handle_events(Some(Duration::from_millis(500))) + .unwrap_or_else(|e| debug!("Error processing USB events: {}", e)); + if thread_exiting_ledger.load(atomic::Ordering::Acquire) { + break; + } + } + }) + .ok()); + + // Trezor event thread + // FIXME: check if we can move it to trezor.rs (lifetime issue) + active_threads.push(thread::Builder::new() + .name("hw_wallet_trezor".to_string()) + .spawn(move || { if let Err(e) = t.update_devices() { - debug!("Error updating trezor devices: {}", e); + debug!("Error updating ledger devices: {}", e); } loop { - usb_context.handle_events(Some(Duration::from_millis(500))) + usb_context_trezor.handle_events(Some(Duration::from_millis(500))) .unwrap_or_else(|e| debug!("Error processing USB events: {}", e)); - if thread_exiting.load(atomic::Ordering::Acquire) { + if thread_exiting_trezor.load(atomic::Ordering::Acquire) { break; } } }) - .ok(); + .ok()); + Ok(HardwareWalletManager { - update_thread: thread, + active_threads: active_threads, exiting: exiting, ledger: ledger, trezor: trezor, @@ -260,9 +255,11 @@ impl HardwareWalletManager { impl Drop for HardwareWalletManager { fn drop(&mut self) { self.exiting.store(true, atomic::Ordering::Release); - if let Some(thread) = self.update_thread.take() { - thread.thread().unpark(); - thread.join().ok(); + for thread in self.active_threads.iter_mut() { + if let Some(thread) = thread.take() { + thread.thread().unpark(); + thread.join().ok(); + } } } } diff --git a/hw/src/trezor.rs b/hw/src/trezor.rs index 1cde534023c..88513e96a7d 100644 --- a/hw/src/trezor.rs +++ b/hw/src/trezor.rs @@ -23,21 +23,24 @@ use super::{WalletInfo, TransactionInfo, KeyPath}; use std::cmp::{min, max}; use std::fmt; -use std::sync::Arc; +use std::sync::{Arc, Weak}; use std::time::Duration; +use std::thread; use ethereum_types::{U256, H256, Address}; use ethkey::Signature; use hidapi; +use libusb; use parking_lot::{Mutex, RwLock}; use protobuf; use protobuf::{Message, ProtobufEnum}; use trezor_sys::messages::{EthereumAddress, PinMatrixAck, MessageType, EthereumTxRequest, EthereumSignTx, EthereumGetAddress, EthereumTxAck, ButtonAck}; -const TREZOR_VID: u16 = 0x534c; -const TREZOR_PIDS: [u16; 1] = [0x0001]; // Trezor v1, keeping this as an array to leave room for Trezor v2 which is in progress +// Expose these to the callback configuration +pub const TREZOR_VID: u16 = 0x534c; +pub const TREZOR_PIDS: [u16; 1] = [0x0001]; // Trezor v1, keeping this as an array to leave room for Trezor v2 which is in progress const ETH_DERIVATION_PATH: [u32; 5] = [0x8000002C, 0x8000003C, 0x80000000, 0, 0]; // m/44'/60'/0'/0/0 const ETC_DERIVATION_PATH: [u32; 5] = [0x8000002C, 0x8000003D, 0x80000000, 0, 0]; // m/44'/61'/0'/0/0 @@ -201,16 +204,10 @@ impl Manager { fn open_path(&self, f: F) -> Result where F: Fn() -> Result { - let mut err = Error::KeyNotFound; - // Try to open device a few times. - for _ in 0..10 { - match f() { - Ok(handle) => return Ok(handle), - Err(e) => err = From::from(e), - } - ::std::thread::sleep(Duration::from_millis(200)); + match f() { + Ok(handle) => Ok(handle), + Err(e) => Err(From::from(e)), } - Err(err) } pub fn pin_matrix_ack(&self, device_path: &str, pin: &str) -> Result { @@ -407,6 +404,39 @@ impl Manager { } } +pub struct EventHandler { + trezor: Weak, +} + +impl EventHandler { + pub fn new(trezor: Weak) -> Self { + Self { trezor: trezor } + } +} + +impl libusb::Hotplug for EventHandler { + fn device_arrived(&mut self, _device: libusb::Device) { + println!("trezor arrived"); + if let Some(t) = self.trezor.upgrade() { + // Wait for the device to boot up + thread::sleep(Duration::from_millis(1000)); + if let Err(e) = t.update_devices() { + debug!("TREZOR Connect Error: {:?}", e); + } + } + } + + fn device_left(&mut self, _device: libusb::Device) { + println!("trezor left"); + if let Some(t) = self.trezor.upgrade() { + if let Err(e) = t.update_devices() { + debug!("TREZOR Disconnect fail: {:?}", e); + } + } + } +} + + #[test] #[ignore] /// This test can't be run without an actual trezor device connected From cdd58ad8a77f4f0d1e5fc5b93e25d87e072b3a22 Mon Sep 17 00:00:00 2001 From: niklasad1 Date: Sat, 10 Feb 2018 11:17:27 +0100 Subject: [PATCH 2/6] Validate product_id of detected ledger devices --- hw/src/ledger.rs | 25 +++++++++++++++++++++++-- hw/src/lib.rs | 9 +++++++-- 2 files changed, 30 insertions(+), 4 deletions(-) diff --git a/hw/src/ledger.rs b/hw/src/ledger.rs index e274a830de9..7a5c601a0d3 100644 --- a/hw/src/ledger.rs +++ b/hw/src/ledger.rs @@ -57,10 +57,14 @@ pub enum Error { Protocol(&'static str), /// Hidapi error. Usb(hidapi::HidError), + /// Libusb error + LibUsb(libusb::Error), /// Device with request key is not available. KeyNotFound, /// Signing has been cancelled by user. UserCancel, + /// Invalid Product + InvalidProduct, } impl fmt::Display for Error { @@ -68,8 +72,10 @@ impl fmt::Display for Error { match *self { Error::Protocol(ref s) => write!(f, "Ledger protocol error: {}", s), Error::Usb(ref e) => write!(f, "USB communication error: {}", e), + Error::LibUsb(ref e) => write!(f, "LibUSB communication error: {}", e), Error::KeyNotFound => write!(f, "Key not found"), Error::UserCancel => write!(f, "Operation has been cancelled"), + Error::InvalidProduct=> write!(f, "Unsupported product was entered"), } } } @@ -80,6 +86,12 @@ impl From for Error { } } +impl From for Error { + fn from(err: libusb::Error) -> Error { + Error::LibUsb(err) + } +} + /// Ledger device manager. pub struct Manager { usb: Arc>, @@ -340,14 +352,23 @@ impl EventHandler { pub fn new(ledger: Weak) -> Self { Self { ledger: ledger } } + + fn is_valid_product(&self, device: &libusb::Device) -> Result<(), Error> { + let product_id = device.device_descriptor()?.product_id(); + match LEDGER_PIDS.contains(&product_id) { + true => Ok(()), + false => Err(Error::InvalidProduct), + } + } } impl libusb::Hotplug for EventHandler { - fn device_arrived(&mut self, _device: libusb::Device) { + fn device_arrived(&mut self, device: libusb::Device) { println!("ledger arrived"); Duration::from_millis(1000); - if let Some(t) = self.ledger.upgrade() { + + if let (Some(t), Ok(_)) = (self.ledger.upgrade(), self.is_valid_product(&device)) { // Wait for the device to bootup thread::sleep(Duration::from_millis(1000)); if let Err(e) = t.update_devices() { diff --git a/hw/src/lib.rs b/hw/src/lib.rs index 87f13abcb1e..14073310fd8 100644 --- a/hw/src/lib.rs +++ b/hw/src/lib.rs @@ -147,12 +147,17 @@ impl HardwareWalletManager { let ledger = Arc::new(ledger::Manager::new(hidapi.clone())); let trezor = Arc::new(trezor::Manager::new(hidapi.clone())); - // Note this support only TREZOR V1 becasue TREZOR V2 has both different vendorID and // productID + // Subscribe to TREZOR V1 + // Note this support only TREZOR V1 becasue TREZOR V2 has another vendorID for some reason + // Because we now only support one product only subscribe to it as the second argument `Some(1)` specifies usb_context_trezor.register_callback( Some(trezor::TREZOR_VID), Some(trezor::TREZOR_PIDS[0]), Some(USB_DEVICE_CLASS_DEVICE), Box::new(trezor::EventHandler::new(Arc::downgrade(&trezor))))?; - // Only specify vendorID and usb device class to start with (unsure if 1 listens for both 0 and 1 or only 1) + // Subscribe to all Ledger Devices + // This means that we need to check that the given productID is supported + // None -> LIBUSB_HOTPLUG_MATCH_ANY + // http://libusb.sourceforge.net/api-1.0/group__hotplug.html#gae6c5f1add6cc754005549c7259dc35ea usb_context_ledger.register_callback( Some(ledger::LEDGER_VID), None, Some(USB_DEVICE_CLASS_DEVICE), Box::new(ledger::EventHandler::new(Arc::downgrade(&ledger))))?; From 76baa273065cd2e1b8b2732353c14d9bf8cc9f5a Mon Sep 17 00:00:00 2001 From: niklasad1 Date: Sun, 11 Feb 2018 22:20:47 +0100 Subject: [PATCH 3/6] closed_device => unlocked_device --- hw/src/ledger.rs | 18 +++++++++++------- hw/src/lib.rs | 8 ++++---- hw/src/trezor.rs | 20 +++++++++++--------- 3 files changed, 26 insertions(+), 20 deletions(-) diff --git a/hw/src/ledger.rs b/hw/src/ledger.rs index 7a5c601a0d3..c273c41799f 100644 --- a/hw/src/ledger.rs +++ b/hw/src/ledger.rs @@ -63,8 +63,8 @@ pub enum Error { KeyNotFound, /// Signing has been cancelled by user. UserCancel, - /// Invalid Product - InvalidProduct, + /// Invalid Device + InvalidDevice, } impl fmt::Display for Error { @@ -75,7 +75,7 @@ impl fmt::Display for Error { Error::LibUsb(ref e) => write!(f, "LibUSB communication error: {}", e), Error::KeyNotFound => write!(f, "Key not found"), Error::UserCancel => write!(f, "Operation has been cancelled"), - Error::InvalidProduct=> write!(f, "Unsupported product was entered"), + Error::InvalidDevice => write!(f, "Unsupported product was entered"), } } } @@ -354,10 +354,14 @@ impl EventHandler { } fn is_valid_product(&self, device: &libusb::Device) -> Result<(), Error> { - let product_id = device.device_descriptor()?.product_id(); - match LEDGER_PIDS.contains(&product_id) { - true => Ok(()), - false => Err(Error::InvalidProduct), + let desc = device.device_descriptor()?; + let vendor_id = desc.vendor_id(); + let product_id = desc.product_id(); + + if vendor_id == LEDGER_VID && LEDGER_PIDS.contains(&product_id) { + Ok(()) + } else { + Err(Error::InvalidDevice) } } } diff --git a/hw/src/lib.rs b/hw/src/lib.rs index 14073310fd8..9b45623a324 100644 --- a/hw/src/lib.rs +++ b/hw/src/lib.rs @@ -148,16 +148,16 @@ impl HardwareWalletManager { let trezor = Arc::new(trezor::Manager::new(hidapi.clone())); // Subscribe to TREZOR V1 - // Note this support only TREZOR V1 becasue TREZOR V2 has another vendorID for some reason - // Because we now only support one product only subscribe to it as the second argument `Some(1)` specifies + // Note, this support only TREZOR V1 becasue TREZOR V2 has another vendorID for some reason + // Also, we now only support one product as the second argument specifies usb_context_trezor.register_callback( Some(trezor::TREZOR_VID), Some(trezor::TREZOR_PIDS[0]), Some(USB_DEVICE_CLASS_DEVICE), Box::new(trezor::EventHandler::new(Arc::downgrade(&trezor))))?; // Subscribe to all Ledger Devices // This means that we need to check that the given productID is supported - // None -> LIBUSB_HOTPLUG_MATCH_ANY - // http://libusb.sourceforge.net/api-1.0/group__hotplug.html#gae6c5f1add6cc754005549c7259dc35ea + // None => LIBUSB_HOTPLUG_MATCH_ANY, in other words that all are subscribed to + // More info can be found: http://libusb.sourceforge.net/api-1.0/group__hotplug.html#gae6c5f1add6cc754005549c7259dc35ea usb_context_ledger.register_callback( Some(ledger::LEDGER_VID), None, Some(USB_DEVICE_CLASS_DEVICE), Box::new(ledger::EventHandler::new(Arc::downgrade(&ledger))))?; diff --git a/hw/src/trezor.rs b/hw/src/trezor.rs index 88513e96a7d..e99d51ac152 100644 --- a/hw/src/trezor.rs +++ b/hw/src/trezor.rs @@ -91,7 +91,7 @@ impl From for Error { pub struct Manager { usb: Arc>, devices: RwLock>, - closed_devices: RwLock>, + locked_devices: RwLock>, key_path: RwLock, } @@ -113,7 +113,7 @@ impl Manager { Manager { usb: hidapi, devices: RwLock::new(Vec::new()), - closed_devices: RwLock::new(Vec::new()), + locked_devices: RwLock::new(Vec::new()), key_path: RwLock::new(KeyPath::Ethereum), } } @@ -124,7 +124,7 @@ impl Manager { usb.refresh_devices(); let devices = usb.devices(); let mut new_devices = Vec::new(); - let mut closed_devices = Vec::new(); + let mut locked_devices = Vec::new(); let mut error = None; for usb_device in devices { let is_trezor = usb_device.vendor_id == TREZOR_VID; @@ -143,7 +143,7 @@ impl Manager { } match self.read_device_info(&usb, &usb_device) { Ok(device) => new_devices.push(device), - Err(Error::ClosedDevice(path)) => closed_devices.push(path.to_string()), + Err(Error::ClosedDevice(path)) => locked_devices.push(path.to_string()), Err(e) => { warn!("Error reading device: {:?}", e); error = Some(e); @@ -151,9 +151,9 @@ impl Manager { } } let count = new_devices.len(); - trace!("Got devices: {:?}, closed: {:?}", new_devices, closed_devices); + trace!("Got devices: {:?}, closed: {:?}", new_devices, locked_devices); *self.devices.write() = new_devices; - *self.closed_devices.write() = closed_devices; + *self.locked_devices.write() = locked_devices; match error { Some(e) => Err(e), None => Ok(count), @@ -193,7 +193,7 @@ impl Manager { } pub fn list_locked_devices(&self) -> Vec { - (*self.closed_devices.read()).clone() + (*self.locked_devices.read()).clone() } /// Get wallet info. @@ -418,11 +418,14 @@ impl libusb::Hotplug for EventHandler { fn device_arrived(&mut self, _device: libusb::Device) { println!("trezor arrived"); if let Some(t) = self.trezor.upgrade() { - // Wait for the device to boot up + // Wait for the device to boot-up thread::sleep(Duration::from_millis(1000)); if let Err(e) = t.update_devices() { debug!("TREZOR Connect Error: {:?}", e); } + println!("unlocked devices: {:?}", t.list_devices()); + println!("locked devices: {:?}", t.list_locked_devices()); + } } @@ -436,7 +439,6 @@ impl libusb::Hotplug for EventHandler { } } - #[test] #[ignore] /// This test can't be run without an actual trezor device connected From 984065cc5cd80023024556bcb1416dd00f041ba2 Mon Sep 17 00:00:00 2001 From: niklasad1 Date: Tue, 13 Feb 2018 10:21:44 +0100 Subject: [PATCH 4/6] address comments --- hw/src/ledger.rs | 58 ++++++++++++++++++++++++------------------------ hw/src/lib.rs | 14 +++++------- hw/src/trezor.rs | 45 ++++++++++++++++++------------------- 3 files changed, 57 insertions(+), 60 deletions(-) diff --git a/hw/src/ledger.rs b/hw/src/ledger.rs index c273c41799f..39d5a7a9c74 100644 --- a/hw/src/ledger.rs +++ b/hw/src/ledger.rs @@ -32,9 +32,11 @@ use parking_lot::{Mutex, RwLock}; use super::{WalletInfo, KeyPath}; -// Expose these to the callback configuration +/// Ledger vendor ID pub const LEDGER_VID: u16 = 0x2c97; -pub const LEDGER_PIDS: [u16; 2] = [0x0000, 0x0001]; // Nano S and Blue +/// Legder product IDs: [Nano S and Blue] +pub const LEDGER_PIDS: [u16; 2] = [0x0000, 0x0001]; + const ETH_DERIVATION_PATH_BE: [u8; 17] = [4, 0x80, 0, 0, 44, 0x80, 0, 0, 60, 0x80, 0, 0, 0, 0, 0, 0, 0]; // 44'/60'/0'/0 const ETC_DERIVATION_PATH_BE: [u8; 21] = [5, 0x80, 0, 0, 44, 0x80, 0, 0, 60, 0x80, 0x02, 0x73, 0xd0, 0x80, 0, 0, 0, 0, 0, 0, 0]; // 44'/60'/160720'/0'/0 @@ -249,10 +251,7 @@ impl Manager { fn open_path(&self, f: F) -> Result where F: Fn() -> Result { - match f() { - Ok(handle) => Ok(handle), - Err(e) => Err(From::from(e)), - } + f().map_err(Into::into) } fn send_apdu(handle: &hidapi::HidDevice, command: u8, p1: u8, p2: u8, data: &[u8]) -> Result, Error> { @@ -342,18 +341,8 @@ impl Manager { message.truncate(new_len); Ok(message) } -} - -pub struct EventHandler { - ledger: Weak, -} - -impl EventHandler { - pub fn new(ledger: Weak) -> Self { - Self { ledger: ledger } - } - fn is_valid_product(&self, device: &libusb::Device) -> Result<(), Error> { + fn is_valid_ledger(device: &libusb::Device) -> Result<(), Error> { let desc = device.device_descriptor()?; let vendor_id = desc.vendor_id(); let product_id = desc.product_id(); @@ -364,28 +353,39 @@ impl EventHandler { Err(Error::InvalidDevice) } } + } +/// Ledger event handler +/// A seperate thread is handling incoming events +pub struct EventHandler { + ledger: Weak, +} + +impl EventHandler { + /// Ledger event handler constructor + pub fn new(ledger: Weak) -> Self { + Self { ledger: ledger } + } +} impl libusb::Hotplug for EventHandler { fn device_arrived(&mut self, device: libusb::Device) { - println!("ledger arrived"); - Duration::from_millis(1000); - - if let (Some(t), Ok(_)) = (self.ledger.upgrade(), self.is_valid_product(&device)) { - // Wait for the device to bootup + if let (Some(ledger), Ok(_)) = (self.ledger.upgrade(), Manager::is_valid_ledger(&device)) { + debug!("Ledger arrived"); + // Wait for the device to boot up thread::sleep(Duration::from_millis(1000)); - if let Err(e) = t.update_devices() { - debug!("Ledger Connect Error: {:?}", e); + if let Err(e) = ledger.update_devices() { + debug!("Ledger connect error: {:?}", e); } } } - fn device_left(&mut self, _device: libusb::Device) { - println!("ledger left"); - if let Some(l) = self.ledger.upgrade() { - if let Err(e) = l.update_devices() { - debug!("Ledger Disconnect Error: {:?}", e); + fn device_left(&mut self, device: libusb::Device) { + if let (Some(ledger), Ok(_)) = (self.ledger.upgrade(), Manager::is_valid_ledger(&device)) { + debug!("Ledger left"); + if let Err(e) = ledger.update_devices() { + debug!("Ledger disconnect error: {:?}", e); } } } diff --git a/hw/src/lib.rs b/hw/src/lib.rs index 9b45623a324..dfc2b88fe2b 100644 --- a/hw/src/lib.rs +++ b/hw/src/lib.rs @@ -42,7 +42,6 @@ use ethereum_types::U256; const USB_DEVICE_CLASS_DEVICE: u8 = 0; - /// Hardware wallet error. #[derive(Debug)] pub enum Error { @@ -139,10 +138,11 @@ pub struct HardwareWalletManager { impl HardwareWalletManager { + /// Hardware wallet constructor pub fn new() -> Result { let usb_context_trezor = Arc::new(libusb::Context::new()?); let usb_context_ledger = Arc::new(libusb::Context::new()?); - let mut active_threads: Vec>> = Vec::with_capacity(10); + let mut active_threads: Vec>> = Vec::new(); let hidapi = Arc::new(Mutex::new(hidapi::HidApi::new().map_err(|e| Error::Hid(e.to_string().clone()))?)); let ledger = Arc::new(ledger::Manager::new(hidapi.clone())); let trezor = Arc::new(trezor::Manager::new(hidapi.clone())); @@ -169,16 +169,15 @@ impl HardwareWalletManager { let t = trezor.clone(); // Ledger event thread - // FIXME: check if we can move it to ledger.rs (lifetime issue) active_threads.push(thread::Builder::new() .name("hw_wallet_ledger".to_string()) .spawn(move || { if let Err(e) = l.update_devices() { - debug!("Error updating ledger devices: {}", e); + debug!("Ledger could not connect at startup, error: {}", e); } loop { usb_context_ledger.handle_events(Some(Duration::from_millis(500))) - .unwrap_or_else(|e| debug!("Error processing USB events: {}", e)); + .unwrap_or_else(|e| debug!("Ledger event handler error: {}", e)); if thread_exiting_ledger.load(atomic::Ordering::Acquire) { break; } @@ -187,16 +186,15 @@ impl HardwareWalletManager { .ok()); // Trezor event thread - // FIXME: check if we can move it to trezor.rs (lifetime issue) active_threads.push(thread::Builder::new() .name("hw_wallet_trezor".to_string()) .spawn(move || { if let Err(e) = t.update_devices() { - debug!("Error updating ledger devices: {}", e); + debug!("Trezor could not connect at startup, error: {}", e); } loop { usb_context_trezor.handle_events(Some(Duration::from_millis(500))) - .unwrap_or_else(|e| debug!("Error processing USB events: {}", e)); + .unwrap_or_else(|e| debug!("Trezor event handler error: {}", e)); if thread_exiting_trezor.load(atomic::Ordering::Acquire) { break; } diff --git a/hw/src/trezor.rs b/hw/src/trezor.rs index e99d51ac152..1c3e3f54277 100644 --- a/hw/src/trezor.rs +++ b/hw/src/trezor.rs @@ -38,13 +38,14 @@ use protobuf::{Message, ProtobufEnum}; use trezor_sys::messages::{EthereumAddress, PinMatrixAck, MessageType, EthereumTxRequest, EthereumSignTx, EthereumGetAddress, EthereumTxAck, ButtonAck}; -// Expose these to the callback configuration +/// Trezor v1 vendor ID pub const TREZOR_VID: u16 = 0x534c; -pub const TREZOR_PIDS: [u16; 1] = [0x0001]; // Trezor v1, keeping this as an array to leave room for Trezor v2 which is in progress +/// Trezor product IDs +pub const TREZOR_PIDS: [u16; 1] = [0x0001]; + const ETH_DERIVATION_PATH: [u32; 5] = [0x8000002C, 0x8000003C, 0x80000000, 0, 0]; // m/44'/60'/0'/0/0 const ETC_DERIVATION_PATH: [u32; 5] = [0x8000002C, 0x8000003D, 0x80000000, 0, 0]; // m/44'/61'/0'/0/0 - /// Hardware wallet error. #[derive(Debug)] pub enum Error { @@ -59,7 +60,7 @@ pub enum Error { /// The Message Type given in the trezor RPC call is not something we recognize BadMessageType, /// Trying to read from a closed device at the given path - ClosedDevice(String), + LockedDevice(String), } impl fmt::Display for Error { @@ -70,7 +71,7 @@ impl fmt::Display for Error { Error::KeyNotFound => write!(f, "Key not found"), Error::UserCancel => write!(f, "Operation has been cancelled"), Error::BadMessageType => write!(f, "Bad Message Type in RPC call"), - Error::ClosedDevice(ref s) => write!(f, "Device is closed, needs PIN to perform operations: {}", s), + Error::LockedDevice(ref s) => write!(f, "Device is locked, needs PIN to perform operations: {}", s), } } } @@ -87,7 +88,7 @@ impl From for Error { } } -/// Ledger device manager. +/// Ledger device manager pub struct Manager { usb: Arc>, devices: RwLock>, @@ -143,7 +144,7 @@ impl Manager { } match self.read_device_info(&usb, &usb_device) { Ok(device) => new_devices.push(device), - Err(Error::ClosedDevice(path)) => locked_devices.push(path.to_string()), + Err(Error::LockedDevice(path)) => locked_devices.push(path.to_string()), Err(e) => { warn!("Error reading device: {:?}", e); error = Some(e); @@ -177,7 +178,7 @@ impl Manager { }, }) } - Ok(None) => Err(Error::ClosedDevice(dev_info.path.clone())), + Ok(None) => Err(Error::LockedDevice(dev_info.path.clone())), Err(e) => Err(e), } } @@ -204,10 +205,7 @@ impl Manager { fn open_path(&self, f: F) -> Result where F: Fn() -> Result { - match f() { - Ok(handle) => Ok(handle), - Err(e) => Err(From::from(e)), - } + f().map_err(Into::into) } pub fn pin_matrix_ack(&self, device_path: &str, pin: &str) -> Result { @@ -404,11 +402,14 @@ impl Manager { } } +/// Trezor event handler +/// A separate thread is handeling incoming events pub struct EventHandler { trezor: Weak, } impl EventHandler { + // Trezor event handler constructor pub fn new(trezor: Weak) -> Self { Self { trezor: trezor } } @@ -416,24 +417,22 @@ impl EventHandler { impl libusb::Hotplug for EventHandler { fn device_arrived(&mut self, _device: libusb::Device) { - println!("trezor arrived"); - if let Some(t) = self.trezor.upgrade() { - // Wait for the device to boot-up + debug!("Trezor V1 arrived"); + if let Some(trezor) = self.trezor.upgrade() { + // Wait for the device to boot up thread::sleep(Duration::from_millis(1000)); - if let Err(e) = t.update_devices() { - debug!("TREZOR Connect Error: {:?}", e); + if let Err(e) = trezor.update_devices() { + debug!("Trezor V1 connect error: {:?}", e); } - println!("unlocked devices: {:?}", t.list_devices()); - println!("locked devices: {:?}", t.list_locked_devices()); } } fn device_left(&mut self, _device: libusb::Device) { - println!("trezor left"); - if let Some(t) = self.trezor.upgrade() { - if let Err(e) = t.update_devices() { - debug!("TREZOR Disconnect fail: {:?}", e); + debug!("Trezor V1 left"); + if let Some(trezor) = self.trezor.upgrade() { + if let Err(e) = trezor.update_devices() { + debug!("Trezor V1 disconnect error: {:?}", e); } } } From cbad867a77e9fdd6837d777bf05eea3e038d7aa0 Mon Sep 17 00:00:00 2001 From: niklasad1 Date: Wed, 14 Feb 2018 17:11:26 +0100 Subject: [PATCH 5/6] add target in debug --- hw/src/ledger.rs | 8 ++++---- hw/src/lib.rs | 9 +++++---- hw/src/trezor.rs | 8 ++++---- 3 files changed, 13 insertions(+), 12 deletions(-) diff --git a/hw/src/ledger.rs b/hw/src/ledger.rs index 39d5a7a9c74..130149c4738 100644 --- a/hw/src/ledger.rs +++ b/hw/src/ledger.rs @@ -372,20 +372,20 @@ impl EventHandler { impl libusb::Hotplug for EventHandler { fn device_arrived(&mut self, device: libusb::Device) { if let (Some(ledger), Ok(_)) = (self.ledger.upgrade(), Manager::is_valid_ledger(&device)) { - debug!("Ledger arrived"); + debug!(target: "hw", "Ledger arrived"); // Wait for the device to boot up thread::sleep(Duration::from_millis(1000)); if let Err(e) = ledger.update_devices() { - debug!("Ledger connect error: {:?}", e); + debug!(target: "hw", "Ledger connect error: {:?}", e); } } } fn device_left(&mut self, device: libusb::Device) { if let (Some(ledger), Ok(_)) = (self.ledger.upgrade(), Manager::is_valid_ledger(&device)) { - debug!("Ledger left"); + debug!(target: "hw", "Ledger left"); if let Err(e) = ledger.update_devices() { - debug!("Ledger disconnect error: {:?}", e); + debug!(target: "hw", "Ledger disconnect error: {:?}", e); } } } diff --git a/hw/src/lib.rs b/hw/src/lib.rs index dfc2b88fe2b..af428cc5a65 100644 --- a/hw/src/lib.rs +++ b/hw/src/lib.rs @@ -173,11 +173,12 @@ impl HardwareWalletManager { .name("hw_wallet_ledger".to_string()) .spawn(move || { if let Err(e) = l.update_devices() { - debug!("Ledger could not connect at startup, error: {}", e); + debug!(target: "hw", "Ledger couldn't connect at startup, error: {}", e); + //debug!("Ledger could not connect at startup, error: {}", e); } loop { usb_context_ledger.handle_events(Some(Duration::from_millis(500))) - .unwrap_or_else(|e| debug!("Ledger event handler error: {}", e)); + .unwrap_or_else(|e| debug!(target: "hw", "Ledger event handler error: {}", e)); if thread_exiting_ledger.load(atomic::Ordering::Acquire) { break; } @@ -190,11 +191,11 @@ impl HardwareWalletManager { .name("hw_wallet_trezor".to_string()) .spawn(move || { if let Err(e) = t.update_devices() { - debug!("Trezor could not connect at startup, error: {}", e); + debug!(target: "hw", "Trezor couldn't connect at startup, error: {}", e); } loop { usb_context_trezor.handle_events(Some(Duration::from_millis(500))) - .unwrap_or_else(|e| debug!("Trezor event handler error: {}", e)); + .unwrap_or_else(|e| debug!(target: "hw", "Trezor event handler error: {}", e)); if thread_exiting_trezor.load(atomic::Ordering::Acquire) { break; } diff --git a/hw/src/trezor.rs b/hw/src/trezor.rs index 1c3e3f54277..4105bb7b552 100644 --- a/hw/src/trezor.rs +++ b/hw/src/trezor.rs @@ -417,22 +417,22 @@ impl EventHandler { impl libusb::Hotplug for EventHandler { fn device_arrived(&mut self, _device: libusb::Device) { - debug!("Trezor V1 arrived"); + debug!(target: "hw", "Trezor V1 arrived"); if let Some(trezor) = self.trezor.upgrade() { // Wait for the device to boot up thread::sleep(Duration::from_millis(1000)); if let Err(e) = trezor.update_devices() { - debug!("Trezor V1 connect error: {:?}", e); + debug!(target: "hw", "Trezor V1 connect error: {:?}", e); } } } fn device_left(&mut self, _device: libusb::Device) { - debug!("Trezor V1 left"); + debug!(target: "hw", "Trezor V1 left"); if let Some(trezor) = self.trezor.upgrade() { if let Err(e) = trezor.update_devices() { - debug!("Trezor V1 disconnect error: {:?}", e); + debug!(target: "hw", "Trezor V1 disconnect error: {:?}", e); } } } From aaff9344fd58968590a91405045ca10d122f3a70 Mon Sep 17 00:00:00 2001 From: niklasad1 Date: Thu, 22 Feb 2018 23:21:37 +0100 Subject: [PATCH 6/6] Address feedback * Remove thread joining in HardwareWalletManager * Remove thread handlers in HardwareWalletManager because this makes them unused --- hw/src/lib.rs | 21 ++++++++------------- 1 file changed, 8 insertions(+), 13 deletions(-) diff --git a/hw/src/lib.rs b/hw/src/lib.rs index af428cc5a65..828c15d7085 100644 --- a/hw/src/lib.rs +++ b/hw/src/lib.rs @@ -130,7 +130,6 @@ impl From for Error { /// Hardware wallet management interface. pub struct HardwareWalletManager { - active_threads: Vec>>, exiting: Arc, ledger: Arc, trezor: Arc, @@ -142,7 +141,6 @@ impl HardwareWalletManager { pub fn new() -> Result { let usb_context_trezor = Arc::new(libusb::Context::new()?); let usb_context_ledger = Arc::new(libusb::Context::new()?); - let mut active_threads: Vec>> = Vec::new(); let hidapi = Arc::new(Mutex::new(hidapi::HidApi::new().map_err(|e| Error::Hid(e.to_string().clone()))?)); let ledger = Arc::new(ledger::Manager::new(hidapi.clone())); let trezor = Arc::new(trezor::Manager::new(hidapi.clone())); @@ -169,7 +167,7 @@ impl HardwareWalletManager { let t = trezor.clone(); // Ledger event thread - active_threads.push(thread::Builder::new() + thread::Builder::new() .name("hw_wallet_ledger".to_string()) .spawn(move || { if let Err(e) = l.update_devices() { @@ -184,10 +182,10 @@ impl HardwareWalletManager { } } }) - .ok()); + .ok(); // Trezor event thread - active_threads.push(thread::Builder::new() + thread::Builder::new() .name("hw_wallet_trezor".to_string()) .spawn(move || { if let Err(e) = t.update_devices() { @@ -201,10 +199,9 @@ impl HardwareWalletManager { } } }) - .ok()); + .ok(); Ok(HardwareWalletManager { - active_threads: active_threads, exiting: exiting, ledger: ledger, trezor: trezor, @@ -258,12 +255,10 @@ impl HardwareWalletManager { impl Drop for HardwareWalletManager { fn drop(&mut self) { + // Indicate to the USB Hotplug handlers that they + // shall terminate but don't wait for them to terminate. + // If they don't terminate for some reason USB Hotplug events will be handled + // even if the HardwareWalletManger has been dropped self.exiting.store(true, atomic::Ordering::Release); - for thread in self.active_threads.iter_mut() { - if let Some(thread) = thread.take() { - thread.thread().unpark(); - thread.join().ok(); - } - } } }