vincenzopalazzo
/
arti
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

Merge branch 'persist-error-cleanup' into 'main' 

tor-persist: Big refactoring on Error type.

See merge request tpo/core/arti!614
This commit is contained in:
eta 2022-07-06 17:20:43 +00:00
parent 6878e3a2d4 853aed6859
commit 0537e88d80
7 changed files with 259 additions and 93 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

1
Cargo.lock generated
View File

@ -3845,6 +3845,7 @@ dependencies = [
name = "tor-persist"
version = "0.4.1"
dependencies = [
"derive_more",
"fs-mistrust",
"fslock",
"sanitize-filename",

1
crates/tor-persist/Cargo.toml
View File

@ -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"] }

1
crates/tor-persist/semver.md Normal file
View File

@ -0,0 +1 @@
BREAKING: Error type completely changed.

146
crates/tor-persist/src/err.rs Normal file
View File

@ -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<std::io::Error>),
/// 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<serde_json::Error>),
}
/// 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<ErrorSource>, 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<std::io::Error> for ErrorSource {
fn from(e: std::io::Error) -> ErrorSource {
ErrorSource::IoError(Arc::new(e))
}
}
impl From<serde_json::Error> for ErrorSource {
fn from(e: serde_json::Error) -> ErrorSource {
ErrorSource::Serde(Arc::new(e))
}
}

98
crates/tor-persist/src/fs.rs
View File

@ -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<Self> {
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<S>(&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<Option<HashMap<String, u32>>> = store.load("quoll");
assert!(h.is_err());
assert!(matches!(h, Err(Error::IoError(_))));
assert!(matches!(h.unwrap_err().source(), ErrorSource::IoError(_)));
Ok(())
}

71
crates/tor-persist/src/lib.rs
View File

@ -44,6 +44,7 @@
#![allow(clippy::significant_drop_in_scrutinee)] // arti/-/merge_requests/588/#note_2812945
//! <!-- @@ end lint list maintained by maint/add_warning @@ -->
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<T> = std::result::Result<T, crate::Error>;
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<std::io::Error>),
/// 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<serde_json::Error>),
/// Problem when deserializing JSON data.
#[error("JSON serialization error")]
Deserialize(#[source] Arc<serde_json::Error>),
}
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<std::io::Error> 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

34
crates/tor-persist/src/testing.rs
View File

@ -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::<Ex1>("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());