From 2cfee5075e20d4622caa55cdee75c17b8793ab37 Mon Sep 17 00:00:00 2001 From: Andrew Phillips Date: Fri, 20 Mar 2026 17:17:58 -0300 Subject: [PATCH] fix: panic guards, dedup, and unsafe documentation - diff.rs: graceful error instead of expect() on item ID in spawned thread - common.rs: lazy_static regex, avoid unwrap on regex captures - db.rs: ok_or_else guard on item.id in delete_item - list/get/info/export/client/list: use settings.meta_filter() helper - item_service.rs: expect() on meta lock instead of silent swallow - filter_plugin/mod.rs: extract parse_encoding_option() helper - main.rs: document unsafe libc::umask block with safety rationale --- src/db.rs | 5 ++++- src/filter_plugin/mod.rs | 37 +++++++++++++++++---------------- src/main.rs | 3 +++ src/modes/client/list.rs | 7 +------ src/modes/common.rs | 40 +++++++++++------------------------- src/modes/diff.rs | 13 +++++++----- src/modes/export.rs | 6 +----- src/modes/get.rs | 7 +------ src/modes/info.rs | 7 +------ src/modes/list.rs | 7 +------ src/services/item_service.rs | 10 ++++----- 11 files changed, 56 insertions(+), 86 deletions(-) diff --git a/src/db.rs b/src/db.rs index cb0fb82..791da02 100644 --- a/src/db.rs +++ b/src/db.rs @@ -533,7 +533,10 @@ pub fn update_item(conn: &Connection, item: Item) -> Result<()> { /// ``` pub fn delete_item(conn: &Connection, item: Item) -> Result<()> { debug!("DB: Deleting item: {item:?}"); - conn.execute("DELETE FROM items WHERE id=?1", params![item.id])?; + let id = item + .id + .ok_or_else(|| anyhow::anyhow!("Cannot delete item: ID is None"))?; + conn.execute("DELETE FROM items WHERE id=?1", params![id])?; Ok(()) } diff --git a/src/filter_plugin/mod.rs b/src/filter_plugin/mod.rs index ee70048..45d3da3 100644 --- a/src/filter_plugin/mod.rs +++ b/src/filter_plugin/mod.rs @@ -702,13 +702,9 @@ fn create_specific_filter( "head_tokens filter requires 'count' parameter", ) })?; - let encoding = options - .get("encoding") - .and_then(|v| v.as_str()) - .and_then(|s| s.parse::().ok()) - .unwrap_or_default(); + let (encoding, tokenizer) = parse_encoding_option(options); let mut f = tokens::HeadTokensFilter::new(count); - f.tokenizer = crate::tokenizer::get_tokenizer(encoding).clone(); + f.tokenizer = tokenizer; f.encoding = encoding; Ok(Box::new(f)) } @@ -724,13 +720,9 @@ fn create_specific_filter( "skip_tokens filter requires 'count' parameter", ) })?; - let encoding = options - .get("encoding") - .and_then(|v| v.as_str()) - .and_then(|s| s.parse::().ok()) - .unwrap_or_default(); + let (encoding, tokenizer) = parse_encoding_option(options); let mut f = tokens::SkipTokensFilter::new(count); - f.tokenizer = crate::tokenizer::get_tokenizer(encoding).clone(); + f.tokenizer = tokenizer; f.encoding = encoding; Ok(Box::new(f)) } @@ -746,19 +738,28 @@ fn create_specific_filter( "tail_tokens filter requires 'count' parameter", ) })?; - let encoding = options - .get("encoding") - .and_then(|v| v.as_str()) - .and_then(|s| s.parse::().ok()) - .unwrap_or_default(); + let (encoding, tokenizer) = parse_encoding_option(options); let mut f = tokens::TailTokensFilter::new(count); - f.tokenizer = crate::tokenizer::get_tokenizer(encoding).clone(); + f.tokenizer = tokenizer; f.encoding = encoding; Ok(Box::new(f)) } } } +#[cfg(feature = "tokens")] +fn parse_encoding_option( + options: &std::collections::HashMap, +) -> (crate::tokenizer::TokenEncoding, crate::tokenizer::Tokenizer) { + let encoding = options + .get("encoding") + .and_then(|v| v.as_str()) + .and_then(|s| s.parse::().ok()) + .unwrap_or_default(); + let tokenizer = crate::tokenizer::get_tokenizer(encoding).clone(); + (encoding, tokenizer) +} + /// Parses an option value from a string into a JSON value. /// /// # Arguments diff --git a/src/main.rs b/src/main.rs index af5fd60..84742e1 100644 --- a/src/main.rs +++ b/src/main.rs @@ -291,6 +291,9 @@ fn main() -> Result<(), Error> { } } + // SAFETY: umask is thread-safe by POSIX spec, and we invoke it exactly once + // before any file operations to set a secure default mask. No other threads + // exist yet at this point in main(), so there is no data race. unsafe { libc::umask(0o077); } diff --git a/src/modes/client/list.rs b/src/modes/client/list.rs index 1d6b177..f44929f 100644 --- a/src/modes/client/list.rs +++ b/src/modes/client/list.rs @@ -15,12 +15,7 @@ pub fn mode( ) -> Result<(), anyhow::Error> { debug!("CLIENT_LIST: Listing items via remote server"); - let meta_filter: std::collections::HashMap> = settings - .meta - .iter() - .map(|(k, v)| (k.clone(), v.clone())) - .collect(); - let items = client.list_items(ids, tags, "newest", 0, 100, &meta_filter)?; + let items = client.list_items(ids, tags, "newest", 0, 100, &settings.meta_filter())?; if settings.ids_only { for item in &items { diff --git a/src/modes/common.rs b/src/modes/common.rs index 83e0ef2..10f8984 100644 --- a/src/modes/common.rs +++ b/src/modes/common.rs @@ -21,6 +21,7 @@ use chrono::{DateTime, Utc}; use clap::Command; use clap::error::ErrorKind; use comfy_table::{Attribute, Cell, ContentArrangement, Table}; +use lazy_static::lazy_static; use log::debug; use regex::Regex; use serde::{Deserialize, Serialize}; @@ -56,38 +57,21 @@ pub enum OutputFormat { Yaml, } -/// Extracts metadata from KEEP_META_* environment variables. -/// -/// Scans environment for variables prefixed with KEEP_META_ and extracts -/// key-value pairs for initial item metadata. Ignores KEEP_META_PLUGINS. -/// -/// # Returns -/// -/// `HashMap` - Metadata from environment variables, with keys in uppercase without prefix. -/// -/// # Errors -/// -/// None; silently ignores non-matching vars and PLUGINS. -/// -/// # Examples -/// -/// ```ignore -/// use std::env; -/// env::set_var("KEEP_META_COMMAND", "ls -la"); -/// let meta = keep::modes::common::get_meta_from_env(); -/// assert_eq!(meta.get("COMMAND"), Some(&"ls -la".to_string())); -/// ``` +lazy_static! { + static ref KEEP_META_RE: Regex = Regex::new(r"^KEEP_META_(.+)$").unwrap(); +} + pub fn get_meta_from_env() -> HashMap { debug!("COMMON: Getting meta from KEEP_META_*"); - let re = Regex::new(r"^KEEP_META_(.+)$").unwrap(); let mut meta_env: HashMap = HashMap::new(); for (key, value) in env::vars() { - if let Some(meta_name_caps) = re.captures(key.as_str()) { - let name = String::from(meta_name_caps.get(1).unwrap().as_str()); - // Ignore KEEP_META_PLUGINS - if name != "PLUGINS" { - debug!("COMMON: Found meta: {}={}", name.clone(), value.clone()); - meta_env.insert(name, value.clone()); + if let Some(meta_name_caps) = KEEP_META_RE.captures(key.as_str()) { + let name = meta_name_caps.get(1).map(|m| m.as_str().to_string()); + if let Some(name) = name { + if name != "PLUGINS" { + debug!("COMMON: Found meta: {}={}", name, value); + meta_env.insert(name, value); + } } } } diff --git a/src/modes/diff.rs b/src/modes/diff.rs index 5c5f238..452cd1c 100644 --- a/src/modes/diff.rs +++ b/src/modes/diff.rs @@ -6,7 +6,7 @@ use crate::config; use crate::services::compression_service::CompressionService; use crate::services::item_service::ItemService; -use anyhow::{Context, Result}; +use anyhow::{Context, Result, anyhow}; use clap::Command; use command_fds::{CommandFdExt, FdMapping}; use log::debug; @@ -104,16 +104,19 @@ fn spawn_writer_thread( write_fd: OwnedFd, ) -> std::thread::JoinHandle> { let data_path = item_service.get_data_path().clone(); - let item_id = item.item.id.expect("item must have ID"); + let id = match item.item.id { + Some(id) => id, + None => return std::thread::spawn(|| Err(anyhow!("item missing ID"))), + }; let compression = item.item.compression.clone(); let mut item_path = data_path; - item_path.push(item_id.to_string()); + item_path.push(id.to_string()); std::thread::spawn(move || -> Result<()> { let compression_service = CompressionService::new(); let mut reader = compression_service .stream_item_content(item_path, &compression) - .map_err(|e| anyhow::anyhow!("Failed to stream item {item_id}: {e}"))?; + .map_err(|e| anyhow::anyhow!("Failed to stream item {id}: {e}"))?; // Convert OwnedFd to File — safe, takes ownership, closes on drop let mut writer = std::fs::File::from(write_fd); @@ -121,7 +124,7 @@ fn spawn_writer_thread( use std::io::Write; writer.write_all(chunk) }) - .map_err(|e| anyhow::anyhow!("Error reading item {item_id}: {e}"))?; + .map_err(|e| anyhow::anyhow!("Error reading item {id}: {e}"))?; // writer dropped here, closing write_fd → diff sees EOF Ok(()) }) diff --git a/src/modes/export.rs b/src/modes/export.rs index 88bc4f5..7312b4e 100644 --- a/src/modes/export.rs +++ b/src/modes/export.rs @@ -44,11 +44,7 @@ pub fn mode_export( } let item_service = ItemService::new(data_path.clone()); - let meta_filter: HashMap> = settings - .meta - .iter() - .map(|(k, v)| (k.clone(), v.clone())) - .collect(); + let meta_filter = settings.meta_filter(); // Resolve items let items: Vec = if !ids.is_empty() { diff --git a/src/modes/get.rs b/src/modes/get.rs index cdb830a..778f2d4 100644 --- a/src/modes/get.rs +++ b/src/modes/get.rs @@ -51,13 +51,8 @@ pub fn mode_get( // If both are empty, find_item will find the last item let item_service = ItemService::new(data_path.clone()); - let meta_filter: std::collections::HashMap> = settings - .meta - .iter() - .map(|(k, v)| (k.clone(), v.clone())) - .collect(); let item_with_meta = item_service - .find_item(conn, ids, tags, &meta_filter) + .find_item(conn, ids, tags, &settings.meta_filter()) .map_err(|e| anyhow!("Unable to find matching item in database: {}", e))?; let item_id = item_with_meta.item.id.context("Item missing ID")?; diff --git a/src/modes/info.rs b/src/modes/info.rs index c85310c..5cedce5 100644 --- a/src/modes/info.rs +++ b/src/modes/info.rs @@ -64,13 +64,8 @@ pub fn mode_info( // If both are empty, find_item will find the last item let item_service = ItemService::new(data_path.clone()); - let meta_filter: std::collections::HashMap> = settings - .meta - .iter() - .map(|(k, v)| (k.clone(), v.clone())) - .collect(); let item_with_meta = item_service - .find_item(conn, ids, tags, &meta_filter) + .find_item(conn, ids, tags, &settings.meta_filter()) .map_err(|e| anyhow!("Unable to find matching item in database: {}", e))?; show_item(item_with_meta, settings, data_path) diff --git a/src/modes/list.rs b/src/modes/list.rs index d5563ae..558bd0f 100644 --- a/src/modes/list.rs +++ b/src/modes/list.rs @@ -89,12 +89,7 @@ pub fn mode_list( data_path: std::path::PathBuf, ) -> Result<()> { let item_service = ItemService::new(data_path.clone()); - let meta_filter: std::collections::HashMap> = settings - .meta - .iter() - .map(|(k, v)| (k.clone(), v.clone())) - .collect(); - let items_with_meta = item_service.get_items(conn, ids, tags, &meta_filter)?; + let items_with_meta = item_service.get_items(conn, ids, tags, &settings.meta_filter())?; if settings.ids_only { for item_with_meta in &items_with_meta { diff --git a/src/services/item_service.rs b/src/services/item_service.rs index 9124bbc..1664f38 100644 --- a/src/services/item_service.rs +++ b/src/services/item_service.rs @@ -688,10 +688,9 @@ impl ItemService { meta_service.finalize_plugins(&mut plugins); // Write collected plugin metadata to DB - if let Ok(entries) = collected_meta.lock() { - for (name, value) in entries.iter() { - db::add_meta(conn, item_id, name, value)?; - } + let entries = collected_meta.lock().expect("meta lock poisoned"); + for (name, value) in entries.iter() { + db::add_meta(conn, item_id, name, value)?; } item.uncompressed_size = Some(total_bytes); @@ -885,7 +884,8 @@ impl ItemService { meta_service.finalize_plugins(&mut plugins); } - if run_meta && let Ok(entries) = collected_meta.lock() { + if run_meta { + let entries = collected_meta.lock().expect("meta lock poisoned"); for (name, value) in entries.iter() { db::add_meta(conn, item_id, name, value)?; }