refactor(core): safe iface for get_glyph_data

[no changelog]

core/embed/gfx/fonts/fonts.h

@@ -38,7 +38,8 @@ typedef struct {
   const uint8_t *glyph_nonprintable;
 } font_info_t;
 
-/// Font identifiers
+/// Font identifiers. Keep in sync with `enum font` definition in
+/// `core/embed/rust/src/ui/display/font.rs`.
 typedef enum {
   FONT_NORMAL = -1,
   FONT_BOLD = -2,

core/embed/rust/src/translations/blob.rs

@@ -240,7 +240,7 @@ impl<'a> Translations<'a> {
     /// translations object. This is to facilitate safe interface to
     /// flash-based translations. See docs for `flash::get` for details.
     #[allow(clippy::needless_lifetimes)]
-    pub fn font<'b>(&'b self, index: u16) -> Option<Table<'b>> {
+    fn font<'b>(&'b self, index: u16) -> Option<Table<'b>> {
         self.fonts
             .get(index)
             .and_then(|data| Table::new(InputStream::new(data)).ok())
@@ -258,6 +258,20 @@ impl<'a> Translations<'a> {
     pub fn header<'b>(&'b self) -> &'b TranslationsHeader<'b> {
         &self.header
     }
+
+    /// Returns a byte slice of the glyph data for the given UTF-8 codepoint in
+    /// the specified font.
+    ///
+    /// SAFETY: Do not mess with the lifetimes in this signature.
+    ///
+    /// The lifetimes are a useful lie that bind the lifetime of the returned
+    /// string not to the underlying data, but to the _reference_ to the
+    /// translations object. This is to facilitate safe interface to
+    /// flash-based translations. See docs for `flash::get` for details.
+    #[allow(clippy::needless_lifetimes)]
+    pub fn get_utf8_glyph<'b>(&'b self, codepoint: u16, font_index: u16) -> Option<&'b [u8]> {
+        self.font(font_index).and_then(|t| t.get(codepoint))
+    }
 }
 
 pub struct TranslationsHeader<'a> {

core/embed/rust/src/translations/mod.rs

@@ -1,33 +1,12 @@
 mod blob;
-mod flash;
+pub mod flash;
 mod generated;
 #[cfg(feature = "micropython")]
 mod obj;
 mod public_keys;
 mod translated_string;
 
+pub use blob::Translations;
 pub use translated_string::TranslatedString as TR;
 
-use crate::ui::display::Font;
 pub const DEFAULT_LANGUAGE: &str = "en-US";
-
-/// # Safety
-///
-/// Returned pointer will only point to valid font data for as long as
-/// the flash content is not invalidated by `erase()` or `write()`.
-pub unsafe fn get_utf8_glyph(codepoint: u16, font: Font) -> *const u8 {
-    // SAFETY: Reference is discarded at the end of the function.
-    // We do return a _pointer_ to the same memory location, but the pointer is
-    // always valid.
-    let Ok(translations) = flash::get() else {
-        return core::ptr::null();
-    };
-    let Some(tr) = translations.as_ref() else {
-        return core::ptr::null();
-    };
-    if let Some(glyph) = tr.font(font as u16).and_then(|t| t.get(codepoint)) {
-        glyph.as_ptr()
-    } else {
-        core::ptr::null()
-    }
-}

core/embed/rust/src/ui/display/font.rs

