context: fix deadlock and panics when resolving parented contexts

set_current held the config RwLock for writing across make_context,
which for a context with a parent re-entered the same lock through
Context::new's global-manager lookup, deadlocking 'pkh context use'.
Context building is now lock-free by construction: make_context
resolves parent chains against a snapshot map (also rejecting parent
cycles), and neither set_current nor remove_context holds a guard
while building a Context.

A corrupt contexts.json no longer aborts every command at manager
init: load falls back to the default local-only config, backs the
corrupt file up to contexts.json.bak so a later save cannot silently
destroy it, and a dangling current/parent context falls back to local
with an error log instead of panicking.
This commit is contained in:
2026-09-16 02:27:11 +02:00
parent 592e98c1e9
commit 6a5c5a7106
7 changed files with 360 additions and 68 deletions
+1 -1
View File
@@ -717,7 +717,7 @@ mod tests {
"Types: deb\nURIs: http://a\nSuites: noble\nComponents: main\n", "Types: deb\nURIs: http://a\nSuites: noble\nComponents: main\n",
) )
.unwrap(); .unwrap();
let ctx = Arc::new(Context::new(ContextConfig::Local)); let ctx = Arc::new(Context::new(ContextConfig::Local).unwrap());
let origin = SourceOrigin { let origin = SourceOrigin {
path: path.clone(), path: path.clone(),
+3 -3
View File
@@ -1336,9 +1336,9 @@ Provides: virtual-thing (= 2.0), plain-virtual
assert!(status.success(), "debian/rules {target} failed"); assert!(status.success(), "debian/rules {target} failed");
} }
let ctx = std::sync::Arc::new(crate::context::Context::new( let ctx = std::sync::Arc::new(
crate::context::ContextConfig::Local, crate::context::Context::new(crate::context::ContextConfig::Local).unwrap(),
)); );
let native_arch = crate::debian::arch::native().unwrap_or_else(|_| "amd64".into()); let native_arch = crate::debian::arch::native().unwrap_or_else(|_| "amd64".into());
let opts = crate::build::binary::BinaryMetadataOptions { let opts = crate::build::binary::BinaryMetadataOptions {
profiles, profiles,
+67 -15
View File
@@ -131,8 +131,46 @@ pub struct Context {
impl Context { impl Context {
/// Create a context from configuration /// 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> {
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<ContextConfig>,
) -> io::Result<Self> {
Self::with_lookup_inner(config, lookup, &mut Vec::new())
}
fn with_lookup_inner(
config: ContextConfig,
lookup: &dyn Fn(&str) -> Option<ContextConfig>,
chain: &mut Vec<String>,
) -> io::Result<Self> {
let parent_name = match &config {
ContextConfig::Schroot { ContextConfig::Schroot {
parent: Some(parent_name), parent: Some(parent_name),
.. ..
@@ -140,23 +178,37 @@ impl Context {
| ContextConfig::Unshare { | ContextConfig::Unshare {
parent: Some(parent_name), parent: Some(parent_name),
.. ..
} => { } => parent_name.clone(),
let config_lock = crate::context::manager::MANAGER.get_config(); _ => {
let parent_config = config_lock return Ok(Self {
.contexts config,
.get(parent_name) parent: None,
.cloned() driver: Mutex::new(None),
.expect("Parent context not found"); });
Some(Arc::new(Context::new(parent_config)))
} }
_ => None,
}; };
Self { if chain.iter().any(|name| name == &parent_name) {
config, return Err(io::Error::new(
parent, io::ErrorKind::InvalidData,
driver: Mutex::new(None), 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 /// Create a context with an explicit parent context
+142 -40
View File
@@ -3,7 +3,7 @@ use serde::{Deserialize, Serialize};
use std::collections::HashMap; use std::collections::HashMap;
use std::fs; use std::fs;
use std::io; use std::io;
use std::path::PathBuf; use std::path::{Path, PathBuf};
use std::sync::Arc; use std::sync::Arc;
use std::sync::RwLock; use std::sync::RwLock;
@@ -48,26 +48,88 @@ impl ContextManager {
fs::create_dir_all(config_dir)?; fs::create_dir_all(config_dir)?;
let config_path = config_dir.join("contexts.json"); let config_path = config_dir.join("contexts.json");
let config = if config_path.exists() { let mut config = Self::load_config(&config_path);
// Load existing configuration file
let content = fs::read_to_string(&config_path)?; // Build the initial Context against the freshly loaded map, before
serde_json::from_str(&content) // the manager itself exists: resolution must not go through the
.map_err(|e| io::Error::new(io::ErrorKind::InvalidData, e))? // global MANAGER here, since a parented current context would
} else { // re-enter its own LazyLock initialization.
// Create a new configuration file let initial = match Self::make_context(&config.context, &config.contexts) {
Config::default() 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 { Ok(Self {
context: RwLock::new(Arc::new(Self::make_context( context: RwLock::new(Arc::new(initial)),
config.context.as_str(),
&config,
))),
config_path, config_path,
config: RwLock::new(config), 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 `<path>.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 /// Obtain current ContextManager configuration
pub fn get_config(&self) -> std::sync::RwLockReadGuard<'_, Config> { pub fn get_config(&self) -> std::sync::RwLockReadGuard<'_, Config> {
self.config.read().unwrap() self.config.read().unwrap()
@@ -77,7 +139,12 @@ impl ContextManager {
pub fn with_path(path: PathBuf) -> Self { pub fn with_path(path: PathBuf) -> Self {
let config = Config::default(); let config = Config::default();
Self { 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_path: path,
config: RwLock::new(config), config: RwLock::new(config),
} }
@@ -92,13 +159,22 @@ impl ContextManager {
Ok(()) Ok(())
} }
fn make_context(name: &str, config: &Config) -> Context { /// Build a [`Context`] for `name` from `contexts`.
let context_config = config ///
.contexts /// Lock-free by construction: parent references are resolved against
.get(name) /// `contexts` itself (see [`Context::with_lookup`]), never against the
.cloned() /// manager's configuration lock. This is what keeps [`ContextManager::new`]
.expect("Context not found in config"); /// working before the global [`MANAGER`] exists, and what allows callers
Context::new(context_config) /// to build contexts without risking a re-entrant read on a lock they
/// already hold for writing.
fn make_context(name: &str, contexts: &HashMap<String, ContextConfig>) -> io::Result<Context> {
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 /// List contexts from configuration
@@ -124,41 +200,67 @@ impl ContextManager {
/// Remove context from configuration /// Remove context from configuration
pub fn remove_context(&self, name: &str) -> io::Result<()> { pub fn remove_context(&self, name: &str) -> io::Result<()> {
let mut config = self.config.write().unwrap();
if name == "local" { if name == "local" {
return Err(io::Error::new( return Err(io::Error::new(
io::ErrorKind::InvalidInput, io::ErrorKind::InvalidInput,
"Cannot remove local context", "Cannot remove local context",
)); ));
} }
if config.contexts.remove(name).is_some() { // Mutate under the write lock, snapshotting the remaining map when
// If we are removing the current context, fallback to local // the removed context was current; the fallback Context is built
if name == config.context { // after the lock is released (same discipline as `set_current`).
config.context = "local".to_string(); let fallback_contexts = {
self.set_current_ephemeral(Self::make_context("local", &config)); 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 if let Some(contexts) = fallback_contexts {
self.save()?; self.set_current_ephemeral(Self::make_context("local", &contexts)?);
} }
self.save()?;
Ok(()) Ok(())
} }
/// Set current context from name (modifying configuration) /// Set current context from name (modifying configuration)
pub fn set_current(&self, name: &str) -> io::Result<()> { pub fn set_current(&self, name: &str) -> io::Result<()> {
let mut config = self.config.write().unwrap(); // Snapshot what `make_context` needs and release the lock before
if config.contexts.contains_key(name) { // building the Context. Building resolves parent contexts, and this
config.context = name.to_string(); // code path used to hold the config write guard while re-entering
self.set_current_ephemeral(Self::make_context(name, &config)); // the same lock for a read — a guaranteed deadlock on a
drop(config); // Drop write lock before saving // std::sync::RwLock (same-thread write-then-read).
self.save()?; let contexts = self.config.read().unwrap().contexts.clone();
Ok(()) if !contexts.contains_key(name) {
} else { return Err(io::Error::new(
Err(io::Error::new(
io::ErrorKind::NotFound, 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 /// Set current context, without modifying configuration
+141 -4
View File
@@ -76,7 +76,7 @@ mod tests {
let src_file = temp_dir.path().join("src.txt"); let src_file = temp_dir.path().join("src.txt");
fs::write(&src_file, "local").unwrap(); 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(); let dest = ctx.ensure_available(&src_file, "/tmp").unwrap();
// Should return a path that exists and has the same content // 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())); 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::<super::manager::Config>(&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] #[test]
fn test_context_file_ops() { fn test_context_file_ops() {
let temp_dir = tempfile::tempdir().unwrap(); 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 file_path = temp_dir.path().join("test.txt");
let content = "hello world"; let content = "hello world";
@@ -200,7 +337,7 @@ mod tests {
fn test_context_copy_preserves_dangling_symlink() { fn test_context_copy_preserves_dangling_symlink() {
use std::os::unix::fs::symlink; use std::os::unix::fs::symlink;
let temp_dir = tempfile::tempdir().unwrap(); 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"); let src_dir = temp_dir.path().join("src");
std::fs::create_dir_all(&src_dir).unwrap(); 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::create_dir_all(src_root.join("src/.svn")).unwrap();
fs::write(src_root.join("src/hello.c"), "int main() {}").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(); let dest = ctx.ensure_available(&src_root, "/tmp").unwrap();
assert!(dest.join("src/hello.c").exists()); assert!(dest.join("src/hello.c").exists());
+4 -3
View File
@@ -14,9 +14,10 @@ pub struct SchrootDriver {
impl SchrootDriver { impl SchrootDriver {
fn parent(&self) -> Arc<Context> { fn parent(&self) -> Arc<Context> {
self.parent self.parent.clone().unwrap_or_else(|| {
.clone() // ContextConfig::Local has no parent, so this cannot fail.
.unwrap_or_else(|| Arc::new(Context::new(ContextConfig::Local))) Arc::new(Context::new(ContextConfig::Local).expect("Local context cannot fail"))
})
} }
fn ensure_session(&self) -> io::Result<String> { fn ensure_session(&self) -> io::Result<String> {
+2 -2
View File
@@ -397,7 +397,7 @@ mod tests {
log::info!("Successfully pulled package {}", package); log::info!("Successfully pulled package {}", package);
// Create a fresh local context for this test // 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 // Change directory to the package directory
let cwd = let cwd =
@@ -578,7 +578,7 @@ mod tests {
let temp_dir = tempfile::tempdir().unwrap(); let temp_dir = tempfile::tempdir().unwrap();
let pkg_dir = create_indep_cross_test_source(temp_dir.path()); 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( crate::deb::build_binary_package(
Some("arm64"), Some("arm64"),