refactor(core/rust): fix Layout::return_value() after Error is no longer Clone
What changed, and why it matters
This commit is a small internal code cleanup in the Trezor firmware's Rust UI layer. It changes how a layout object hands back its result to caller code: instead of returning a copy of the result, it now 'takes' the result away so the same value cannot be retrieved twice. The change was needed because the error type no longer supports being copied (Clone). There is no direct evidence in the commit that this fixes an exploitable security bug; it appears to be a refactoring to keep the code compiling and to make the API behavior clearer (subsequent calls return None).
Treat as a routine refactoring commit. Reviewers should verify that no Micropython/Python callers rely on calling `return_value()` multiple times and getting the same result, since behavior now changes to returning None on subsequent calls. Confirm that the `take_value` change does not introduce use-after-move or double-error-handling issues in downstream code. No urgent security action is indicated by the commit itself.
Security signals we found
Change from copying/cloning return value to taking it (single-use semantics)
Use of `root_mut()` instead of `root()` in `obj_return_value`
Docstring now warns that `return_value()` may raise if constructing the return value errors
No explicit security bug, CVE, or vulnerability description in commit message or diff
Evidence from the diff
The patch refactors the Layout trait from a generic Layout<T> to an associated type Layout { type Value; } and replaces value(&self) -> Option<&T> with take_value(&mut self) -> Option<Self::Value>. Implementations in SwipeFlow and RootComponent now use Option::take() to move the stored returned_value out. The Python/Micropython binding obj_return_value now calls root_mut() and take_value() instead of cloning. Docstrings are updated to note that return_value() is not idempotent and may raise on error construction. A minor fix in ui_layout_paint removes an unnecessary ? after an already-unwrapped boolean. The stated reason is that Error is no longer Clone.
Changed components
core/embed/rust/src/ui/api/firmware_micropython.rscore/embed/rust/src/ui/flow/swipe.rscore/embed/rust/src/ui/layout/base.rscore/embed/rust/src/ui/layout/obj.rscore/mocks/generated/trezorui_api.pyiInspect captured patch +34 / −21
### core/embed/rust/src/ui/api/firmware_micropython.rs
@@ -1438,8 +1438,13 @@ pub static mp_module_trezorui_api: Module = obj_module! {
/// def get_transition_out(self) -> AttachType:
/// """Return the transition type."""
///
- /// def return_value(self) -> T:
- /// """Retrieve the return value of the layout object."""
+ /// def return_value(self) -> T | None:
+ /// """Take the return value of the layout object.
+ ///
+ /// Not idempotent: after `return_value()` is called, subsequent calls will return None.
+ ///
+ /// May raise in case there was an error when constructing the return value.
+ /// """
///
/// # TODO: remove after https://github.com/trezor/trezor-firmware/issues/6811 is resolved.
/// def __del__(self) -> None:
### core/embed/rust/src/ui/flow/swipe.rs
@@ -282,7 +282,9 @@ impl SwipeFlow {
/// This way we can completely avoid implementing `Component`. That also allows
/// us to pass around concrete Renderers instead of having to conform to
/// `Component`'s not-object-safe interface.
-impl Layout<Result<Obj, Error>> for SwipeFlow {
+impl Layout for SwipeFlow {
+ type Value = Result<Obj, Error>;
+
fn place(&mut self) {
for elem in self.store.iter_mut() {
elem.place(ModelUI::SCREEN);
@@ -293,8 +295,8 @@ impl Layout<Result<Obj, Error>> for SwipeFlow {
self.event(ctx, event)
}
- fn value(&self) -> Option<&Result<Obj, Error>> {
- self.returned_value.as_ref()
+ fn take_value(&mut self) -> Option<Self::Value> {
+ self.returned_value.take()
}
fn paint(&mut self) -> Result<(), PaintOutOfBounds> {
### core/embed/rust/src/ui/layout/base.rs
@@ -12,10 +12,12 @@ pub enum LayoutState {
pub struct PaintOutOfBounds;
-pub trait Layout<T> {
+pub trait Layout {
+ type Value;
+
fn place(&mut self);
fn event(&mut self, ctx: &mut EventCtx, event: Event) -> Option<LayoutState>;
- fn value(&self) -> Option<&T>;
+ fn take_value(&mut self) -> Option<Self::Value>;
fn paint(&mut self) -> Result<(), PaintOutOfBounds>;
}
### core/embed/rust/src/ui/layout/obj.rs
@@ -115,10 +115,12 @@ where
}
}
-impl<T> Layout<Result<Obj, Error>> for RootComponent<T, ModelUI>
+impl<T> Layout for RootComponent<T, ModelUI>
where
T: Component + ComponentMsgObj,
{
+ type Value = Result<Obj, Error>;
+
fn place(&mut self) {
self.inner.place(ModelUI::SCREEN);
}
@@ -134,8 +136,8 @@ where
}
}
- fn value(&self) -> Option<&Result<Obj, Error>> {
- self.returned_value.as_ref()
+ fn take_value(&mut self) -> Option<Self::Value> {
+ self.returned_value.take()
}
fn paint(&mut self) -> Result<(), PaintOutOfBounds> {
@@ -168,8 +170,8 @@ where
}
}
-pub trait LayoutMaybeTrace: Layout<Result<Obj, Error>> + MaybeTrace {}
-impl<T> LayoutMaybeTrace for T where T: Layout<Result<Obj, Error>> + MaybeTrace {}
+pub trait LayoutMaybeTrace: Layout<Value = Result<Obj, Error>> + MaybeTrace {}
+impl<T> LayoutMaybeTrace for T where T: Layout<Value = Result<Obj, Error>> + MaybeTrace {}
#[derive(Copy, Clone, PartialEq, Eq)]
#[cfg_attr(feature = "debug", derive(ufmt::derive::uDebug))]
@@ -364,10 +366,9 @@ impl LayoutObjInner {
self.transition_out.to_obj()
}
- fn obj_return_value(&self) -> Result<Obj, Error> {
- self.root()?
- .value()
- .cloned()
+ fn obj_return_value(&mut self) -> Result<Obj, Error> {
+ self.root_mut()?
+ .take_value()
.unwrap_or(Ok(Obj::const_none()))
}
}
@@ -629,11 +630,11 @@ extern "C" fn ui_layout_timer(this: Obj, token: Obj) -> Obj {
extern "C" fn ui_layout_paint(this: Obj) -> Obj {
let block = || {
let this: Gc<LayoutObj> = this.try_into()?;
- let painted = this.inner_mut().obj_paint_if_requested();
- if painted? {
+ let painted = this.inner_mut().obj_paint_if_requested()?;
+ if painted {
display::refresh();
}
- Ok(painted?.into())
+ Ok(painted.into())
};
unsafe { util::try_or_raise(block) }
}
### core/mocks/generated/trezorui_api.pyi
@@ -76,8 +76,11 @@ class LayoutObj(Generic[T]):
"""Return (code, type) of button request made during the last event or timer pass."""
def get_transition_out(self) -> AttachType:
"""Return the transition type."""
- def return_value(self) -> T:
- """Retrieve the return value of the layout object."""
+ def return_value(self) -> T | None:
+ """Take the return value of the layout object.
+ Not idempotent: after `return_value()` is called, subsequent calls will return None.
+ May raise in case there was an error when constructing the return value.
+ """
# TODO: remove after https://github.com/trezor/trezor-firmware/issues/6811 is resolved.
def __del__(self) -> None:
"""Calls drop on contents of the root component."""Why this scored 26/100
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.