From bac1de50ffbd38c098167685c8b997fb691dab9c Mon Sep 17 00:00:00 2001 From: Linuxuser67 <41808005+Linuxuser67@users.noreply.github.com> Date: Tue, 27 Oct 2026 08:05:57 +0100 Subject: [PATCH] fix: address remaining review findings Follow-up to #548 for the audited review of main at 6de8538: - Reject non-hex characters in the extension's hex decoder: `from_str_radix` accepts a leading `+`, so "+f" decoded successfully before. Covered by a unit test. - Document both `unsafe` blocks in `real_home_dir` with SAFETY comments. - Route the gallery stderr-tail locks through `runtime::lock_recover` instead of unwrapping on poison. - Turn live clippy suppressions into `#[expect(..., reason = "...")]` and drop the four `too_many_arguments` allowances that no longer fire (their functions take at most 7 arguments). - Date the legacy paired-extension branch for removal after 2027-03-01. - Delete the empty `runtime` test module left by a removed test. --- src/extension_server.rs | 22 +++++++++++++++++++--- src/gallery_argv.rs | 10 ++++++++-- src/gallery_runner.rs | 5 +++-- src/inline_add.rs | 16 ++++++++++++---- src/inline_gallery.rs | 5 ++++- src/runtime.rs | 8 -------- src/video_argv.rs | 2 -- src/video_runner.rs | 21 ++++++++++++++++----- src/video_tools.rs | 3 +++ 9 files changed, 65 insertions(+), 27 deletions(-) diff --git a/src/extension_server.rs b/src/extension_server.rs index 40114a809..7595f24c9 100644 --- a/src/extension_server.rs +++ b/src/extension_server.rs @@ -220,7 +220,8 @@ fn hex_encode(bytes: &[u8]) -> String { } fn hex_decode(text: &str) -> Option> { - if !text.len().is_multiple_of(2) || !text.is_ascii() { + // Reject non-hex digits outright: `from_str_radix` accepts a leading `+`. + if !text.len().is_multiple_of(2) || !text.bytes().all(|b| b.is_ascii_hexdigit()) { return None; } (0..text.len()) @@ -1204,10 +1205,12 @@ async fn handle_add( if req.cookies.len() > 100 { return StatusCode::BAD_REQUEST.into_response(); } - // During a rolling upgrade, old paired extensions do not include the + // Legacy compatibility: old paired extensions do not include the // timestamp/nonce pair. Preserve their URL handoff, but downgrade them to // the same confirmation-only, no-cookie behavior as unpaired callers. // A partially present or malformed pair is never treated as legacy. + // Remove after 2027-03-01, once extension builds predating the nonce have + // aged out of auto-update. let legacy_authenticated = token_authenticated && req.timestamp_ms.is_none() && req.nonce.is_none(); if token_authenticated @@ -1346,7 +1349,10 @@ async fn handle_add( } #[cfg(test)] -#[allow(clippy::await_holding_lock)] +#[allow( + clippy::await_holding_lock, + reason = "tests hold the serial-test guard across awaits by design" +)] mod tests { use super::*; use tokio::io::{AsyncReadExt, AsyncWriteExt}; @@ -2328,6 +2334,16 @@ mod tests { assert!(!r.contains(&other)); } + #[test] + fn hex_decode_rejects_non_hex_digits() { + assert_eq!(hex_decode("0F"), Some(vec![0x0f])); + // `from_str_radix` accepts a leading `+`; the decoder must not. + assert_eq!(hex_decode("+f"), None); + assert_eq!(hex_decode("0f+0"), None); + assert_eq!(hex_decode("0g"), None); + assert_eq!(hex_decode("0"), None); + } + #[tokio::test] async fn hello_rejects_bad_nonce_unpaired_server_and_bad_host() { let srv = start(4).await; diff --git a/src/gallery_argv.rs b/src/gallery_argv.rs index 1689e5d4c..6c795b1ae 100644 --- a/src/gallery_argv.rs +++ b/src/gallery_argv.rs @@ -131,7 +131,10 @@ fn format_range(start: usize, end: usize) -> String { } #[cfg(test)] -#[allow(clippy::too_many_arguments)] +#[expect( + clippy::too_many_arguments, + reason = "test-only wrapper that forwards the full argv matrix to the proxy variant" +)] pub fn download_argv( tool: &Path, url: &str, @@ -155,7 +158,10 @@ pub fn download_argv( ) } -#[allow(clippy::too_many_arguments)] +#[expect( + clippy::too_many_arguments, + reason = "argv assembly matrix: every parameter maps to one flag; a struct adds indirection" +)] pub fn download_argv_with_proxy( tool: &Path, url: &str, diff --git a/src/gallery_runner.rs b/src/gallery_runner.rs index 25469dc23..f379a1c07 100644 --- a/src/gallery_runner.rs +++ b/src/gallery_runner.rs @@ -12,6 +12,7 @@ use tokio::process::Command; use crate::gallery_argv::{GalleryJob, download_argv_with_proxy}; use crate::gallery_progress::{GalleryProgress, ProgressLine, parse_progress_line}; use crate::gallery_tools::GalleryError; +use crate::runtime::lock_recover; use crate::video_spawn::ProcessGroupGuard; /// Outcome of a gallery download. @@ -204,7 +205,7 @@ pub async fn run_gallery_download_with_proxy( // Debug: echo stderr live. The line is still captured // below for the failure tail. crate::backend_debug::echo_line(&line); - let mut buf = stderr_lines_clone.lock().unwrap(); + let mut buf = lock_recover(&stderr_lines_clone); buf.push(line); if buf.len() > 20 { buf.remove(0); @@ -279,7 +280,7 @@ pub async fn run_gallery_download_with_proxy( if !status.success() { let code = status.code(); - let stderr_tail = stderr_lines.lock().unwrap().join("\n"); + let stderr_tail = lock_recover(&stderr_lines).join("\n"); // Log a redacted tail (gallery-dl failure lines embed media URLs with // signed query tokens; the journal gets no query strings). The // user-facing message keeps the full tail (the dialog scrolls). diff --git a/src/inline_add.rs b/src/inline_add.rs index 259044254..1cc37488b 100644 --- a/src/inline_add.rs +++ b/src/inline_add.rs @@ -486,7 +486,10 @@ pub(crate) struct VideoStep { /// manager, dest, etc. are in scope). The button invokes it if set. /// The nested Rc/RefCell/Option is the wiring cost of connecting the /// button (wired at VideoStep construction) to context available later. - #[allow(clippy::type_complexity)] + #[expect( + clippy::type_complexity, + reason = "nested Rc/RefCell/Option keeps the button wiring in one place" + )] pub(crate) gallery_select_opener: Rc>>>, /// Abort handle for the in-flight gallery file enumeration (picker). /// Aborted when the card closes, a new lookup starts, or the opener is @@ -817,7 +820,6 @@ fn selected_audio_only(step: &VideoStep) -> bool { .unwrap_or(false) } -#[allow(clippy::too_many_arguments)] fn submit_probed_single( manager: &Rc, dest: &Rc>, @@ -902,7 +904,10 @@ fn submit_probed_single( /// Dispatch a fresh preview to its submit path: singles keep their chosen /// format, while collections queue their resolved entries. The caller's /// freshness gate stays. -#[allow(clippy::too_many_arguments)] +#[expect( + clippy::too_many_arguments, + reason = "the preview and submit paths share this GUI wiring; a context struct only renames it" +)] fn submit_probe( manager: &Rc, dest: &Rc>, @@ -1449,7 +1454,10 @@ fn push_playlist_items_page( /// Multi-file .torrent intake normally queues every file without displaying /// the selection page; the page remains available if enqueueing fails. -#[allow(clippy::too_many_arguments)] +#[expect( + clippy::too_many_arguments, + reason = "single call site; bundling these into a struct would only move the arguments" +)] fn push_torrent_picker_page( nav: &adw::NavigationView, manager: Rc, diff --git a/src/inline_gallery.rs b/src/inline_gallery.rs index 1121473c1..5d2a491f7 100644 --- a/src/inline_gallery.rs +++ b/src/inline_gallery.rs @@ -243,7 +243,10 @@ impl GalleryPickerStream { /// The grid arrives empty and fills from a streamed enumeration (see /// `GalleryPickerStream`); `None` means another picker page already owns /// the screen, so the caller should drop its enumeration. -#[allow(clippy::too_many_arguments)] +#[expect( + clippy::too_many_arguments, + reason = "the picker page builder threads live GUI state from the enumeration caller" +)] pub(crate) fn push_gallery_picker_page( nav: &adw::NavigationView, manager: Rc, diff --git a/src/runtime.rs b/src/runtime.rs index e728eb7bb..d2c59da26 100644 --- a/src/runtime.rs +++ b/src/runtime.rs @@ -18,11 +18,3 @@ pub(crate) fn tokio_rt() -> &'static tokio::runtime::Runtime { pub(crate) fn lock_recover(m: &Mutex) -> std::sync::MutexGuard<'_, T> { m.lock().unwrap_or_else(|e| e.into_inner()) } - -#[cfg(test)] -mod tests { - // NOTE: `two_slow_tasks_do_not_block_third_on_two_workers` was removed: - // it tested tokio's scheduler (timing margins on a 2-worker runtime), - // not Grab code, and its rationale referenced `block_on_sync`, which no - // longer exists. -} diff --git a/src/video_argv.rs b/src/video_argv.rs index 276c6a69e..d9a7c48f5 100644 --- a/src/video_argv.rs +++ b/src/video_argv.rs @@ -194,7 +194,6 @@ pub(crate) fn playlist_scope_args(playlist_index: Option) -> Vec } /// yt-dlp argv for one unified direct download into a staging temp. Pure. -#[allow(clippy::too_many_arguments)] pub(crate) fn unified_download_argv( job: &VideoJob, spec: &str, @@ -474,7 +473,6 @@ pub(crate) fn live_from_start_unsupported(log: &str) -> bool { } /// yt-dlp argv for one VOD HLS capture (planner-pinned variant id, merge/extract, proxy/identity). Pure. -#[allow(clippy::too_many_arguments)] pub(crate) fn hls_download_argv( job: &VideoJob, hls_format_id: &str, diff --git a/src/video_runner.rs b/src/video_runner.rs index ba00214d0..d384fe0a1 100644 --- a/src/video_runner.rs +++ b/src/video_runner.rs @@ -508,7 +508,10 @@ async fn try_merge_dash_partials( Some(size) } -#[allow(clippy::too_many_arguments)] +#[expect( + clippy::too_many_arguments, + reason = "one attempt pipeline: splitting it would scatter cancellation and merge state" +)] pub(crate) async fn run_unified_ytdlp( youtube_bin: &Path, ffmpeg_bin: &Path, @@ -704,7 +707,6 @@ pub(crate) async fn await_child( } /// One yt-dlp spawn: parse template progress, collect the log tail, capture `--print after_move:filepath`. `Ok((None, _))` is a user abort (the caller stays quiet). `on_merge` fires once on the first merge line. `timeout` is a stall budget, not a wall clock: any stdout line resets it, so only silence kills the attempt. The stall deadline gives way to a generous merge wall-clock once the merge starts (see `await_child`). -#[allow(clippy::too_many_arguments)] async fn run_ytdlp_attempt( youtube_bin: &Path, argv: &[String], @@ -1555,7 +1557,10 @@ pub(crate) fn live_info_json_args(path: Option<&std::path::Path>) -> Vec } } -#[allow(clippy::too_many_arguments)] +#[expect( + clippy::too_many_arguments, + reason = "mirrors run_unified_ytdlp so both attempts keep the same cancellation contract" +)] pub(crate) async fn run_live_ytdlp( youtube_bin: &Path, ffmpeg_bin: &Path, @@ -1584,7 +1589,10 @@ pub(crate) async fn run_live_ytdlp( // recording — only the .part shell. // NOTE: the `&staging` borrow is required (&PathBuf -> &Path coercion); // clippy::needless_borrow false-positives on it inside let-chains (1.99). - #[allow(clippy::needless_borrow)] + #[expect( + clippy::needless_borrow, + reason = "clippy false positive inside a let-chain: `&staging` is `&PathBuf -> &Path`" + )] if let Some(old) = read_manifest(&staging, job.item_id) && let Some(old_name) = old.staging_name.as_deref() && old_name != staging_name @@ -2070,7 +2078,10 @@ pub(crate) async fn run_live_ytdlp( Ok(file_len(&job.dest)) } -#[allow(clippy::too_many_arguments)] +#[expect( + clippy::too_many_arguments, + reason = "mirrors the other attempt runners so the HLS path keeps the same contract" +)] pub(crate) async fn run_hls_ytdlp( youtube_bin: &Path, ffmpeg_bin: &Path, diff --git a/src/video_tools.rs b/src/video_tools.rs index e28ee1f22..a0f0ba99c 100644 --- a/src/video_tools.rs +++ b/src/video_tools.rs @@ -1490,6 +1490,7 @@ pub(crate) fn real_home_dir() -> Option { // NSS may need more than the POSIX minimum. Retry ERANGE but bound growth // so a broken NSS module cannot make this allocate indefinitely. const MAX_PASSWD_BUFFER: usize = 1024 * 1024; + // SAFETY: `sysconf` reads the process environment; no pointers involved. let suggested = unsafe { libc::sysconf(libc::_SC_GETPW_R_SIZE_MAX) }; let initial = if suggested > 0 { (suggested as usize).clamp(1024, MAX_PASSWD_BUFFER) @@ -1522,6 +1523,8 @@ pub(crate) fn real_home_dir() -> Option { if entry.pw_dir.is_null() { return None; } + // SAFETY: `getpwuid_r` succeeded, so `pw_dir` is a non-null C string + // inside `buffer`; the bytes are copied out before `buffer` changes. let bytes = unsafe { CStr::from_ptr(entry.pw_dir) }.to_bytes(); return (!bytes.is_empty()).then(|| PathBuf::from(std::ffi::OsStr::from_bytes(bytes))); }