diff --git a/src/apt/sources.rs b/src/apt/sources.rs index cea888d..82c376e 100644 --- a/src/apt/sources.rs +++ b/src/apt/sources.rs @@ -717,7 +717,7 @@ mod tests { "Types: deb\nURIs: http://a\nSuites: noble\nComponents: main\n", ) .unwrap(); - let ctx = Arc::new(Context::new(ContextConfig::Local)); + let ctx = Arc::new(Context::new(ContextConfig::Local).unwrap()); let origin = SourceOrigin { path: path.clone(), diff --git a/src/build/mod.rs b/src/build/mod.rs index bbac02a..48da509 100644 --- a/src/build/mod.rs +++ b/src/build/mod.rs @@ -1336,9 +1336,9 @@ Provides: virtual-thing (= 2.0), plain-virtual assert!(status.success(), "debian/rules {target} failed"); } - let ctx = std::sync::Arc::new(crate::context::Context::new( - crate::context::ContextConfig::Local, - )); + let ctx = std::sync::Arc::new( + crate::context::Context::new(crate::context::ContextConfig::Local).unwrap(), + ); let native_arch = crate::debian::arch::native().unwrap_or_else(|_| "amd64".into()); let opts = crate::build::binary::BinaryMetadataOptions { profiles, diff --git a/src/context/api.rs b/src/context/api.rs index 493bd8f..8a90ec9 100644 --- a/src/context/api.rs +++ b/src/context/api.rs @@ -131,8 +131,46 @@ pub struct Context { impl Context { /// Create a context from configuration - pub fn new(config: ContextConfig) -> Self { - let parent = match &config { + /// + /// Parent contexts named in the configuration are resolved through the + /// global context manager; a dangling parent name is reported as an + /// error instead of panicking. + /// + /// Note that this takes a read lock on the global manager's + /// configuration: never call it while holding that lock for writing. + /// [`crate::context::ContextManager`] itself goes through + /// [`Context::with_lookup`] instead, which takes no locks. + pub fn new(config: ContextConfig) -> io::Result { + Self::with_lookup(config, &|name| { + crate::context::manager::MANAGER + .get_config() + .contexts + .get(name) + .cloned() + }) + } + + /// Create a context from configuration, resolving `parent` context names + /// through `lookup` instead of the global context manager. + /// + /// `lookup` must be lock-free: this is what allows + /// [`crate::context::ContextManager`] to build contexts while holding + /// (or before the very existence of) its configuration lock. Returns an + /// error when a referenced parent does not exist or when the parent + /// chain contains a cycle. + pub(crate) fn with_lookup( + config: ContextConfig, + lookup: &dyn Fn(&str) -> Option, + ) -> io::Result { + Self::with_lookup_inner(config, lookup, &mut Vec::new()) + } + + fn with_lookup_inner( + config: ContextConfig, + lookup: &dyn Fn(&str) -> Option, + chain: &mut Vec, + ) -> io::Result { + let parent_name = match &config { ContextConfig::Schroot { parent: Some(parent_name), .. @@ -140,23 +178,37 @@ impl Context { | ContextConfig::Unshare { parent: Some(parent_name), .. - } => { - let config_lock = crate::context::manager::MANAGER.get_config(); - let parent_config = config_lock - .contexts - .get(parent_name) - .cloned() - .expect("Parent context not found"); - Some(Arc::new(Context::new(parent_config))) + } => parent_name.clone(), + _ => { + return Ok(Self { + config, + parent: None, + driver: Mutex::new(None), + }); } - _ => None, }; - Self { - config, - parent, - driver: Mutex::new(None), + if chain.iter().any(|name| name == &parent_name) { + return Err(io::Error::new( + io::ErrorKind::InvalidData, + format!("Parent context cycle: '{parent_name}' appears in its own parent chain"), + )); } + let parent_config = lookup(&parent_name).ok_or_else(|| { + io::Error::new( + io::ErrorKind::NotFound, + format!("Parent context '{parent_name}' not found"), + ) + })?; + chain.push(parent_name); + let parent = Self::with_lookup_inner(parent_config, lookup, chain); + chain.pop(); + + Ok(Self { + config, + parent: Some(Arc::new(parent?)), + driver: Mutex::new(None), + }) } /// Create a context with an explicit parent context diff --git a/src/context/manager.rs b/src/context/manager.rs index a8df511..13b20fa 100644 --- a/src/context/manager.rs +++ b/src/context/manager.rs @@ -3,7 +3,7 @@ use serde::{Deserialize, Serialize}; use std::collections::HashMap; use std::fs; use std::io; -use std::path::PathBuf; +use std::path::{Path, PathBuf}; use std::sync::Arc; use std::sync::RwLock; @@ -48,26 +48,88 @@ impl ContextManager { fs::create_dir_all(config_dir)?; let config_path = config_dir.join("contexts.json"); - let config = if config_path.exists() { - // Load existing configuration file - let content = fs::read_to_string(&config_path)?; - serde_json::from_str(&content) - .map_err(|e| io::Error::new(io::ErrorKind::InvalidData, e))? - } else { - // Create a new configuration file - Config::default() + let mut config = Self::load_config(&config_path); + + // Build the initial Context against the freshly loaded map, before + // the manager itself exists: resolution must not go through the + // global MANAGER here, since a parented current context would + // re-enter its own LazyLock initialization. + let initial = match Self::make_context(&config.context, &config.contexts) { + Ok(context) => context, + Err(e) => { + log::error!( + "Cannot build current context '{}' from {}: {e}; falling back to 'local'", + config.context, + config_path.display() + ); + config.context = "local".to_string(); + Self::make_context("local", &config.contexts).unwrap_or_else(|e| { + // Only possible in a hand-edited configuration without + // any 'local' entry; a plain Local context has no parent + // and cannot fail to build. + log::error!( + "'local' context missing from {}: {e}", + config_path.display() + ); + Context::new(ContextConfig::Local).expect("Local context cannot fail") + }) + } }; Ok(Self { - context: RwLock::new(Arc::new(Self::make_context( - config.context.as_str(), - &config, - ))), + context: RwLock::new(Arc::new(initial)), config_path, config: RwLock::new(config), }) } + /// Load the configuration stored at `path`. + /// + /// A missing file yields [`Config::default`]. A file that cannot be read + /// or parsed must not take the whole program down: this falls back to + /// the default (local-only) configuration and logs an error. Because a + /// later [`ContextManager::save`] would otherwise silently overwrite the + /// corrupt file and destroy its content, the corrupt file is first + /// backed up to `.bak` (best effort). + pub(crate) fn load_config(path: &Path) -> Config { + if !path.exists() { + return Config::default(); + } + let loaded = fs::read_to_string(path).and_then(|content| { + serde_json::from_str(&content) + .map_err(|e| io::Error::new(io::ErrorKind::InvalidData, e)) + }); + match loaded { + Ok(config) => config, + Err(e) => { + log::error!( + "Context configuration {} is corrupt ({e}); using the default (local-only) configuration", + path.display() + ); + Self::backup_corrupt_file(path); + Config::default() + } + } + } + + /// Back up a corrupt configuration file (best effort) so a later save + /// cannot silently destroy its content. + fn backup_corrupt_file(path: &Path) { + let mut os = path.as_os_str().to_os_string(); + os.push(".bak"); + let backup_path = PathBuf::from(os); + match fs::copy(path, &backup_path) { + Ok(_) => log::warn!( + "Corrupt context configuration backed up to {}", + backup_path.display() + ), + Err(e) => log::warn!( + "Could not back up corrupt context configuration to {}: {e}", + backup_path.display() + ), + } + } + /// Obtain current ContextManager configuration pub fn get_config(&self) -> std::sync::RwLockReadGuard<'_, Config> { self.config.read().unwrap() @@ -77,7 +139,12 @@ impl ContextManager { pub fn with_path(path: PathBuf) -> Self { let config = Config::default(); Self { - context: RwLock::new(Arc::new(Self::make_context("local", &config))), + // 'local' is always present in Config::default and has no + // parent, so this cannot fail. + context: RwLock::new(Arc::new( + Self::make_context("local", &config.contexts) + .expect("default 'local' context cannot fail"), + )), config_path: path, config: RwLock::new(config), } @@ -92,13 +159,22 @@ impl ContextManager { Ok(()) } - fn make_context(name: &str, config: &Config) -> Context { - let context_config = config - .contexts - .get(name) - .cloned() - .expect("Context not found in config"); - Context::new(context_config) + /// Build a [`Context`] for `name` from `contexts`. + /// + /// Lock-free by construction: parent references are resolved against + /// `contexts` itself (see [`Context::with_lookup`]), never against the + /// manager's configuration lock. This is what keeps [`ContextManager::new`] + /// working before the global [`MANAGER`] exists, and what allows callers + /// to build contexts without risking a re-entrant read on a lock they + /// already hold for writing. + fn make_context(name: &str, contexts: &HashMap) -> io::Result { + let context_config = contexts.get(name).cloned().ok_or_else(|| { + io::Error::new( + io::ErrorKind::NotFound, + format!("Context '{name}' not found in configuration"), + ) + })?; + Context::with_lookup(context_config, &|parent| contexts.get(parent).cloned()) } /// List contexts from configuration @@ -124,41 +200,67 @@ impl ContextManager { /// Remove context from configuration pub fn remove_context(&self, name: &str) -> io::Result<()> { - let mut config = self.config.write().unwrap(); if name == "local" { return Err(io::Error::new( io::ErrorKind::InvalidInput, "Cannot remove local context", )); } - if config.contexts.remove(name).is_some() { - // If we are removing the current context, fallback to local - if name == config.context { - config.context = "local".to_string(); - self.set_current_ephemeral(Self::make_context("local", &config)); + // Mutate under the write lock, snapshotting the remaining map when + // the removed context was current; the fallback Context is built + // after the lock is released (same discipline as `set_current`). + let fallback_contexts = { + let mut config = self.config.write().unwrap(); + if config.contexts.remove(name).is_none() { + return Ok(()); } + if name == config.context { + // If we are removing the current context, fallback to local + config.context = "local".to_string(); + Some(config.contexts.clone()) + } else { + None + } + }; - drop(config); // Drop write lock before saving - self.save()?; + if let Some(contexts) = fallback_contexts { + self.set_current_ephemeral(Self::make_context("local", &contexts)?); } + self.save()?; Ok(()) } /// Set current context from name (modifying configuration) pub fn set_current(&self, name: &str) -> io::Result<()> { - let mut config = self.config.write().unwrap(); - if config.contexts.contains_key(name) { - config.context = name.to_string(); - self.set_current_ephemeral(Self::make_context(name, &config)); - drop(config); // Drop write lock before saving - self.save()?; - Ok(()) - } else { - Err(io::Error::new( + // Snapshot what `make_context` needs and release the lock before + // building the Context. Building resolves parent contexts, and this + // code path used to hold the config write guard while re-entering + // the same lock for a read — a guaranteed deadlock on a + // std::sync::RwLock (same-thread write-then-read). + let contexts = self.config.read().unwrap().contexts.clone(); + if !contexts.contains_key(name) { + return Err(io::Error::new( io::ErrorKind::NotFound, - format!("Context '{}' not found", name), - )) + format!("Context '{name}' not found"), + )); } + let context = Self::make_context(name, &contexts)?; + + // Re-take the write lock briefly to commit. The name may have been + // removed between snapshot and commit; report the same NotFound + // error instead of persisting a dangling current-context reference. + let mut config = self.config.write().unwrap(); + if !config.contexts.contains_key(name) { + return Err(io::Error::new( + io::ErrorKind::NotFound, + format!("Context '{name}' not found"), + )); + } + config.context = name.to_string(); + drop(config); // Drop write lock before saving + self.set_current_ephemeral(context); + self.save()?; + Ok(()) } /// Set current context, without modifying configuration diff --git a/src/context/mod.rs b/src/context/mod.rs index c7b0c6d..4ad8982 100644 --- a/src/context/mod.rs +++ b/src/context/mod.rs @@ -76,7 +76,7 @@ mod tests { let src_file = temp_dir.path().join("src.txt"); fs::write(&src_file, "local").unwrap(); - let ctx = Context::new(ContextConfig::Local); + let ctx = Context::new(ContextConfig::Local).unwrap(); let dest = ctx.ensure_available(&src_file, "/tmp").unwrap(); // Should return a path that exists and has the same content @@ -155,10 +155,147 @@ mod tests { assert!(mgr.list_contexts().contains(&"local".to_string())); } + /// `set_current` on a context whose configuration carries a `parent` + /// must complete without deadlocking: building the Context resolves the + /// parent chain, which used to re-enter the config lock while + /// `set_current` still held it for writing (a guaranteed deadlock on a + /// std::sync::RwLock, same-thread write-then-read). + #[test] + fn test_set_current_parented_context_no_deadlock() { + let temp_file = NamedTempFile::new().unwrap(); + let mgr = Arc::new(ContextManager::with_path(temp_file.path().to_path_buf())); + + mgr.add_context("base", ContextConfig::Local).unwrap(); + mgr.add_context( + "child", + ContextConfig::Schroot { + name: "testchroot".to_string(), + parent: Some("base".to_string()), + }, + ) + .unwrap(); + + // Run with a timeout so a regression fails fast instead of hanging + // the test binary forever. + let (tx, rx) = std::sync::mpsc::channel(); + let worker = { + let mgr = mgr.clone(); + std::thread::spawn(move || { + let result = mgr.set_current("child"); + tx.send(()).expect("receiver still waiting"); + result + }) + }; + match rx.recv_timeout(std::time::Duration::from_secs(30)) { + Ok(()) => {} + Err(std::sync::mpsc::RecvTimeoutError::Timeout) => { + panic!("set_current() deadlocked building a parented context"); + } + Err(std::sync::mpsc::RecvTimeoutError::Disconnected) => { + panic!("set_current() thread panicked before completing"); + } + } + worker.join().unwrap().unwrap(); + assert_eq!(mgr.current_name(), "child"); + } + + /// A context referencing a missing parent must produce an error, not a + /// panic (the parent lookup used to `.expect()`). + #[test] + fn test_set_current_dangling_parent_errors() { + let temp_file = NamedTempFile::new().unwrap(); + let mgr = ContextManager::with_path(temp_file.path().to_path_buf()); + mgr.add_context( + "orphan", + ContextConfig::Unshare { + path: "/some/chroot".to_string(), + parent: Some("missing".to_string()), + }, + ) + .unwrap(); + + let err = mgr.set_current("orphan").unwrap_err(); + assert_eq!(err.kind(), std::io::ErrorKind::NotFound); + // Nothing was committed: the current context is unchanged. + assert_eq!(mgr.current_name(), "local"); + } + + /// A parent cycle in a hand-edited configuration must be rejected with + /// an error instead of recursing until the stack overflows. + #[test] + fn test_set_current_parent_cycle_errors() { + let temp_file = NamedTempFile::new().unwrap(); + let mgr = ContextManager::with_path(temp_file.path().to_path_buf()); + mgr.add_context( + "a", + ContextConfig::Schroot { + name: "schroot-a".to_string(), + parent: Some("b".to_string()), + }, + ) + .unwrap(); + mgr.add_context( + "b", + ContextConfig::Unshare { + path: "/chroot-b".to_string(), + parent: Some("a".to_string()), + }, + ) + .unwrap(); + + let err = mgr.set_current("a").unwrap_err(); + assert_eq!(err.kind(), std::io::ErrorKind::InvalidData); + } + + /// A corrupt contexts.json must not take the manager down: + /// `load_config` falls back to the default (local-only) configuration, + /// keeps the corrupt file in place and backs it up to contexts.json.bak + /// so a later save cannot silently destroy its content. + #[test] + fn test_load_config_corrupt_file_falls_back_and_backs_up() { + let temp_dir = tempfile::tempdir().unwrap(); + let config_path = temp_dir.path().join("contexts.json"); + let garbage = "{ this is definitely not valid json"; + fs::write(&config_path, garbage).unwrap(); + + let config = ContextManager::load_config(&config_path); + + // Falls back to the default (local-only) configuration... + assert_eq!(config.context, "local"); + assert!(config.contexts.contains_key("local")); + + // ...preserving the corrupt file via the backup, original untouched. + let backup_path = temp_dir.path().join("contexts.json.bak"); + assert_eq!(fs::read_to_string(&backup_path).unwrap(), garbage); + assert_eq!(fs::read_to_string(&config_path).unwrap(), garbage); + + // A subsequent save replaces only the original, never the backup. + let mgr = ContextManager::with_path(config_path.clone()); + mgr.add_context("newctx", ContextConfig::Local).unwrap(); + let rewritten = fs::read_to_string(&config_path).unwrap(); + serde_json::from_str::(&rewritten).unwrap(); + assert_eq!(fs::read_to_string(&backup_path).unwrap(), garbage); + } + + /// A missing contexts.json yields the default configuration and writes + /// nothing (no file, no backup) until an explicit save. + #[test] + fn test_load_config_missing_file_defaults() { + let temp_dir = tempfile::tempdir().unwrap(); + let config_path = temp_dir.path().join("contexts.json"); + + let config = ContextManager::load_config(&config_path); + + assert_eq!(config.context, "local"); + assert!(config.contexts.contains_key("local")); + assert!(!config_path.exists()); + assert!(!temp_dir.path().join("contexts.json.bak").exists()); + } + #[test] fn test_context_file_ops() { let temp_dir = tempfile::tempdir().unwrap(); - let ctx = Context::new(ContextConfig::Local); + let ctx = Context::new(ContextConfig::Local).unwrap(); let file_path = temp_dir.path().join("test.txt"); let content = "hello world"; @@ -200,7 +337,7 @@ mod tests { fn test_context_copy_preserves_dangling_symlink() { use std::os::unix::fs::symlink; let temp_dir = tempfile::tempdir().unwrap(); - let ctx = Context::new(ContextConfig::Local); + let ctx = Context::new(ContextConfig::Local).unwrap(); let src_dir = temp_dir.path().join("src"); std::fs::create_dir_all(&src_dir).unwrap(); @@ -236,7 +373,7 @@ mod tests { fs::create_dir_all(src_root.join("src/.svn")).unwrap(); fs::write(src_root.join("src/hello.c"), "int main() {}").unwrap(); - let ctx = Context::new(ContextConfig::Local); + let ctx = Context::new(ContextConfig::Local).unwrap(); let dest = ctx.ensure_available(&src_root, "/tmp").unwrap(); assert!(dest.join("src/hello.c").exists()); diff --git a/src/context/schroot.rs b/src/context/schroot.rs index b7316bd..e12d6ef 100644 --- a/src/context/schroot.rs +++ b/src/context/schroot.rs @@ -14,9 +14,10 @@ pub struct SchrootDriver { impl SchrootDriver { fn parent(&self) -> Arc { - self.parent - .clone() - .unwrap_or_else(|| Arc::new(Context::new(ContextConfig::Local))) + self.parent.clone().unwrap_or_else(|| { + // ContextConfig::Local has no parent, so this cannot fail. + Arc::new(Context::new(ContextConfig::Local).expect("Local context cannot fail")) + }) } fn ensure_session(&self) -> io::Result { diff --git a/src/deb/mod.rs b/src/deb/mod.rs index 2ff25ac..0dfc160 100644 --- a/src/deb/mod.rs +++ b/src/deb/mod.rs @@ -397,7 +397,7 @@ mod tests { log::info!("Successfully pulled package {}", package); // Create a fresh local context for this test - let ctx = Arc::new(Context::new(crate::context::ContextConfig::Local)); + let ctx = Arc::new(Context::new(crate::context::ContextConfig::Local).unwrap()); // Change directory to the package directory let cwd = @@ -578,7 +578,7 @@ mod tests { let temp_dir = tempfile::tempdir().unwrap(); let pkg_dir = create_indep_cross_test_source(temp_dir.path()); - let ctx = Arc::new(Context::new(crate::context::ContextConfig::Local)); + let ctx = Arc::new(Context::new(crate::context::ContextConfig::Local).unwrap()); crate::deb::build_binary_package( Some("arm64"),