From b666895303cbb372c07b01a8542eda0e029bc46d Mon Sep 17 00:00:00 2001 From: matejcik Date: Thu, 9 Sep 2021 16:53:05 +0200 Subject: [PATCH] feat(core/rust): catch, handle and propagate uPy exceptions --- core/.changelog.d/1811.changed | 1 + core/embed/extmod/trezorobj.c | 18 +++ core/embed/extmod/trezorobj.h | 2 + core/embed/rust/build.rs | 15 ++- core/embed/rust/micropython.h | 1 + core/embed/rust/src/error.rs | 72 ++++++----- core/embed/rust/src/micropython/buffer.rs | 4 +- core/embed/rust/src/micropython/dict.rs | 13 +- core/embed/rust/src/micropython/gc.rs | 15 ++- core/embed/rust/src/micropython/iter.rs | 36 ++++-- core/embed/rust/src/micropython/list.rs | 18 +-- core/embed/rust/src/micropython/map.rs | 50 ++++---- core/embed/rust/src/micropython/obj.rs | 137 ++++++++++++++------- core/embed/rust/src/micropython/qstr.rs | 2 +- core/embed/rust/src/micropython/runtime.rs | 31 +++-- core/embed/rust/src/protobuf/decode.rs | 69 ++++++----- core/embed/rust/src/protobuf/defs.rs | 2 +- core/embed/rust/src/protobuf/encode.rs | 11 +- core/embed/rust/src/protobuf/error.rs | 32 +++++ core/embed/rust/src/protobuf/mod.rs | 1 + core/embed/rust/src/protobuf/obj.rs | 30 ++--- core/embed/rust/src/util.rs | 8 +- core/src/trezor/wire/__init__.py | 2 +- core/tests/test_trezor.protobuf.py | 2 +- 24 files changed, 375 insertions(+), 197 deletions(-) create mode 100644 core/.changelog.d/1811.changed create mode 100644 core/embed/rust/src/protobuf/error.rs diff --git a/core/.changelog.d/1811.changed b/core/.changelog.d/1811.changed new file mode 100644 index 0000000000..f561a1173b --- /dev/null +++ b/core/.changelog.d/1811.changed @@ -0,0 +1 @@ +Errors from protobuf decoding are now more expressive. diff --git a/core/embed/extmod/trezorobj.c b/core/embed/extmod/trezorobj.c index 29575266ba..153930d017 100644 --- a/core/embed/extmod/trezorobj.c +++ b/core/embed/extmod/trezorobj.c @@ -20,7 +20,9 @@ #include #include "memzero.h" +#include "py/obj.h" #include "py/objint.h" +#include "py/objstr.h" #include "py/runtime.h" static bool mpz_as_ll_checked(const mpz_t *i, long long *value) { @@ -74,3 +76,19 @@ mp_obj_t trezor_obj_call_protected(void (*func)(void *), void *arg) { return MP_OBJ_FROM_PTR(nlr.ret_val); } } + +mp_obj_t trezor_obj_str_from_rom_text(const char *str) { + // taken from mp_obj_new_exception_msg + mp_obj_str_t *o_str = m_new_obj_maybe(mp_obj_str_t); + if (o_str == NULL) return NULL; + + o_str->base.type = &mp_type_str; + o_str->len = strlen(str); + o_str->data = (const byte *)str; +#if MICROPY_ROM_TEXT_COMPRESSION + o_str->hash = 0; // will be computed only if string object is accessed +#else + o_str->hash = qstr_compute_hash(o_str->data, o_str->len); +#endif + return MP_OBJ_FROM_PTR(o_str); +} diff --git a/core/embed/extmod/trezorobj.h b/core/embed/extmod/trezorobj.h index 2fa43f5e44..b1efd7f776 100644 --- a/core/embed/extmod/trezorobj.h +++ b/core/embed/extmod/trezorobj.h @@ -79,4 +79,6 @@ bool trezor_obj_get_ll_checked(mp_obj_t obj, long long *value); mp_obj_t trezor_obj_call_protected(void (*func)(void *), void *arg); +mp_obj_t trezor_obj_str_from_rom_text(const char *str); + #endif diff --git a/core/embed/rust/build.rs b/core/embed/rust/build.rs index a845447389..add740f9d9 100644 --- a/core/embed/rust/build.rs +++ b/core/embed/rust/build.rs @@ -61,6 +61,7 @@ fn generate_micropython_bindings() { .allowlist_function("mp_call_function_n_kw") .allowlist_function("trezor_obj_get_ll_checked") .allowlist_function("trezor_obj_get_ull_checked") + .allowlist_function("trezor_obj_str_from_rom_text") // buffer .allowlist_function("mp_get_buffer") .allowlist_var("MP_BUFFER_READ") @@ -93,9 +94,19 @@ fn generate_micropython_bindings() { .allowlist_function("mp_map_init") .allowlist_function("mp_map_init_fixed_table") .allowlist_function("mp_map_lookup") - // runtime - .allowlist_function("mp_raise_ValueError") + // exceptions + .allowlist_function("nlr_jump") + .allowlist_function("mp_obj_new_exception") + .allowlist_function("mp_obj_new_exception_msg") + .allowlist_function("mp_obj_new_exception_arg1") + .allowlist_function("mp_obj_new_exception_args") .allowlist_function("trezor_obj_call_protected") + .allowlist_var("mp_type_AttributeError") + .allowlist_var("mp_type_KeyError") + .allowlist_var("mp_type_MemoryError") + .allowlist_var("mp_type_OverflowError") + .allowlist_var("mp_type_ValueError") + .allowlist_var("mp_type_TypeError") // typ .allowlist_var("mp_type_type"); diff --git a/core/embed/rust/micropython.h b/core/embed/rust/micropython.h index 9838a9cd30..eba890703e 100644 --- a/core/embed/rust/micropython.h +++ b/core/embed/rust/micropython.h @@ -1,4 +1,5 @@ #include "py/gc.h" +#include "py/nlr.h" #include "py/obj.h" #include "py/runtime.h" diff --git a/core/embed/rust/src/error.rs b/core/embed/rust/src/error.rs index 9977d9161a..ba6af4c475 100644 --- a/core/embed/rust/src/error.rs +++ b/core/embed/rust/src/error.rs @@ -1,46 +1,60 @@ -use core::convert::Infallible; -use core::fmt; - +use core::convert::{Infallible, TryInto}; use cstr_core::CStr; +use crate::micropython::{ffi, obj::Obj, qstr::Qstr}; + +#[derive(Debug)] pub enum Error { - Missing, + TypeError, OutOfRange, - InvalidType, - NotBuffer, - NotInt, - InvalidOperation, + MissingKwargs, + AllocationFailed, + CaughtException(Obj), + KeyError(Obj), + AttributeError(Qstr), + ValueError(&'static CStr), + ValueErrorParam(&'static CStr, Obj), } impl Error { - pub fn as_cstr(&self) -> &'static CStr { - // SAFETY: Safe because we are passing in \0-terminated literals. + /// Create an exception instance matching the error code. + /// The result of this call should only be used to immediately raise the + /// exception, because the object is not guaranteed to remain intact. + /// Micropython might reuse the same space for creating a different + /// exception. + pub unsafe fn to_obj(self) -> Obj { unsafe { - let cstr = |s: &'static str| CStr::from_bytes_with_nul_unchecked(s.as_bytes()); + // SAFETY: + // - first argument is a reference to a valid exception type + // EXCEPTION: Sensibly, `new_exception_*` does not raise. match self { - Error::Missing => cstr("Missing\0"), - Error::OutOfRange => cstr("OutOfRange\0"), - Error::InvalidType => cstr("InvalidType\0"), - Error::NotBuffer => cstr("NotBuffer\0"), - Error::NotInt => cstr("NotInt\0"), - Error::InvalidOperation => cstr("InvalidOperation\0"), + Error::TypeError => ffi::mp_obj_new_exception(&ffi::mp_type_TypeError), + Error::OutOfRange => ffi::mp_obj_new_exception(&ffi::mp_type_OverflowError), + Error::MissingKwargs => ffi::mp_obj_new_exception(&ffi::mp_type_TypeError), + Error::AllocationFailed => ffi::mp_obj_new_exception(&ffi::mp_type_MemoryError), + Error::CaughtException(obj) => obj, + Error::KeyError(key) => { + ffi::mp_obj_new_exception_arg1(&ffi::mp_type_KeyError, key.into()) + } + Error::ValueError(msg) => { + ffi::mp_obj_new_exception_msg(&ffi::mp_type_ValueError, msg.as_ptr()) + } + Error::ValueErrorParam(msg, param) => { + if let Ok(msg) = msg.try_into() { + let args: [Obj; 2] = [msg, param.into()]; + ffi::mp_obj_new_exception_args(&ffi::mp_type_ValueError, 2, args.as_ptr()) + } else { + ffi::mp_obj_new_exception(&ffi::mp_type_ValueError) + } + } + Error::AttributeError(attr) => { + ffi::mp_obj_new_exception_arg1(&ffi::mp_type_AttributeError, attr.into()) + } } } } } -impl From for &'static CStr { - fn from(val: Error) -> Self { - val.as_cstr() - } -} - -impl fmt::Debug for Error { - fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - self.as_cstr().fmt(f) - } -} - // Implements a conversion from `core::convert::Infallible` to `Error` to so // that code generic over `TryFrom` can work with values covered by the blanket // impl for `Into`: `https://doc.rust-lang.org/std/convert/enum.Infallible.html` diff --git a/core/embed/rust/src/micropython/buffer.rs b/core/embed/rust/src/micropython/buffer.rs index cac8d81e5e..d3da64938d 100644 --- a/core/embed/rust/src/micropython/buffer.rs +++ b/core/embed/rust/src/micropython/buffer.rs @@ -112,10 +112,12 @@ fn get_buffer_info(obj: Obj, flags: u32) -> Result // `bufinfo.buf` contains a pointer to data of `bufinfo.len` bytes. Later // we consider this data either GC-allocated or effectively `'static`, embedding // them in `Buffer`/`BufferMut`. + // EXCEPTION: Does not raise for Micropython's builtin types, and we don't + // implement custom buffer protocols. if unsafe { ffi::mp_get_buffer(obj, &mut bufinfo, flags as _) } { Ok(bufinfo) } else { - Err(Error::NotBuffer) + Err(Error::TypeError) } } diff --git a/core/embed/rust/src/micropython/dict.rs b/core/embed/rust/src/micropython/dict.rs index 2d4b9dd20a..77dcf10a4c 100644 --- a/core/embed/rust/src/micropython/dict.rs +++ b/core/embed/rust/src/micropython/dict.rs @@ -2,7 +2,7 @@ use core::convert::TryFrom; use crate::error::Error; -use super::{ffi, gc::Gc, map::Map, obj::Obj}; +use super::{ffi, gc::Gc, map::Map, obj::Obj, runtime::catch_exception}; /// Insides of the MicroPython `dict` object. pub type Dict = ffi::mp_obj_dict_t; @@ -10,13 +10,14 @@ pub type Dict = ffi::mp_obj_dict_t; impl Dict { /// Allocate a new dictionary on the GC heap, empty, but with a capacity of /// `capacity` items. - pub fn alloc_with_capacity(capacity: usize) -> Gc { - unsafe { + pub fn alloc_with_capacity(capacity: usize) -> Result, Error> { + catch_exception(|| unsafe { // SAFETY: We expect that `ffi::mp_obj_new_dict` either returns a GC-allocated - // pointer to `ffi::mp_obj_dict_t` or raises (i.e. on allocation failure). + // pointer to `ffi::mp_obj_dict_t` or raises. + // EXCEPTION: Will raise if allocation fails. let ptr = ffi::mp_obj_new_dict(capacity); Gc::from_raw(ptr.as_ptr().cast()) - } + }) } /// Constructs a dictionary definition by taking ownership of given [`Map`]. @@ -61,7 +62,7 @@ impl TryFrom for Gc { let this = unsafe { Gc::from_raw(value.as_ptr().cast()) }; Ok(this) } else { - Err(Error::InvalidType) + Err(Error::TypeError) } } } diff --git a/core/embed/rust/src/micropython/gc.rs b/core/embed/rust/src/micropython/gc.rs index 5e5c9de196..865498d649 100644 --- a/core/embed/rust/src/micropython/gc.rs +++ b/core/embed/rust/src/micropython/gc.rs @@ -4,6 +4,8 @@ use core::{ ptr::{self, NonNull}, }; +use crate::error::Error; + use super::ffi; /// A pointer type for values on the garbage-collected heap. @@ -15,7 +17,7 @@ pub struct Gc(NonNull); impl Gc { /// Allocate memory on the heap managed by the MicroPython garbage collector /// and then place `v` into it. `v` will _not_ get its destructor called. - pub fn new(v: T) -> Self { + pub fn new(v: T) -> Result { let layout = Layout::for_value(&v); // TODO: Assert that `layout.align()` is the same as the GC alignment. // SAFETY: @@ -23,10 +25,15 @@ impl Gc { // not support custom alignment. // - `ptr` is guaranteed to stay valid as long as it's reachable from the stack // or the MicroPython heap. + // EXCEPTION: Returns null instead of raising. unsafe { - let raw = ffi::gc_alloc(layout.size(), 0).cast(); - ptr::write(raw, v); - Self::from_raw(raw) + let raw = ffi::gc_alloc(layout.size(), 0); + if raw.is_null() { + return Err(Error::AllocationFailed); + } + let typed = raw.cast(); + ptr::write(typed, v); + Ok(Self::from_raw(typed)) } } diff --git a/core/embed/rust/src/micropython/iter.rs b/core/embed/rust/src/micropython/iter.rs index 90dbb5cbf3..61da51d3d7 100644 --- a/core/embed/rust/src/micropython/iter.rs +++ b/core/embed/rust/src/micropython/iter.rs @@ -4,6 +4,7 @@ use crate::error::Error; use crate::micropython::obj::Obj; use super::ffi; +use super::runtime::catch_exception; pub struct IterBuf { iter_buf: ffi::mp_obj_iter_buf_t, @@ -26,6 +27,7 @@ pub struct Iter<'a> { iter: Obj, iter_buf: &'a mut IterBuf, finished: bool, + caught_exception: Obj, } impl<'a> Iter<'a> { @@ -35,16 +37,24 @@ impl<'a> Iter<'a> { // uses memory from the passed `iter_buf`. We maintain this invariant by // taking a mut ref to `IterBuf` and tying it to the lifetime of returned // `Iter`. - // - Raises if `o` is not iterable and in other cases as well. // - Returned obj is referencing into `iter_buf`. - // TODO: Use a fn that doesn't raise on non-iterable object. - let iter = unsafe { ffi::mp_getiter(o, &mut iter_buf.iter_buf) }; + // EXCEPTION: Raises if `o` is not iterable and possibly in other cases too. + let iter = catch_exception(|| unsafe { ffi::mp_getiter(o, &mut iter_buf.iter_buf) })?; Ok(Self { iter, iter_buf, finished: false, + caught_exception: Obj::const_null(), }) } + + pub fn error(&self) -> Option { + if self.caught_exception.is_null() { + None + } else { + Some(Error::CaughtException(self.caught_exception)) + } + } } impl<'a> Iterator for Iter<'a> { @@ -57,13 +67,19 @@ impl<'a> Iterator for Iter<'a> { // SAFETY: // - We assume that `mp_iternext` returns objects without any lifetime // invariants, i.e. heap-allocated, unlike `mp_getiter`. - // - Can raise. - let item = unsafe { ffi::mp_iternext(self.iter) }; - if item == Obj::const_stop_iteration() { - self.finished = true; - None - } else { - Some(item) + // EXCEPTION: Can raise from the underlying iterator. + let item = catch_exception(|| unsafe { ffi::mp_iternext(self.iter) }); + match item { + Err(Error::CaughtException(exc)) => { + self.caught_exception = exc; + None + } + Err(_) => panic!("unexpected error"), + Ok(item) if item == Obj::const_stop_iteration() => { + self.finished = true; + None + } + Ok(item) => Some(item), } } } diff --git a/core/embed/rust/src/micropython/list.rs b/core/embed/rust/src/micropython/list.rs index 23882d50c7..4811459be6 100644 --- a/core/embed/rust/src/micropython/list.rs +++ b/core/embed/rust/src/micropython/list.rs @@ -2,25 +2,29 @@ use core::convert::TryFrom; use crate::error::Error; -use super::{ffi, gc::Gc, obj::Obj}; +use super::{ffi, gc::Gc, obj::Obj, runtime::catch_exception}; pub type List = ffi::mp_obj_list_t; impl List { - pub fn alloc(values: &[Obj]) -> Gc { + pub fn alloc(values: &[Obj]) -> Result, Error> { // SAFETY: Although `values` are copied into the new list and not mutated, // `mp_obj_new_list` is taking them through a mut pointer. - unsafe { + // EXCEPTION: Will raise if allocation fails. + catch_exception(|| unsafe { let list = ffi::mp_obj_new_list(values.len(), values.as_ptr() as *mut Obj); Gc::from_raw(list.as_ptr().cast()) - } + }) } - pub fn append(&mut self, value: Obj) { + pub fn append(&mut self, value: Obj) -> Result<(), Error> { unsafe { let ptr = self as *mut Self; let list = Obj::from_ptr(ptr.cast()); - ffi::mp_obj_list_append(list, value); + // EXCEPTION: Will raise if allocation fails. + catch_exception(|| { + ffi::mp_obj_list_append(list, value); + }) } } } @@ -44,7 +48,7 @@ impl TryFrom for Gc { let this = unsafe { Gc::from_raw(value.as_ptr().cast()) }; Ok(this) } else { - Err(Error::InvalidType) + Err(Error::TypeError) } } } diff --git a/core/embed/rust/src/micropython/map.rs b/core/embed/rust/src/micropython/map.rs index de032878dd..895d35d4ed 100644 --- a/core/embed/rust/src/micropython/map.rs +++ b/core/embed/rust/src/micropython/map.rs @@ -5,7 +5,7 @@ use crate::{ micropython::{obj::Obj, qstr::Qstr}, }; -use super::ffi; +use super::{ffi, runtime::catch_exception}; pub type Map = ffi::mp_map_t; pub type MapElem = ffi::mp_map_elem_t; @@ -42,20 +42,24 @@ impl Map { impl Map { pub fn from_fixed<'a>(table: &'a [MapElem]) -> MapRef<'a> { let mut map = MaybeUninit::uninit(); - // SAFETY: `mp_map_init` completely initializes all fields of `map`. + // SAFETY: `mp_map_init_fixed_table` completely initializes all fields of `map`. unsafe { + // EXCEPTION: Does not raise. ffi::mp_map_init_fixed_table(map.as_mut_ptr(), table.len(), table.as_ptr().cast()); MapRef::new(map.assume_init()) } } - pub fn with_capacity(capacity: usize) -> Self { + pub fn with_capacity(capacity: usize) -> Result { let mut map = MaybeUninit::uninit(); // SAFETY: `mp_map_init` completely initializes all fields of `map`, allocating // the backing storage for `capacity` items on the heap. unsafe { - ffi::mp_map_init(map.as_mut_ptr(), capacity); - map.assume_init() + // EXCEPTION: Will raise if allocation fails. + catch_exception(|| { + ffi::mp_map_init(map.as_mut_ptr(), capacity); + })?; + Ok(map.assume_init()) } } @@ -86,18 +90,19 @@ impl Map { // cast to mut ptr is therefore safe. unsafe { let map = self as *const Self as *mut Self; + // EXCEPTION: Does not raise for MP_MAP_LOOKUP. let elem = ffi::mp_map_lookup(map, index, ffi::_mp_map_lookup_kind_t_MP_MAP_LOOKUP) .as_ref() - .ok_or(Error::Missing)?; + .ok_or(Error::KeyError(index))?; Ok(elem.value) } } - pub fn set(&mut self, index: impl Into, value: impl Into) { + pub fn set(&mut self, index: impl Into, value: impl Into) -> Result<(), Error> { self.set_obj(index.into(), value.into()) } - pub fn set_obj(&mut self, index: Obj, value: Obj) { + pub fn set_obj(&mut self, index: Obj, value: Obj) -> Result<(), Error> { // SAFETY: // - `mp_map_lookup` with `_mp_map_lookup_kind_t_MP_MAP_LOOKUP_ADD_IF_NOT_FOUND // returns a pointer to a `mp_map_elem_t` value with a lifetime valid for the @@ -107,15 +112,19 @@ impl Map { // replace it. unsafe { let map = self as *mut Self; - let elem = ffi::mp_map_lookup( - map, - index, - ffi::_mp_map_lookup_kind_t_MP_MAP_LOOKUP_ADD_IF_NOT_FOUND, - ) + // EXCEPTION: Will raise if allocation fails. + let elem = catch_exception(|| { + ffi::mp_map_lookup( + map, + index, + ffi::_mp_map_lookup_kind_t_MP_MAP_LOOKUP_ADD_IF_NOT_FOUND, + ) + })? .as_mut() .unwrap(); elem.value = value; } + Ok(()) } pub fn delete(&mut self, index: impl Into) { @@ -125,6 +134,7 @@ impl Map { pub fn delete_obj(&mut self, index: Obj) { unsafe { let map = self as *mut Self; + // EXCEPTION: Does not raise for MP_MAP_LOOKUP_REMOVE_IF_FOUND. ffi::mp_map_lookup( map, index, @@ -134,9 +144,9 @@ impl Map { } } -impl Clone for Map { - fn clone(&self) -> Self { - let mut map = Self::with_capacity(self.len()); +impl Map { + pub fn try_clone(&self) -> Result { + let mut map = Self::with_capacity(self.len())?; unsafe { ptr::copy_nonoverlapping(self.table, map.table, self.len()); } @@ -144,13 +154,7 @@ impl Clone for Map { map.set_all_keys_are_qstrs(self.all_keys_are_qstrs()); map.set_is_ordered(self.is_ordered()); map.set_is_fixed(0); - map - } -} - -impl Default for Map { - fn default() -> Self { - Self::with_capacity(0) + Ok(map) } } diff --git a/core/embed/rust/src/micropython/obj.rs b/core/embed/rust/src/micropython/obj.rs index df4c4bbdce..40a3186e2c 100644 --- a/core/embed/rust/src/micropython/obj.rs +++ b/core/embed/rust/src/micropython/obj.rs @@ -1,9 +1,12 @@ use core::convert::{TryFrom, TryInto}; use core::num::TryFromIntError; +use cstr_core::CStr; + use crate::error::Error; use super::ffi; +use super::runtime::catch_exception; pub type Obj = ffi::mp_obj_t; pub type ObjBase = ffi::mp_obj_base_t; @@ -99,11 +102,13 @@ impl Obj { } impl Obj { - pub fn call_with_n_args(self, args: &[Obj]) -> Obj { + pub fn call_with_n_args(self, args: &[Obj]) -> Result { // SAFETY: - // - Can call python code and therefore raise. // - Each of `args` has no lifetime bounds. - unsafe { ffi::mp_call_function_n_kw(self, args.len(), 0, args.as_ptr()) } + // EXCEPTION: Calls Python code so can raise arbitrarily. + catch_exception(|| unsafe { + ffi::mp_call_function_n_kw(self, args.len(), 0, args.as_ptr()) + }) } } @@ -117,8 +122,9 @@ impl TryFrom for bool { fn try_from(obj: Obj) -> Result { // TODO: Avoid type casts on the Python side. // SAFETY: - // - Can call python code and therefore raise. - if unsafe { ffi::mp_obj_is_true(obj) } { + // - `obj` can be anything uPy understands. + // EXCEPTION: Can call Python code (on custom instances) and therefore raise. + if catch_exception(|| unsafe { ffi::mp_obj_is_true(obj) })? { Ok(true) } else { Ok(false) @@ -134,12 +140,15 @@ impl TryFrom for i32 { // TODO: Avoid type casts on the Python side. // SAFETY: - // - Can raise if `obj` is int but cannot fit into `cty::mp_int_t`. - if unsafe { ffi::mp_obj_get_int_maybe(obj.as_ptr(), &mut int) } { - let int = int.try_into()?; - Ok(int) + // - `int` is a mutable pointer of the right type. + // - `obj` can be anything uPy understands. + // EXCEPTION: Can raise if `obj` is int but cannot fit into `cty::mp_int_t`. + let result = + catch_exception(|| unsafe { ffi::mp_obj_get_int_maybe(obj.as_ptr(), &mut int) })?; + if result { + Ok(int.try_into()?) } else { - Err(Error::NotInt) + Err(Error::TypeError) } } } @@ -150,10 +159,14 @@ impl TryFrom for i64 { fn try_from(obj: Obj) -> Result { let mut ll: cty::c_longlong = 0; + // SAFETY: + // - `ll` is a mutable variable of the right type. + // - `obj` can be anything uPy understands. + // EXCEPTION: Does not raise. if unsafe { ffi::trezor_obj_get_ll_checked(obj, &mut ll) } { Ok(ll) } else { - Err(Error::NotInt) + Err(Error::TypeError) } } } @@ -172,76 +185,104 @@ impl From for Obj { } } -impl From for Obj { - fn from(val: i32) -> Self { +impl TryFrom for Obj { + type Error = Error; + + fn try_from(val: i32) -> Result { // `mp_obj_new_int` accepts a `mp_int_t` argument, which is word-sized. We // primarily target 32-bit architecture, and therefore keep the primary signed // conversion type as `i32`, but convert through `into()` if the types differ. - // SAFETY: - // - Can raise if allocation fails. - unsafe { ffi::mp_obj_new_int(val.into()) } + // EXCEPTION: Can raise if `val` is larger than smallint and allocation fails. + catch_exception(|| unsafe { ffi::mp_obj_new_int(val.into()) }) } } -impl From for Obj { - fn from(val: i64) -> Self { +impl TryFrom for Obj { + type Error = Error; + + fn try_from(val: i64) -> Result { // Because `mp_obj_new_int_from_ll` allocates even if `val` fits into small-int, // we try to go through `mp_obj_new_int` first. match i32::try_from(val) { - Ok(smaller_val) => smaller_val.into(), - // SAFETY: - // - Can raise if allocation fails. - Err(_) => unsafe { ffi::mp_obj_new_int_from_ll(val) }, + Ok(smaller_val) => smaller_val.try_into(), + // EXCEPTION: Will raise if allocation fails. + Err(_) => catch_exception(|| unsafe { ffi::mp_obj_new_int_from_ll(val) }), } } } -impl From for Obj { - fn from(val: u32) -> Self { +impl TryFrom for Obj { + type Error = Error; + + fn try_from(val: u32) -> Result { // `mp_obj_new_int_from_uint` accepts a `mp_uint_t` argument, which is // word-sized. We primarily target 32-bit architecture, and therefore keep // the primary unsigned conversion type as `u32`, but convert through `into()` // if the types differ. - // SAFETY: - // - Can raise if allocation fails. - unsafe { ffi::mp_obj_new_int_from_uint(val.into()) } + // EXCEPTION: Can raise if `val` is larger than smallint and allocation fails. + catch_exception(|| unsafe { ffi::mp_obj_new_int_from_uint(val.into()) }) } } -impl From for Obj { - fn from(val: u64) -> Self { +impl TryFrom for Obj { + type Error = Error; + + fn try_from(val: u64) -> Result { // Because `mp_obj_new_int_from_ull` allocates even if `val` fits into // small-int, we try to go through `mp_obj_new_int_from_uint` first. match u32::try_from(val) { - Ok(smaller_val) => smaller_val.into(), - // SAFETY: - // - Can raise if allocation fails. - Err(_) => unsafe { ffi::mp_obj_new_int_from_ull(val) }, + Ok(smaller_val) => smaller_val.try_into(), + // EXCEPTION: Will raise if allocation fails. + Err(_) => catch_exception(|| unsafe { ffi::mp_obj_new_int_from_ull(val) }), } } } /// Byte slices are converted into `bytes` MicroPython objects, by allocating /// new space on the heap and copying. -impl From<&[u8]> for Obj { - fn from(val: &[u8]) -> Self { +impl TryFrom<&[u8]> for Obj { + type Error = Error; + + fn try_from(val: &[u8]) -> Result { // SAFETY: - // - Can raise if allocation fails. - unsafe { ffi::mp_obj_new_bytes(val.as_ptr(), val.len()) } + // - Should work with any data + // EXCEPTION: Will raise if allocation fails. + catch_exception(|| unsafe { ffi::mp_obj_new_bytes(val.as_ptr(), val.len()) }) } } /// String slices are converted into `str` MicroPython objects. Strings that are /// already interned will turn up as QSTRs, strings not found in the QSTR pool /// will be allocated on the heap and copied. -impl From<&str> for Obj { - fn from(val: &str) -> Self { +impl TryFrom<&str> for Obj { + type Error = Error; + + fn try_from(val: &str) -> Result { // SAFETY: - // - Can raise if allocation fails. // - `str` is guaranteed to be UTF-8. - unsafe { ffi::mp_obj_new_str(val.as_ptr().cast(), val.len()) } + // EXCEPTION: Will raise if allocation fails. + catch_exception(|| unsafe { ffi::mp_obj_new_str(val.as_ptr().cast(), val.len()) }) + } +} + +/// ROM null-terminated strings can be converted to `str` MicroPython objects +/// without making a copy on the heap, only allocating the `mp_obj_str_t` +/// struct. +impl TryFrom<&'static CStr> for Obj { + type Error = Error; + + fn try_from(val: &'static CStr) -> Result { + // SAFETY: + // - `CStr` is guaranteed to be null-terminated UTF-8. + // - the argument is static so it will remain valid for the lifetime of result. + let obj = unsafe { ffi::trezor_obj_str_from_rom_text(val.as_ptr()) }; + if obj.is_null() { + Err(Error::AllocationFailed) + } else { + Ok(obj) + } } } @@ -251,20 +292,24 @@ impl From<&str> for Obj { impl From for Obj { fn from(val: u8) -> Self { - u32::from(val).into() + // u8 will fit into smallint so no error should happen here + u32::from(val).try_into().unwrap() } } impl From for Obj { fn from(val: u16) -> Self { - u32::from(val).into() + // u16 will fit into smallint so no error should happen here + u32::from(val).try_into().unwrap() } } -impl From for Obj { - fn from(val: usize) -> Self { +impl TryFrom for Obj { + type Error = Error; + + fn try_from(val: usize) -> Result { // Willingly truncate the bits on 128-bit architectures. - (val as u64).into() + (val as u64).try_into() } } diff --git a/core/embed/rust/src/micropython/qstr.rs b/core/embed/rust/src/micropython/qstr.rs index 1a6425a492..0eda8d33f1 100644 --- a/core/embed/rust/src/micropython/qstr.rs +++ b/core/embed/rust/src/micropython/qstr.rs @@ -45,7 +45,7 @@ impl TryFrom for Qstr { if value.is_qstr() { Ok(Self::from_obj_bits(value.as_bits())) } else { - Err(Error::InvalidType) + Err(Error::TypeError) } } } diff --git a/core/embed/rust/src/micropython/runtime.rs b/core/embed/rust/src/micropython/runtime.rs index 7c7b451210..fb4eccc7f2 100644 --- a/core/embed/rust/src/micropython/runtime.rs +++ b/core/embed/rust/src/micropython/runtime.rs @@ -1,19 +1,26 @@ use core::mem::MaybeUninit; -use cstr_core::CStr; +use crate::error::Error; use super::{ffi, obj::Obj}; -pub fn raise_value_error(msg: &'static CStr) -> ! { +/// Raise a micropython exception via NLR jump. +/// Jumps directly out of the context without running any destructors, +/// finalizers, etc. This is very likely to break a lot of Rust's assumptions. +/// Should only be called in places where you really don't care anymore. +pub unsafe fn raise_exception(err: Error) -> ! { unsafe { - ffi::mp_raise_ValueError(msg.as_ptr()); + // SAFETY: + // - argument must be an exception instance + // (err.to_obj() should return the right thing) + ffi::nlr_jump(err.to_obj().as_ptr()); } panic!(); } /// Execute `func` while catching MicroPython exceptions. Returns `Ok` in the /// successful case, and `Err` with the caught `Obj` in case of a raise. -pub fn except(mut func: F) -> Result +pub fn catch_exception(mut func: F) -> Result where F: FnMut() -> T, { @@ -29,15 +36,16 @@ where let mut wrapper = || { result = MaybeUninit::new(func()); }; - // `wrapper` is a closure, and to pass it over the FFI, we split it into a function - // pointer, and a user-data pointer. `ffi::trezor_obj_call_protected` then calls - // the `callback` with the `argument`. + // `wrapper` is a closure, and to pass it over the FFI, we split it into a + // function pointer, and a user-data pointer. + // `ffi::trezor_obj_call_protected` then calls the `callback` with the + // `argument`. let (callback, argument) = split_func_into_callback_and_argument(&mut wrapper); let exception = ffi::trezor_obj_call_protected(Some(callback), argument); if exception == Obj::const_none() { Ok(result.assume_init()) } else { - Err(exception) + Err(Error::CaughtException(exception)) } } } @@ -73,15 +81,14 @@ mod tests { #[test] fn except_returns_ok_on_no_exception() { - let result = except(|| 1); + let result = catch_exception(|| 1); assert!(result.is_ok()); assert_eq!(result.unwrap(), 1); } #[test] - fn except_catches_value_error() { - let msg = unsafe { CStr::from_bytes_with_nul_unchecked(b"msg\0") }; - let result = except(|| raise_value_error(&msg)); + fn except_catches_raised() { + let result = catch_exception(|| raise_exception(Error::TypeError)); assert!(result.is_err()); } } diff --git a/core/embed/rust/src/protobuf/decode.rs b/core/embed/rust/src/protobuf/decode.rs index e95e2fc0ae..0629d38a8f 100644 --- a/core/embed/rust/src/protobuf/decode.rs +++ b/core/embed/rust/src/protobuf/decode.rs @@ -7,6 +7,8 @@ use crate::{ util, }; +use super::error; + use super::{ defs::{self, FieldDef, FieldType, MsgDef}, obj::{MsgDefObj, MsgObj}, @@ -17,8 +19,8 @@ use super::{ pub extern "C" fn protobuf_type_for_name(name: Obj) -> Obj { util::try_or_raise(|| { let name = Qstr::try_from(name)?; - let def = MsgDef::for_name(name.to_u16()).ok_or(Error::Missing)?; - let obj = MsgDefObj::alloc(def).into(); + let def = MsgDef::for_name(name.to_u16()).ok_or_else(|| Error::KeyError(name.into()))?; + let obj = MsgDefObj::alloc(def)?.into(); Ok(obj) }) } @@ -27,8 +29,8 @@ pub extern "C" fn protobuf_type_for_name(name: Obj) -> Obj { pub extern "C" fn protobuf_type_for_wire(wire_id: Obj) -> Obj { util::try_or_raise(|| { let wire_id = u16::try_from(wire_id)?; - let def = MsgDef::for_wire_id(wire_id).ok_or(Error::Missing)?; - let obj = MsgDefObj::alloc(def).into(); + let def = MsgDef::for_wire_id(wire_id).ok_or_else(|| Error::KeyError(wire_id.into()))?; + let obj = MsgDefObj::alloc(def)?.into(); Ok(obj) }) } @@ -45,7 +47,7 @@ pub extern "C" fn protobuf_decode(buf: Obj, msg_def: Obj, enable_experimental: O // explicitly allowed. Messages can also mark certain fields as // experimental (not the whole message). This is enforced during the // decoding. - return Err(Error::InvalidType); + return Err(error::experimental_not_enabled()); } let stream = &mut InputStream::new(&buf); @@ -70,7 +72,7 @@ impl Decoder { stream: &mut InputStream, msg: &MsgDef, ) -> Result { - let mut obj = self.empty_message(msg); + let mut obj = self.empty_message(msg)?; // SAFETY: We assume that `obj` is not aliased here. let map = unsafe { Gc::as_mut(&mut obj) }.map_mut(); self.decode_fields_into(stream, msg, map)?; @@ -82,11 +84,11 @@ impl Decoder { /// Create a new message instance and fill it from `values`, handling the /// default and required fields correctly. pub fn message_from_values(&self, values: &Map, msg: &MsgDef) -> Result { - let mut obj = self.empty_message(msg); + let mut obj = self.empty_message(msg)?; // SAFETY: We assume that `obj` is not aliased here. let map = unsafe { Gc::as_mut(&mut obj) }.map_mut(); for elem in values.elems() { - map.set(elem.key, elem.value); + map.set(elem.key, elem.value)?; } self.decode_defaults_into(msg, map)?; self.assign_required_into(msg, map)?; @@ -95,7 +97,7 @@ impl Decoder { /// Allocate the backing message object with enough pre-allocated space for /// all fields. - pub fn empty_message(&self, msg: &MsgDef) -> Gc { + pub fn empty_message(&self, msg: &MsgDef) -> Result, Error> { MsgObj::alloc_with_capacity(msg.fields.len(), msg) } @@ -126,14 +128,14 @@ impl Decoder { // SAFETY: We assume that `list` is not aliased here. This holds for // uses in `message_from_stream` and `message_from_values`, because we // start with an empty `Map` and fill with unique lists. - unsafe { Gc::as_mut(&mut list) }.append(field_value); + unsafe { Gc::as_mut(&mut list) }.append(field_value)?; } else { - let list = List::alloc(&[field_value]); - map.set(field_name, list); + let list = List::alloc(&[field_value])?; + map.set(field_name, list)?; } } else { // Singular field, assign the value directly. - map.set(field_name, field_value); + map.set(field_name, field_value)?; } } None => { @@ -148,7 +150,7 @@ impl Decoder { stream.read(len)?; } _ => { - return Err(Error::InvalidType); + return Err(error::unknown_field_type()); } } } @@ -169,7 +171,9 @@ impl Decoder { // We need to look to the field descriptor to know how to interpret the value // after the field tag. while let Ok(field_tag) = stream.read_byte() { - let field = msg.field(field_tag).ok_or(Error::Missing)?; + let field = msg + .field(field_tag) + .ok_or_else(|| Error::KeyError(field_tag.into()))?; let field_name = Qstr::from(field.name); if map.contains_key(field_name) { // Field already has a value assigned, skip it. @@ -183,13 +187,13 @@ impl Decoder { stream.read(len)?; } _ => { - return Err(Error::InvalidType); + return Err(error::unknown_field_type()); } } } else { // Decode the value and assign it. let field_value = self.decode_field(stream, field)?; - map.set(field_name, field_value); + map.set(field_name, field_value)?; } } Ok(()) @@ -206,14 +210,14 @@ impl Decoder { } if field.is_required() { // Required field is missing, abort. - return Err(Error::Missing); + return Err(error::missing_required_field(field_name)); } if field.is_repeated() { // Optional repeated field, set to a new empty list. - map.set(field_name, List::alloc(&[])); + map.set(field_name, List::alloc(&[])?)?; } else { // Optional singular field, set to None. - map.set(field_name, Obj::const_none()); + map.set(field_name, Obj::const_none())?; } } Ok(()) @@ -222,14 +226,14 @@ impl Decoder { /// Decode one field value from the input stream. fn decode_field(&self, stream: &mut InputStream, field: &FieldDef) -> Result { if field.is_experimental() && !self.enable_experimental { - return Err(Error::InvalidType); + return Err(error::experimental_not_enabled()); } let num = stream.read_uvarint()?; match field.get_type() { - FieldType::UVarInt => Ok(num.into()), + FieldType::UVarInt => Ok(num.try_into()?), FieldType::SVarInt => { let signed_int = zigzag::to_signed(num); - Ok(signed_int.into()) + Ok(signed_int.try_into()?) } FieldType::Bool => { let boolean = num != 0; @@ -238,20 +242,21 @@ impl Decoder { FieldType::Bytes => { let buf_len = num.try_into()?; let buf = stream.read(buf_len)?; - Ok(buf.into()) + buf.try_into() } FieldType::String => { let buf_len = num.try_into()?; let buf = stream.read(buf_len)?; - let unicode = str::from_utf8(buf).map_err(|_| Error::InvalidType)?; - Ok(unicode.into()) + let unicode = + str::from_utf8(buf).map_err(|_| error::invalid_value(field.name.into()))?; + unicode.try_into() } FieldType::Enum(enum_type) => { let enum_val = num.try_into()?; if enum_type.values.contains(&enum_val) { Ok(enum_val.into()) } else { - Err(Error::InvalidType) + Err(error::invalid_value(field.name.into())) } } FieldType::Msg(msg_type) => { @@ -277,7 +282,7 @@ impl<'a> InputStream<'a> { let buf = self .buf .get(self.pos..self.pos + len) - .ok_or(Error::Missing)?; + .ok_or_else(error::end_of_buffer)?; self.pos += len; Ok(Self::new(buf)) } @@ -286,13 +291,17 @@ impl<'a> InputStream<'a> { let buf = self .buf .get(self.pos..self.pos + len) - .ok_or(Error::Missing)?; + .ok_or_else(error::end_of_buffer)?; self.pos += len; Ok(buf) } pub fn read_byte(&mut self) -> Result { - let val = self.buf.get(self.pos).copied().ok_or(Error::Missing)?; + let val = self + .buf + .get(self.pos) + .copied() + .ok_or_else(error::end_of_buffer)?; self.pos += 1; Ok(val) } diff --git a/core/embed/rust/src/protobuf/defs.rs b/core/embed/rust/src/protobuf/defs.rs index 5d07789c83..eb61885355 100644 --- a/core/embed/rust/src/protobuf/defs.rs +++ b/core/embed/rust/src/protobuf/defs.rs @@ -129,7 +129,7 @@ pub fn find_name_by_msg_offset(msg_offset: u16) -> Result { .filter(|def| def.msg_offset == msg_offset) .next() .map(|def| def.msg_name) - .ok_or(Error::Missing) + .ok_or_else(|| Error::KeyError(msg_offset.into())) } fn find_msg_offset_by_name(msg_name: u16) -> Option { diff --git a/core/embed/rust/src/protobuf/encode.rs b/core/embed/rust/src/protobuf/encode.rs index 4cf0112fdb..78883e714e 100644 --- a/core/embed/rust/src/protobuf/encode.rs +++ b/core/embed/rust/src/protobuf/encode.rs @@ -1,4 +1,4 @@ -use core::convert::TryFrom; +use core::convert::{TryFrom, TryInto}; use crate::{ error::Error, @@ -15,6 +15,7 @@ use crate::{ use super::{ defs::{FieldDef, FieldType, MsgDef}, + error, obj::MsgObj, zigzag, }; @@ -28,7 +29,7 @@ pub extern "C" fn protobuf_len(obj: Obj) -> Obj { Encoder.encode_message(stream, &obj.def(), &obj)?; - Ok(stream.len.into()) + Ok(stream.len.try_into()?) }) } @@ -46,7 +47,7 @@ pub extern "C" fn protobuf_encode(buf: Obj, obj: Obj) -> Obj { Encoder.encode_message(stream, &obj.def(), &obj)?; - Ok(stream.len().into()) + Ok(stream.len().try_into()?) }) } @@ -220,7 +221,7 @@ impl<'a> OutputStream for BufferStream<'a> { *pos += len; buf.copy_from_slice(val); }) - .ok_or(Error::Missing) + .ok_or_else(error::end_of_buffer) } fn write_byte(&mut self, val: u8) -> Result<(), Error> { @@ -231,6 +232,6 @@ impl<'a> OutputStream for BufferStream<'a> { *pos += 1; *buf = val; }) - .ok_or(Error::Missing) + .ok_or_else(error::end_of_buffer) } } diff --git a/core/embed/rust/src/protobuf/error.rs b/core/embed/rust/src/protobuf/error.rs new file mode 100644 index 0000000000..1e8625e65a --- /dev/null +++ b/core/embed/rust/src/protobuf/error.rs @@ -0,0 +1,32 @@ +use cstr_core::CStr; + +use crate::error::Error; +use crate::micropython::qstr::Qstr; + +/* XXX const version of from_bytes_with_nul_unchecked is nightly-only */ + +pub fn experimental_not_enabled() -> Error { + let msg = + unsafe { CStr::from_bytes_with_nul_unchecked(b"Experimental features are disabled.\0") }; + Error::ValueError(msg) +} + +pub fn unknown_field_type() -> Error { + let msg = unsafe { CStr::from_bytes_with_nul_unchecked(b"Unknown field type.\0") }; + Error::ValueError(msg) +} + +pub fn missing_required_field(field: Qstr) -> Error { + let msg = unsafe { CStr::from_bytes_with_nul_unchecked(b"Missing required field\0") }; + Error::ValueErrorParam(msg, field.into()) +} + +pub fn invalid_value(field: Qstr) -> Error { + let msg = unsafe { CStr::from_bytes_with_nul_unchecked(b"Invalid value for field\0") }; + Error::ValueErrorParam(msg, field.into()) +} + +pub fn end_of_buffer() -> Error { + let msg = unsafe { CStr::from_bytes_with_nul_unchecked(b"End of buffer.\0") }; + Error::ValueError(msg) +} diff --git a/core/embed/rust/src/protobuf/mod.rs b/core/embed/rust/src/protobuf/mod.rs index 6731fc4479..2960aeb69c 100644 --- a/core/embed/rust/src/protobuf/mod.rs +++ b/core/embed/rust/src/protobuf/mod.rs @@ -1,5 +1,6 @@ mod decode; mod defs; mod encode; +mod error; mod obj; mod zigzag; diff --git a/core/embed/rust/src/protobuf/obj.rs b/core/embed/rust/src/protobuf/obj.rs index bfd6544d6f..ead0b719b1 100644 --- a/core/embed/rust/src/protobuf/obj.rs +++ b/core/embed/rust/src/protobuf/obj.rs @@ -26,10 +26,10 @@ pub struct MsgObj { } impl MsgObj { - pub fn alloc_with_capacity(capacity: usize, msg: &MsgDef) -> Gc { + pub fn alloc_with_capacity(capacity: usize, msg: &MsgDef) -> Result, Error> { Gc::new(Self { base: Self::obj_type().to_base(), - map: Map::with_capacity(capacity), + map: Map::with_capacity(capacity)?, msg_wire_id: msg.wire_id, msg_offset: msg.offset, }) @@ -79,23 +79,23 @@ impl MsgObj { // Conversion to dict. Allocate a new dict object with a copy of our map // and return it. This is a bit different from how uPy does it now, because // we're returning a mutable dict. - Ok(Gc::new(Dict::with_map(self.map.clone())).into()) + Ok(Gc::new(Dict::with_map(self.map.try_clone()?))?.into()) } - _ => Err(Error::Missing), + _ => Err(Error::AttributeError(attr)), } } fn setattr(&mut self, attr: Qstr, value: Obj) -> Result<(), Error> { if value == Obj::const_null() { // this would be a delattr - return Err(Error::InvalidOperation); + return Err(Error::TypeError); } if self.map.contains_key(attr) { - self.map.set(attr, value); + self.map.set(attr, value)?; Ok(()) } else { - Err(Error::Missing) + Err(Error::AttributeError(attr)) } } } @@ -120,7 +120,7 @@ impl TryFrom for Gc { let this = unsafe { Gc::from_raw(value.as_ptr().cast()) }; Ok(this) } else { - Err(Error::InvalidType) + Err(Error::TypeError) } } } @@ -152,11 +152,12 @@ pub struct MsgDefObj { } impl MsgDefObj { - pub fn alloc(def: MsgDef) -> Gc { - Gc::new(Self { + pub fn alloc(def: MsgDef) -> Result, Error> { + let this = Gc::new(Self { base: Self::obj_type().to_base(), def, - }) + })?; + Ok(this) } pub fn msg(&self) -> &MsgDef { @@ -193,7 +194,7 @@ impl TryFrom for Gc { let this = unsafe { Gc::from_raw(value.as_ptr().cast()) }; Ok(this) } else { - Err(Error::InvalidType) + Err(Error::TypeError) } } } @@ -204,7 +205,8 @@ unsafe extern "C" fn msg_def_obj_attr(self_in: Obj, attr: ffi::qstr, dest: *mut let attr = Qstr::from_u16(attr as _); if unsafe { dest.read() } != Obj::const_null() { - return Err(Error::InvalidOperation); + // this would be a setattr + return Err(Error::TypeError); } match attr { @@ -235,7 +237,7 @@ unsafe extern "C" fn msg_def_obj_attr(self_in: Obj, attr: ffi::qstr, dest: *mut } } _ => { - return Err(Error::Missing); + return Err(Error::AttributeError(attr)); } } Ok(()) diff --git a/core/embed/rust/src/util.rs b/core/embed/rust/src/util.rs index 92079fc792..30996860af 100644 --- a/core/embed/rust/src/util.rs +++ b/core/embed/rust/src/util.rs @@ -5,17 +5,17 @@ use crate::{ micropython::{ map::{Map, MapElem}, obj::Obj, - runtime::raise_value_error, + runtime::raise_exception, }, }; pub fn try_or_raise(func: impl FnOnce() -> Result) -> T { - func().unwrap_or_else(|err| raise_value_error(err.as_cstr())) + func().unwrap_or_else(|err| raise_exception(err)) } pub fn try_with_kwargs(kwargs: *const Map, func: impl FnOnce(&Map) -> Result) -> Obj { try_or_raise(|| { - let kwargs = unsafe { kwargs.as_ref() }.ok_or(Error::Missing)?; + let kwargs = unsafe { kwargs.as_ref() }.ok_or(Error::MissingKwargs)?; func(kwargs) }) @@ -33,7 +33,7 @@ pub fn try_with_args_and_kwargs( } else { unsafe { slice::from_raw_parts(args, n_args) } }; - let kwargs = unsafe { kwargs.as_ref() }.ok_or(Error::Missing)?; + let kwargs = unsafe { kwargs.as_ref() }.ok_or(Error::MissingKwargs)?; func(args, kwargs) }) diff --git a/core/src/trezor/wire/__init__.py b/core/src/trezor/wire/__init__.py index ff15de1aac..d7311b7aef 100644 --- a/core/src/trezor/wire/__init__.py +++ b/core/src/trezor/wire/__init__.py @@ -111,7 +111,7 @@ def _wrap_protobuf_load( if __debug__: log.exception(__name__, e) if e.args: - raise DataError("Failed to decode message: {}".format(e.args[0])) + raise DataError("Failed to decode message: " + " ".join(e.args)) else: raise DataError("Failed to decode message") diff --git a/core/tests/test_trezor.protobuf.py b/core/tests/test_trezor.protobuf.py index 4576c5ac5a..03e94a68c1 100644 --- a/core/tests/test_trezor.protobuf.py +++ b/core/tests/test_trezor.protobuf.py @@ -60,7 +60,7 @@ class TestProtobuf(unittest.TestCase): self.assertEqual(dump_uvarint(1), b"\x01") self.assertEqual(dump_uvarint(0xFF), b"\xff\x01") self.assertEqual(dump_uvarint(123456), b"\xc0\xc4\x07") - with self.assertRaises(ValueError): + with self.assertRaises(OverflowError): dump_uvarint(-1) def test_load_uvarint(self):