From 39d9cbb624e4541f6a6a458709cfd92a27d73075 Mon Sep 17 00:00:00 2001 From: nevaforget Date: Fri, 24 Apr 2026 12:34:00 +0200 Subject: [PATCH] =?UTF-8?q?fix:=20audit=20fixes=20=E2=80=94=20RefCell=20ac?= =?UTF-8?q?ross=20await,=20async=20avatar=20decode=20(v0.6.10)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - init_fingerprint_async: hoist username before the await so a concurrent connect_monitor signal (hotplug / suspend-resume) cannot cause a RefCell panic. Re-borrow after the await for signal wiring. - set_avatar_from_file: decode via gio::File::read_future + Pixbuf::from_stream_at_scale_future so the GTK main thread stays responsive during monitor hotplug. Default icon shown while loading. --- Cargo.lock | 2 +- Cargo.toml | 2 +- DECISIONS.md | 7 +++++++ src/lockscreen.rs | 47 +++++++++++++++++++++++++++++------------------ src/main.rs | 40 +++++++++++++++++++++++----------------- 5 files changed, 61 insertions(+), 37 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index e5e7757..0e9d0c8 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -575,7 +575,7 @@ dependencies = [ [[package]] name = "moonlock" -version = "0.6.8" +version = "0.6.9" dependencies = [ "gdk-pixbuf", "gdk4", diff --git a/Cargo.toml b/Cargo.toml index 0acd58a..ef29185 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "moonlock" -version = "0.6.9" +version = "0.6.10" edition = "2024" description = "A secure Wayland lockscreen with GTK4, PAM and fingerprint support" license = "MIT" diff --git a/DECISIONS.md b/DECISIONS.md index 8fa1695..0d1d0c6 100644 --- a/DECISIONS.md +++ b/DECISIONS.md @@ -2,6 +2,13 @@ Architectural and design decisions for Moonlock, in reverse chronological order. +## 2026-04-24 – Audit fixes: RefCell borrow across await, async avatar decode + +- **Who**: ClaudeCode, Dom +- **Why**: Triple audit found two HIGH issues. (1) `init_fingerprint_async` held a `RefCell` immutable borrow across `is_available_async().await` — a concurrent `connect_monitor` signal (hotplug / suspend-resume) invoking `borrow_mut()` during the await would panic. (2) `set_avatar_from_file` decoded avatars synchronously via `Pixbuf::from_file_at_scale`, blocking the GTK main thread inside the `connect_monitor` handler. With `MAX_AVATAR_FILE_SIZE` at 10 MB the worst-case stall was 200–500 ms on monitor hotplug. +- **Tradeoffs**: Avatar is shown as the symbolic default icon for a brief window while decoding completes. Wallpaper stays synchronous because `connect_monitor` fires during `lock()` and needs the texture already present (see 2026-04-09). +- **How**: (1) Extract `username` into a local `String` in `init_fingerprint_async`, drop the borrow before the await, re-borrow in a new scope after — no awaits inside the second borrow, so hotplug during signal setup is safe. (2) `set_avatar_from_file` now uses `gio::File::read_future` + `Pixbuf::from_stream_at_scale_future` for async I/O and decode. The default icon is shown immediately; the decoded texture replaces it when ready. `Pixbuf` itself is `!Send`, so `gio::spawn_blocking` does not apply — the GIO async stream loader keeps the `Pixbuf` on the main thread while the kernel does the I/O asynchronously. + ## 2026-04-09 – Monitor hotplug via connect_monitor signal - **Who**: ClaudeCode, Dom diff --git a/src/lockscreen.rs b/src/lockscreen.rs index 40c16ee..e000075 100644 --- a/src/lockscreen.rs +++ b/src/lockscreen.rs @@ -623,30 +623,41 @@ fn render_blurred_texture( } /// Load an image file and set it as the avatar. Stores the texture in the cache. +/// Decoding runs via GIO async I/O + async pixbuf stream loader so the GTK main +/// loop stays responsive — avatars may be loaded inside the `connect_monitor` +/// signal handler at hotplug time, which must not block. The fallback icon is +/// shown immediately; the decoded texture replaces it when ready. fn set_avatar_from_file( image: >k::Image, path: &Path, cache: &Rc>>, ) { - let path_str = match path.to_str() { - Some(s) => s, - None => { - log::warn!("Avatar path is not valid UTF-8: {:?}", path); - image.set_icon_name(Some("avatar-default-symbolic")); - return; + image.set_icon_name(Some("avatar-default-symbolic")); + + let display_path = path.to_path_buf(); + let file = gio::File::for_path(path); + let image_clone = image.clone(); + let cache_clone = cache.clone(); + + glib::spawn_future_local(async move { + let stream = match file.read_future(glib::Priority::default()).await { + Ok(s) => s, + Err(e) => { + log::warn!("Failed to open avatar {}: {e}", display_path.display()); + return; + } + }; + match Pixbuf::from_stream_at_scale_future(&stream, AVATAR_SIZE, AVATAR_SIZE, true).await { + Ok(pixbuf) => { + let texture = gdk::Texture::for_pixbuf(&pixbuf); + image_clone.set_paintable(Some(&texture)); + *cache_clone.borrow_mut() = Some(texture); + } + Err(e) => { + log::warn!("Failed to decode avatar from {}: {e}", display_path.display()); + } } - }; - match Pixbuf::from_file_at_scale(path_str, AVATAR_SIZE, AVATAR_SIZE, true) { - Ok(pixbuf) => { - let texture = gdk::Texture::for_pixbuf(&pixbuf); - image.set_paintable(Some(&texture)); - *cache.borrow_mut() = Some(texture); - } - Err(e) => { - log::warn!("Failed to load avatar from {:?}: {e}", path); - image.set_icon_name(Some("avatar-default-symbolic")); - } - } + }); } /// Load the default avatar SVG from GResources, tinted with the foreground color. diff --git a/src/main.rs b/src/main.rs index adea4c2..87df818 100644 --- a/src/main.rs +++ b/src/main.rs @@ -168,32 +168,38 @@ fn init_fingerprint_async( let mut listener = FingerprintListener::new(); listener.init_async().await; - let handles = all_handles.borrow(); - if handles.is_empty() { - return; - } + // Extract username without holding a borrow across the await below — + // otherwise a concurrent connect_monitor signal (hotplug / suspend-resume) + // that tries to borrow_mut() panics at runtime. + let username = { + let handles = all_handles.borrow(); + if handles.is_empty() { + return; + } + let u = handles[0].username.clone(); + if u.is_empty() { + return; + } + u + }; - // Use the first monitor's username to check enrollment - let username = &handles[0].username; - if username.is_empty() { - return; - } - - if !listener.is_available_async(username).await { + if !listener.is_available_async(&username).await { log::debug!("fprintd not available or no enrolled fingers"); return; } let fp_rc = Rc::new(RefCell::new(listener)); - // Show fingerprint label on all existing monitors - for h in handles.iter() { - lockscreen::show_fingerprint_label(h, &fp_rc); + // Re-borrow after the await — no further awaits in this scope, so it is + // safe to hold the borrow briefly while wiring up the labels. + { + let handles = all_handles.borrow(); + for h in handles.iter() { + lockscreen::show_fingerprint_label(h, &fp_rc); + } + lockscreen::start_fingerprint(&handles[0], &fp_rc); } - // Start verification listener on the first monitor only - lockscreen::start_fingerprint(&handles[0], &fp_rc); - // Publish the listener so hotplugged monitors get FP labels too *shared_fp.borrow_mut() = Some(fp_rc); });