@@ -1,5 +1,7 @@
+use spin::RwLockReadGuard;
+
 use crate::{
-    trezorhal::display,
+    trezorhal::display::{self},
     ui::{
         constant,
         geometry::Offset,
@@ -9,12 +11,14 @@ use crate::{
 use core::slice;
 
 #[cfg(feature = "translations")]
-use crate::translations::get_utf8_glyph;
+use crate::translations::flash;
+#[cfg(feature = "translations")]
+use crate::translations::Translations;
 
 /// Representation of a single glyph.
 /// We use standard typographic terms. For a nice explanation, see, e.g.,
 /// the FreeType docs at https://www.freetype.org/freetype2/docs/glyphs/glyphs-3.html
-pub struct Glyph {
+pub struct Glyph<'a> {
     /// Total width of the glyph itself
     pub width: i16,
     /// Total height of the glyph itself
@@ -25,40 +29,32 @@ pub struct Glyph {
     pub bearing_x: i16,
     /// Top-side vertical bearing
     pub bearing_y: i16,
-    data: &'static [u8],
+    data: &'a [u8],
 }
 
-impl Glyph {
+impl<'a> Glyph<'a> {
-    /// Construct a `Glyph` from a raw pointer.
+    /// Creates a new `Glyph` from a byte slice containing font data.
     ///
-    /// # Safety
+    /// Expected data format (bytes):
-    ///
+    /// - 0: glyph width
-    /// This function is unsafe because the caller has to guarantee that `data`
+    /// - 1: glyph height
-    /// is pointing to a memory containing a valid glyph data, that is:
+    /// - 2: advance width
-    /// - contains valid glyph metadata
+    /// - 3: x-bearing
-    /// - data has appropriate size
+    /// - 4: y-bearing
-    /// - data must have static lifetime
+    /// - 5...: bitmap data, packed according to FONT_BPP (bits per pixel)
-    pub unsafe fn load(data: *const u8) -> Self {
+    pub fn load(data: &'a [u8]) -> Self {
-        unsafe {
+        let width = data[0] as i16;
-            let width = *data.offset(0) as i16;
+        let height = data[1] as i16;
-            let height = *data.offset(1) as i16;
 
-            let data_bytes = match constant::FONT_BPP {
+        let size = calculate_glyph_size(data);
-                1 => (width * height + 7) / 8, // packed bits
+        ensure!(data.len() == size, "Invalid glyph data size");
-                2 => (width * height + 3) / 4, // packed bits
+        Glyph {
-                4 => (width + 1) / 2 * height, // row aligned to bytes
+            width,
-                8 => width * height,
+            height,
-                _ => fatal_error!("Unsupported font bpp"),
+            adv: data[2] as i16,
-            };
+            bearing_x: data[3] as i16,
-
+            bearing_y: data[4] as i16,
-            Glyph {
+            data: &data[5..],
-                width,
-                height,
-                adv: *data.offset(2) as i16,
-                bearing_x: *data.offset(3) as i16,
-                bearing_y: *data.offset(4) as i16,
-                data: slice::from_raw_parts(data.offset(5), data_bytes as usize),
-            }
         }
     }
 
@@ -88,7 +84,7 @@ impl Glyph {
         c_data >> 4
     }
 
-    pub fn bitmap(&self) -> Bitmap<'static> {
+    pub fn bitmap(&self) -> Bitmap<'a> {
         match constant::FONT_BPP {
             1 => unwrap!(Bitmap::new(
                 BitmapFormat::MONO1P,
@@ -109,6 +105,114 @@ impl Glyph {
     }
 }
 
+/// A provider of font glyphs and their metadata.
+///
+/// Manages access to font resources and handles UTF-8 character glyphs
+///
+/// The provider holds necessary lock for accessing translation data
+/// and is typically used through the `Font::with_glyph_data` method
+/// to ensure proper resource cleanup.
+///
+/// # Example
+/// ```
+/// let font = Font::NORMAL;
+/// font.with_glyph_data(|data| {
+///     let glyph = data.get_glyph('A');
+///     // use glyph...
+/// });
+/// ```
+pub struct GlyphData {
+    font: Font,
+    #[cfg(feature = "translations")]
+    translations_guard: Option<RwLockReadGuard<'static, Option<Translations<'static>>>>,
+}
+
+impl GlyphData {
+    fn new(font: Font) -> Self {
+        #[cfg(feature = "translations")]
+        let translations_guard = flash::get().ok();
+
+        Self {
+            font,
+            #[cfg(feature = "translations")]
+            translations_guard,
+        }
+    }
+
+    pub fn get_glyph(&self, ch: char) -> Glyph<'_> {
+        let ch = match ch {
+            '\u{00a0}' => '\u{0020}',
+            c => c,
+        };
+        let gl_data = self.get_glyph_data(ch as u16);
+
+        Glyph::load(unwrap!(gl_data, "Failed to load glyph"))
+    }
+
+    fn get_glyph_data(&self, codepoint: u16) -> Option<&[u8]> {
+        display::get_font_info(self.font.into()).map(|font_info| {
+            if codepoint >= ' ' as u16 && codepoint < 0x7F {
+                // ASCII character
+                let offset = codepoint - ' ' as u16;
+                unsafe {
+                    let ptr = *font_info.glyph_data.offset(offset as isize);
+                    self.load_glyph_from_ptr(ptr)
+                }
+            } else {
+                #[cfg(feature = "translations")]
+                {
+                    if codepoint >= 0x7F {
+                        // UTF8 character from embedded blob
+                        if let Some(glyph) = self
+                            .translations_guard
+                            .as_ref()
+                            .and_then(|guard| guard.as_ref())
+                            .and_then(|translations| {
+                                translations.get_utf8_glyph(codepoint, self.font as u16)
+                            })
+                        {
+                            return glyph;
+                        }
+                    }
+                }
+                self.glyph_nonprintable()
+            }
+        })
+    }
+
+    /// Returns glyph data slice from a raw pointer by reading the header and
+    /// calculating full size.
+    unsafe fn load_glyph_from_ptr(&self, ptr: *const u8) -> &[u8] {
+        unsafe {
+            let header = slice::from_raw_parts(ptr, 2);
+            let full_size = calculate_glyph_size(header);
+            slice::from_raw_parts(ptr, full_size)
+        }
+    }
+
+    /// Returns glyph data slize for non-printable characters.
+    fn glyph_nonprintable(&self) -> &[u8] {
+        display::get_font_info(self.font.into())
+            .map(|font_info| unsafe { self.load_glyph_from_ptr(font_info.glyph_nonprintable) })
+            .unwrap()
+    }
+}
+
+fn calculate_glyph_size(header: &[u8]) -> usize {
+    let width = header[0] as i16;
+    let height = header[1] as i16;
+
+    let data_bytes = match constant::FONT_BPP {
+        1 => (width * height + 7) / 8, // packed bits
+        2 => (width * height + 3) / 4, // packed bits
+        4 => (width + 1) / 2 * height, // row aligned to bytes
+        8 => width * height,
+        _ => fatal_error!("Unsupported font bpp"),
+    };
+
+    5 + data_bytes as usize // header (5 bytes) + bitmap data
+}
+
 /// Font constants. Keep in sync with `font_id_t` definition in
 /// `core/embed/gfx/fonts/fonts.h`.
 #[derive(Copy, Clone, PartialEq, Eq, FromPrimitive)]
@@ -134,17 +238,12 @@ impl From<Font> for i32 {
 impl Font {
     /// Supports UTF8 characters
     pub fn text_width(self, text: &str) -> i16 {
-        text.chars().fold(0, |acc, c| acc + self.char_width(c))
+        self.with_glyph_data(|data| {
-    }
+            text.chars().fold(0, |acc, c| {
-
+                let char_width = data.get_glyph(c).adv;
-    /// Supports UTF8 characters
+                acc + char_width
-    fn get_first_glyph_from_text(self, text: &str) -> Option<Glyph> {
+            })
-        text.chars().next().map(|c| self.get_glyph(c))
+        })
-    }
-
-    /// Supports UTF8 characters
-    fn get_last_glyph_from_text(self, text: &str) -> Option<Glyph> {
-        text.chars().next_back().map(|c| self.get_glyph(c))
     }
 
     /// Width of the text that is visible.
@@ -155,17 +254,17 @@ impl Font {
             return 0;
         }
 
-        let first_char_bearing = if let Some(glyph) = self.get_first_glyph_from_text(text) {
+        let (first_char_bearing, last_char_bearing) = self.with_glyph_data(|data| {
-            glyph.bearing_x
+            let first = text
-        } else {
+                .chars()
-            0
+                .next()
-        };
+                .map_or(0, |c| data.get_glyph(c).bearing_x);
-
+            let last = text
-        let last_char_bearing = if let Some(glyph) = self.get_last_glyph_from_text(text) {
+                .chars()
-            glyph.right_side_bearing()
+                .next_back()
-        } else {
+                .map_or(0, |c| data.get_glyph(c).right_side_bearing());
-            0
+            (first, last)
-        };
+        });
 
         // Strip leftmost and rightmost spaces/bearings/margins.
         self.text_width(text) - first_char_bearing - last_char_bearing
@@ -178,11 +277,13 @@ impl Font {
     /// the glyphs representing the characters in the provided text.
     pub fn visible_text_height(self, text: &str) -> i16 {
         let (mut ascent, mut descent) = (0, 0);
-        for c in text.chars() {
+        self.with_glyph_data(|data| {
-            let glyph = self.get_glyph(c);
+            for c in text.chars() {
-            ascent = ascent.max(glyph.bearing_y);
+                let glyph = data.get_glyph(c);
-            descent = descent.max(glyph.height - glyph.bearing_y);
+                ascent = ascent.max(glyph.bearing_y);
-        }
+                descent = descent.max(glyph.height - glyph.bearing_y);
+            }
+        });
         ascent + descent
     }
 
@@ -202,26 +303,25 @@ impl Font {
             return 0;
         }
 
-        self.get_first_glyph_from_text(text)
+        text.chars().next().map_or(0, |c| {
-            .map_or(0, |glyph| glyph.bearing_x)
+            self.with_glyph_data(|data| data.get_glyph(c).bearing_x)
+        })
     }
 
     pub fn char_width(self, ch: char) -> i16 {
-        self.get_glyph(ch).adv
+        self.with_glyph_data(|data| data.get_glyph(ch).adv)
     }
 
     pub fn text_height(self) -> i16 {
-        display::get_font_info(self.into()).map_or(i16::MAX, |font| font.height.try_into().unwrap())
+        unwrap!(display::get_font_info(self.into())).height as i16
     }
 
     pub fn text_max_height(self) -> i16 {
-        display::get_font_info(self.into())
+        unwrap!(display::get_font_info(self.into())).max_height as i16
-            .map_or(i16::MAX, |font| font.max_height.try_into().unwrap())
     }
 
     pub fn text_baseline(self) -> i16 {
-        display::get_font_info(self.into())
+        unwrap!(display::get_font_info(self.into())).baseline as i16
-            .map_or(i16::MAX, |font| font.baseline.try_into().unwrap())
     }
 
     pub fn line_height(self) -> i16 {
@@ -247,41 +347,16 @@ impl Font {
         (start + end + self.visible_text_height(text)) / 2
     }
 
-    fn get_glyph_data(&self, codepoint: u16) -> *const u8 {
+    /// Safely manages temporary access to glyph data without risking
-        display::get_font_info((*self).into()).map_or(core::ptr::null(), |font_info| {
+    /// translation lock deadlocks. See `GlyphData` for more details.
-            #[cfg(feature = "translations")]
+    pub fn with_glyph_data<T, F>(&self, f: F) -> T
-            {
+    where
-                if codepoint >= 0x7F {
+        F: FnOnce(&GlyphData) -> T,
-                    // UTF8 character from embedded blob
+    {
-                    return unsafe { get_utf8_glyph(codepoint, *self) };
+        // Create a new GlyphData instance that will be dropped at the end of this
-                }
+        // function, releasing any translations lock
-            }
+        let glyph_data = GlyphData::new(*self);
-
+        f(&glyph_data)
-            if codepoint >= ' ' as u16 && codepoint < 0x7F {
-                // ASCII character
-                unsafe {
-                    *font_info
-                        .glyph_data
-                        .offset((codepoint - ' ' as u16) as isize)
-                }
-            } else {
-                font_info.glyph_nonprintable
-            }
-        })
-    }
-
-    pub fn get_glyph(self, ch: char) -> Glyph {
-        /* have the non-breaking space counted for width but not counted as a
-         * breaking point */
-        let ch = match ch {
-            '\u{00a0}' => '\u{0020}',
-            c => c,
-        };
-        let gl_data = self.get_glyph_data(ch as u16);
-
-        ensure!(!gl_data.is_null(), "Failed to load glyph");
-        // SAFETY: Glyph::load is valid for data returned by get_char_glyph
-        unsafe { Glyph::load(gl_data) }
     }
 
     /// Get the longest prefix of a given `text` (breaking at word boundaries)
@@ -289,45 +364,51 @@ impl Font {
     pub fn longest_prefix(self, width: i16, text: &str) -> &str {
         let mut prev_word_boundary = 0;
         let mut text_width = 0;
-        for (i, c) in text.char_indices() {
+        self.with_glyph_data(|data| {
-            let c_width = self.char_width(c);
+            for (i, c) in text.char_indices() {
-            if text_width + c_width > width {
+                let char_width = data.get_glyph(c).adv;
-                // Another character would not fit => split at the previous word boundary
+                let c_width = char_width;
-                return &text[0..prev_word_boundary];
+                if text_width + c_width > width {
+                    // Another character would not fit => split at the previous word boundary
+                    return &text[0..prev_word_boundary];
+                }
+                if c == ' ' {
+                    prev_word_boundary = i;
+                }
+                text_width += c_width;
             }
-            if c == ' ' {
+            text // the whole text fits
-                prev_word_boundary = i;
+        })
-            }
-            text_width += c_width;
-        }
-
-        text // the whole text fits
     }
 
     /// Get the length of the longest suffix from a given `text`
     /// that will fit into the area `width` pixels wide.
     pub fn longest_suffix(self, width: i16, text: &str) -> usize {
         let mut text_width = 0;
-        for (chars_from_right, c) in text.chars().rev().enumerate() {
-            let c_width = self.char_width(c);
-            if text_width + c_width > width {
-                // Another character cannot be fitted, we're done.
-                return chars_from_right;
-            }
-            text_width += c_width;
-        }
 
-        text.len() // it fits in its entirety
+        self.with_glyph_data(|data| {
+            for (chars_from_right, c) in text.chars().rev().enumerate() {
+                let char_width = data.get_glyph(c).adv;
+                if text_width + char_width > width {
+                    // Another character cannot be fitted, we're done.
+                    return chars_from_right;
+                }
+                text_width += char_width;
+            }
+            text.len() // it fits in its entirety
+        })
     }
 
     pub fn visible_text_height_ex(&self, text: &str) -> (i16, i16) {
         let (mut ascent, mut descent) = (0, 0);
-        for c in text.chars() {
+        self.with_glyph_data(|data| {
-            let glyph = self.get_glyph(c);
+            for c in text.chars() {
-            ascent = ascent.max(glyph.bearing_y);
+                let glyph = data.get_glyph(c);
-            descent = descent.max(glyph.height - glyph.bearing_y);
+                ascent = ascent.max(glyph.bearing_y);
-        }
+                descent = descent.max(glyph.height - glyph.bearing_y);
-        (ascent, descent)
+            }
+            (ascent, descent)
+        })
     }
 }
 

core/embed/rust/src/ui/shape/text.rs

@@ -97,24 +97,26 @@ impl<'a> Shape<'_> for Text<'a> {
 
         // TODO: optimize  text clipping, use canvas.viewport()
 
-        for ch in self.text.chars() {
+        self.font.with_glyph_data(|glyph_data| {
-            if r.x0 >= r.x1 {
+            for ch in self.text.chars() {
-                break;
+                if r.x0 >= r.x1 {
+                    break;
+                }
+
+                let glyph = glyph_data.get_glyph(ch);
+                let glyph_bitmap = glyph.bitmap();
+                let glyph_view = BitmapView::new(&glyph_bitmap)
+                    .with_alpha(self.alpha)
+                    .with_fg(self.color)
+                    .with_offset(Offset::new(
+                        -glyph.bearing_x,
+                        -(max_ascent - glyph.bearing_y),
+                    ));
+
+                canvas.blend_bitmap(r, glyph_view);
+                r.x0 += glyph.adv;
             }
-
+        });
-            let glyph = self.font.get_glyph(ch);
-            let glyph_bitmap = glyph.bitmap();
-            let glyph_view = BitmapView::new(&glyph_bitmap)
-                .with_alpha(self.alpha)
-                .with_fg(self.color)
-                .with_offset(Offset::new(
-                    -glyph.bearing_x,
-                    -(max_ascent - glyph.bearing_y),
-                ));
-
-            canvas.blend_bitmap(r, glyph_view);
-            r.x0 += glyph.adv;
-        }
     }
 }