diff --git a/Cargo.lock b/Cargo.lock index c1d543b7c..f7adce58f 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3845,6 +3845,7 @@ dependencies = [ name = "tor-persist" version = "0.4.1" dependencies = [ + "derive_more", "fs-mistrust", "fslock", "sanitize-filename", diff --git a/crates/tor-persist/Cargo.toml b/crates/tor-persist/Cargo.toml index 0e8ecf9e3..637239ddb 100644 --- a/crates/tor-persist/Cargo.toml +++ b/crates/tor-persist/Cargo.toml @@ -17,6 +17,7 @@ repository = "https://gitlab.torproject.org/tpo/core/arti.git/" testing = [] [dependencies] +derive_more = "0.99" fs-mistrust = { path = "../fs-mistrust", version = "0.3.0", features = ["walkdir"] } sanitize-filename = "0.4.0" serde = { version = "1.0.103", features = ["derive"] } diff --git a/crates/tor-persist/semver.md b/crates/tor-persist/semver.md new file mode 100644 index 000000000..d490e295f --- /dev/null +++ b/crates/tor-persist/semver.md @@ -0,0 +1 @@ +BREAKING: Error type completely changed. diff --git a/crates/tor-persist/src/err.rs b/crates/tor-persist/src/err.rs new file mode 100644 index 000000000..9ad0d5842 --- /dev/null +++ b/crates/tor-persist/src/err.rs @@ -0,0 +1,146 @@ +//! Error types for `tor-persist. + +use std::sync::Arc; + +use tor_error::ErrorKind; + +/// A resource that we failed to access or where we found a problem. +#[derive(Debug, Clone, derive_more::Display)] +pub(crate) enum Resource { + /// The manager as a whole. + #[display(fmt = "persistent storage manager")] + Manager, + /// A checked directory. + #[display(fmt = "directory {}", "dir.display()")] + Directory { + /// The path to the directory. + dir: std::path::PathBuf, + }, + /// A file on disk within our checked directory. + #[display(fmt = "{} in {}", "file.display()", "container.display()")] + File { + /// The path to the checked directory + container: std::path::PathBuf, + /// The path within the checked directory to the file. + file: std::path::PathBuf, + }, + /// Testing-only: a scratch-item in a memory-backed store. + #[display(fmt = "{} in memory-backed store", key)] + Temporary { + /// The key for the scratch item + key: String, + }, +} + +/// An action that we were trying to perform when an error occurred. +#[derive(Debug, Clone, derive_more::Display, Eq, PartialEq)] +pub(crate) enum Action { + /// We were trying to load an element from the store. + #[display(fmt = "loading persistent data")] + Loading, + /// We were trying to save an element into the store. + #[display(fmt = "storing persistent data")] + Storing, + /// We were trying to acquire the lock for the store. + #[display(fmt = "acquiring lock")] + Locking, + /// We were trying to release the lock for the store. + #[display(fmt = "releasing lock")] + Unlocking, + /// We were trying to validate the storage and initialize the manager. + #[display(fmt = "constructing storage manager")] + Initializing, +} + +/// An underlying error manipulating persistent state. +/// +/// Since these are more or less orthogonal to what we were doing and where the +/// problem was, this is a separate type. +#[derive(thiserror::Error, Debug, Clone)] +#[non_exhaustive] +pub enum ErrorSource { + /// An IO error occurred. + #[error("IO error")] + IoError(#[source] Arc), + + /// Permissions on a file or path were incorrect + #[error("Invalid permissions")] + Permissions(#[from] fs_mistrust::Error), + + /// Tried to save without holding an exclusive lock. + // + // TODO This error seems to actually be sometimes used to make store a no-op. + // We should consider whether this is best handled as an error, but for now + // this seems adequate. + #[error("Storage not locked")] + NoLock, + + /// Problem when serializing or deserializing JSON data. + #[error("JSON error")] + Serde(#[from] Arc), +} + +/// An error that occurred while manipulating persistent state. +#[derive(Clone, Debug, derive_more::Display)] +#[display(fmt = "{} while {} on {}", source, action, resource)] +pub struct Error { + /// The underlying error failure. + source: ErrorSource, + /// The action we were trying to perform + action: Action, + /// The resource we were trying to perform it on. + resource: Resource, +} + +impl std::error::Error for Error { + fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { + self.source.source() + } +} + +impl Error { + /// Return the underlying error source. + pub fn source(&self) -> &ErrorSource { + &self.source + } + + /// Construct a new Error from its components. + pub(crate) fn new(err: impl Into, action: Action, resource: Resource) -> Self { + Error { + source: err.into(), + action, + resource, + } + } +} + +impl tor_error::HasKind for Error { + #[rustfmt::skip] // the tabular layout of the `match` makes this a lot clearer + fn kind(&self) -> ErrorKind { + use ErrorSource as E; + use tor_error::ErrorKind as K; + match &self.source { + E::IoError(..) => K::PersistentStateAccessFailed, + E::Permissions(e) => if e.is_bad_permission() { + K::FsPermissions + } else { + K::PersistentStateAccessFailed + } + E::NoLock => K::BadApiUsage, + E::Serde(..) if self.action == Action::Storing => K::Internal, + E::Serde(..) => K::PersistentStateCorrupted, + } + } +} + +impl From for ErrorSource { + fn from(e: std::io::Error) -> ErrorSource { + ErrorSource::IoError(Arc::new(e)) + } +} + +impl From for ErrorSource { + fn from(e: serde_json::Error) -> ErrorSource { + ErrorSource::Serde(Arc::new(e)) + } +} diff --git a/crates/tor-persist/src/fs.rs b/crates/tor-persist/src/fs.rs index c3b725e80..cbd23e0be 100644 --- a/crates/tor-persist/src/fs.rs +++ b/crates/tor-persist/src/fs.rs @@ -2,7 +2,7 @@ mod clean; -use crate::{load_error, store_error}; +use crate::err::{Action, ErrorSource, Resource}; use crate::{Error, LockStatus, Result, StateMgr}; use fs_mistrust::CheckedDir; use serde::{de::DeserializeOwned, Serialize}; @@ -63,15 +63,37 @@ impl FsStateMgr { mistrust: &fs_mistrust::Mistrust, ) -> Result { let path = path.as_ref(); - let statepath = path.join("state"); + let dir = path.join("state"); let statepath = mistrust .verifier() .check_content() - .make_secure_dir(statepath)?; - let lockpath = statepath.join("state.lock")?; + .make_secure_dir(&dir) + .map_err(|e| { + Error::new( + e, + Action::Initializing, + Resource::Directory { dir: dir.clone() }, + ) + })?; + let lockpath = statepath.join("state.lock").map_err(|e| { + Error::new( + e, + Action::Initializing, + Resource::Directory { dir: dir.clone() }, + ) + })?; - let lockfile = Mutex::new(fslock::LockFile::open(&lockpath)?); + let lockfile = Mutex::new(fslock::LockFile::open(&lockpath).map_err(|e| { + Error::new( + e, + Action::Initializing, + Resource::File { + container: dir, + file: "state.lock".into(), + }, + ) + })?); Ok(FsStateMgr { inner: Arc::new(FsStateMgrInner { @@ -121,6 +143,22 @@ impl FsStateMgr { } } } + + /// Return a `Resource` object representing the file with a given key. + fn err_resource(&self, key: &str) -> Resource { + Resource::File { + container: self.path().to_path_buf(), + file: self.rel_filename(key), + } + } + + /// Return a `Resource` object representing our lock file. + fn err_resource_lock(&self) -> Resource { + Resource::File { + container: self.path().to_path_buf(), + file: "state.lock".into(), + } + } } impl StateMgr for FsStateMgr { @@ -140,7 +178,10 @@ impl StateMgr for FsStateMgr { .expect("Poisoned lock on state lockfile"); if lockfile.owns_lock() { Ok(LockStatus::AlreadyHeld) - } else if lockfile.try_lock()? { + } else if lockfile + .try_lock() + .map_err(|e| Error::new(e, Action::Locking, self.err_resource_lock()))? + { self.clean(SystemTime::now()); Ok(LockStatus::NewlyAcquired) } else { @@ -154,7 +195,9 @@ impl StateMgr for FsStateMgr { .lock() .expect("Poisoned lock on state lockfile"); if lockfile.owns_lock() { - lockfile.unlock()?; + lockfile + .unlock() + .map_err(|e| Error::new(e, Action::Unlocking, self.err_resource_lock()))?; } Ok(()) } @@ -167,11 +210,19 @@ impl StateMgr for FsStateMgr { let string = match self.inner.statepath.read_to_string(rel_fname) { Ok(string) => string, Err(fs_mistrust::Error::NotFound(_)) => return Ok(None), - Err(fs_mistrust::Error::Io { err, .. }) => return Err(Error::IoError(err)), - Err(e) => return Err(e.into()), + Err(fs_mistrust::Error::Io { err, .. }) => { + return Err(Error::new( + ErrorSource::IoError(err), + Action::Loading, + self.err_resource(key), + )) + } + Err(e) => return Err(Error::new(e, Action::Loading, self.err_resource(key))), }; - Ok(Some(serde_json::from_str(&string).map_err(load_error)?)) + Ok(Some(serde_json::from_str(&string).map_err(|source| { + Error::new(source, Action::Loading, self.err_resource(key)) + })?)) } fn store(&self, key: &str, val: &S) -> Result<()> @@ -179,14 +230,22 @@ impl StateMgr for FsStateMgr { S: Serialize, { if !self.can_store() { - return Err(Error::NoLock); + return Err(Error::new( + ErrorSource::NoLock, + Action::Storing, + Resource::Manager, + )); } let rel_fname = self.rel_filename(key); - let output = serde_json::to_string_pretty(val).map_err(store_error)?; + let output = serde_json::to_string_pretty(val) + .map_err(|e| Error::new(e, Action::Storing, self.err_resource(key)))?; - self.inner.statepath.write_and_replace(rel_fname, output)?; + self.inner + .statepath + .write_and_replace(rel_fname, output) + .map_err(|e| Error::new(e, Action::Storing, self.err_resource(key)))?; Ok(()) } @@ -226,7 +285,10 @@ mod test { .into_iter() .collect(); - assert!(matches!(store.store("xyz", &stuff4), Err(Error::NoLock))); + assert!(matches!( + store.store("xyz", &stuff4).unwrap_err().source(), + ErrorSource::NoLock + )); assert_eq!(store.try_lock()?, LockStatus::NewlyAcquired); store.store("xyz", &stuff4)?; @@ -284,7 +346,7 @@ mod test { std::fs::write(&fname2, "{}").unwrap(); // Make the store directory read-only and make sure that we can't delete from it. - std::fs::set_permissions(&statedir, ro_dir)?; + std::fs::set_permissions(&statedir, ro_dir).unwrap(); store.clean(SystemTime::now() + Duration::from_secs(365 * 86400)); let lst: Vec<_> = statedir.read_dir().unwrap().collect(); if lst.len() == 2 { @@ -293,12 +355,12 @@ mod test { } assert_eq!(lst.len(), 3); // We can't remove the file, but we didn't freak out. Great! // Try failing to read a mode-0 file. - std::fs::set_permissions(&statedir, rw_dir)?; - std::fs::set_permissions(&fname2, unusable)?; + std::fs::set_permissions(&statedir, rw_dir).unwrap(); + std::fs::set_permissions(&fname2, unusable).unwrap(); let h: Result>> = store.load("quoll"); assert!(h.is_err()); - assert!(matches!(h, Err(Error::IoError(_)))); + assert!(matches!(h.unwrap_err().source(), ErrorSource::IoError(_))); Ok(()) } diff --git a/crates/tor-persist/src/lib.rs b/crates/tor-persist/src/lib.rs index 7c84f63a3..7649e044a 100644 --- a/crates/tor-persist/src/lib.rs +++ b/crates/tor-persist/src/lib.rs @@ -44,6 +44,7 @@ #![allow(clippy::significant_drop_in_scrutinee)] // arti/-/merge_requests/588/#note_2812945 //! +mod err; #[cfg(not(target_arch = "wasm32"))] mod fs; mod handle; @@ -56,6 +57,7 @@ use std::sync::Arc; /// Wrapper type for Results returned from this crate. type Result = std::result::Result; +pub use err::Error; #[cfg(not(target_arch = "wasm32"))] pub use fs::FsStateMgr; pub use handle::{DynStorageHandle, StorageHandle}; @@ -63,8 +65,6 @@ pub use serde_json::Value as JsonValue; #[cfg(feature = "testing")] pub use testing::TestingStateMgr; -use tor_error::ErrorKind; - /// An object that can manage persistent state. /// /// State is implemented as a simple key-value store, where the values @@ -137,73 +137,6 @@ impl LockStatus { } } -/// An error manipulating persistent state. -// -// Such errors are "global" in the sense that it doesn't relate to any guard or any circuit -// or anything, so callers may use `#[from]` when they include it in their own error. -#[derive(thiserror::Error, Debug, Clone)] -#[non_exhaustive] -pub enum Error { - /// An IO error occurred. - #[error("IO error")] - IoError(#[source] Arc), - - /// Permissions on a file or path were incorrect - #[error("Invalid permissions on state file")] - Permissions(#[from] fs_mistrust::Error), - - /// Tried to save without holding an exclusive lock. - // - // TODO This error seems to actually be sometimes used to make store a no-op. - // We should consider whether this is best handled as an error, but for now - // this seems adequate. - #[error("Storage not locked")] - NoLock, - - /// Problem when serializing JSON data. - #[error("JSON serialization error")] - Serialize(#[source] Arc), - - /// Problem when deserializing JSON data. - #[error("JSON serialization error")] - Deserialize(#[source] Arc), -} - -impl tor_error::HasKind for Error { - #[rustfmt::skip] // the tabular layout of the `match` makes this a lot clearer - fn kind(&self) -> ErrorKind { - use Error as E; - use tor_error::ErrorKind as K; - match self { - E::IoError(..) => K::PersistentStateAccessFailed, - E::Permissions(e) => if e.is_bad_permission() { - K::FsPermissions - } else { - K::PersistentStateAccessFailed - } - E::NoLock => K::BadApiUsage, - E::Serialize(..) => K::Internal, - E::Deserialize(..) => K::PersistentStateCorrupted, - } - } -} - -impl From for Error { - fn from(e: std::io::Error) -> Error { - Error::IoError(Arc::new(e)) - } -} - -/// Error conversion for JSON errors; use only when loading -fn load_error(e: serde_json::Error) -> Error { - Error::Deserialize(Arc::new(e)) -} - -/// Error conversion for JSON errors; use only when storing -fn store_error(e: serde_json::Error) -> Error { - Error::Serialize(Arc::new(e)) -} - /// A wrapper type for types whose representation may change in future versions of Arti. /// /// This uses `#[serde(untagged)]` to attempt deserializing as a type `T` first, and falls back diff --git a/crates/tor-persist/src/testing.rs b/crates/tor-persist/src/testing.rs index 01f264302..49efe09fd 100644 --- a/crates/tor-persist/src/testing.rs +++ b/crates/tor-persist/src/testing.rs @@ -1,6 +1,6 @@ //! Testing-only StateMgr that stores values in a hash table. -use crate::{load_error, store_error}; +use crate::err::{Action, ErrorSource, Resource}; use crate::{Error, LockStatus, Result, StateMgr}; use serde::{de::DeserializeOwned, Serialize}; use std::collections::HashMap; @@ -87,6 +87,13 @@ impl TestingStateMgr { inner: Arc::new(Mutex::new(new_inner)), } } + + /// Return an error Resource corresponding to a given `key`. + fn err_resource(&self, key: &str) -> Resource { + Resource::Temporary { + key: key.to_string(), + } + } } impl StateMgr for TestingStateMgr { @@ -98,7 +105,11 @@ impl StateMgr for TestingStateMgr { let storage = inner.storage.lock().expect("Lock poisoned."); let content = storage.entries.get(key); match content { - Some(value) => Ok(Some(serde_json::from_str(value).map_err(load_error)?)), + Some(value) => { + Ok(Some(serde_json::from_str(value).map_err(|e| { + Error::new(e, Action::Loading, self.err_resource(key)) + })?)) + } None => Ok(None), } } @@ -109,11 +120,16 @@ impl StateMgr for TestingStateMgr { { let inner = self.inner.lock().expect("Lock poisoned."); if !inner.lock_held { - return Err(Error::NoLock); + return Err(Error::new( + ErrorSource::NoLock, + Action::Storing, + Resource::Manager, + )); } let mut storage = inner.storage.lock().expect("Lock poisoned."); - let val = serde_json::to_string_pretty(val).map_err(store_error)?; + let val = serde_json::to_string_pretty(val) + .map_err(|e| Error::new(e, Action::Storing, self.err_resource(key)))?; storage.entries.insert(key.to_string(), val); Ok(()) @@ -191,7 +207,10 @@ mod test { }; assert_eq!(mgr.load::("item1").unwrap(), None); - assert!(matches!(mgr.store("item1", &v1), Err(Error::NoLock))); + assert!(matches!( + mgr.store("item1", &v1).unwrap_err().source(), + ErrorSource::NoLock + )); assert!(!mgr.can_store()); assert_eq!(mgr.try_lock().unwrap(), LockStatus::NewlyAcquired); @@ -250,7 +269,10 @@ mod test { s2: "yrfmstbyes".into(), }; - assert!(matches!(h1.store(&v1), Err(Error::NoLock))); + assert!(matches!( + h1.store(&v1).unwrap_err().source(), + ErrorSource::NoLock + )); assert!(mgr.try_lock().unwrap().held()); assert!(h1.can_store()); assert!(h1.store(&v1).is_ok());