From 3413646934384d76cba47e171a271bcae10e06e2 Mon Sep 17 00:00:00 2001 From: Matthias Beyer Date: Thu, 10 Mar 2016 18:14:53 +0100 Subject: [PATCH 1/3] Revert "Check whether the StoreId is inside the store, before doing anything on the FS" This reverts commit 373502217e62ad07f76af4579680ad7156bbe390 as this commit was introducing a bug. The StoreId type says `/test/example` for a store id path, which is completely valid, as the root (`/`) is the store itself. The id_in_store() function assumed that the store-id is the full (file-system) path to the store entry, which is false. This commit does not introduce a fix but revert the check. --- libimagstore/src/error.rs | 2 -- libimagstore/src/store.rs | 25 +------------------------ 2 files changed, 1 insertion(+), 26 deletions(-) diff --git a/libimagstore/src/error.rs b/libimagstore/src/error.rs index be1375f0..39b47034 100644 --- a/libimagstore/src/error.rs +++ b/libimagstore/src/error.rs @@ -19,7 +19,6 @@ pub enum StoreErrorKind { IoError, StorePathExists, StorePathCreate, - StorePathOutsideStore, LockPoisoned, EntryAlreadyBorrowed, EntryAlreadyExists, @@ -42,7 +41,6 @@ fn store_error_type_as_str(e: &StoreErrorKind) -> &'static str { &StoreErrorKind::IoError => "File Error", &StoreErrorKind::StorePathExists => "Store path exists", &StoreErrorKind::StorePathCreate => "Store path create", - &StoreErrorKind::StorePathOutsideStore => "Store path would be outside of store", &StoreErrorKind::LockPoisoned => "The internal Store Lock has been poisoned", &StoreErrorKind::EntryAlreadyBorrowed => "Entry is already borrowed", diff --git a/libimagstore/src/store.rs b/libimagstore/src/store.rs index f1018d8a..bb4e6f0d 100644 --- a/libimagstore/src/store.rs +++ b/libimagstore/src/store.rs @@ -131,11 +131,6 @@ impl Store { /// Creates the Entry at the given location (inside the entry) pub fn create<'a>(&'a self, id: StoreId) -> Result> { - if !self.id_in_store(&id) { - debug!("'{:?}' seems not to be in '{:?}'", id, self.location); - return Err(StoreError::new(StoreErrorKind::StorePathOutsideStore, None)); - } - let hsmap = self.entries.write(); if hsmap.is_err() { return Err(StoreError::new(StoreErrorKind::LockPoisoned, None)) @@ -155,11 +150,6 @@ impl Store { /// Borrow a given Entry. When the `FileLockEntry` is either `update`d or /// dropped, the new Entry is written to disk pub fn retrieve<'a>(&'a self, id: StoreId) -> Result> { - if !self.id_in_store(&id) { - debug!("'{:?}' seems not to be in '{:?}'", id, self.location); - return Err(StoreError::new(StoreErrorKind::StorePathOutsideStore, None)); - } - self.entries .write() .map_err(|_| StoreError::new(StoreErrorKind::LockPoisoned, None)) @@ -228,11 +218,6 @@ impl Store { /// Delete an entry pub fn delete(&self, id: StoreId) -> Result<()> { - if !self.id_in_store(&id) { - debug!("'{:?}' seems not to be in '{:?}'", id, self.location); - return Err(StoreError::new(StoreErrorKind::StorePathOutsideStore, None)); - } - let entries_lock = self.entries.write(); if entries_lock.is_err() { return Err(StoreError::new(StoreErrorKind::LockPoisoned, None)) @@ -250,19 +235,11 @@ impl Store { remove_file(&id).map_err(|e| StoreError::new(StoreErrorKind::FileError, Some(Box::new(e)))) } - fn id_in_store(&self, path: &StoreId) -> bool { - path.canonicalize() - .map(|can| { - can.starts_with(&self.location) - }) - .unwrap_or(path.starts_with(&self.location)) - // we return false, as fs::canonicalize() returns an Err(..) on filesystem errors - } - /// Gets the path where this store is on the disk pub fn path(&self) -> &PathBuf { &self.location } + } impl Drop for Store { From a3a09ff4ee522f2a26ab2231c83c365d07afc3a6 Mon Sep 17 00:00:00 2001 From: Matthias Beyer Date: Thu, 10 Mar 2016 18:44:24 +0100 Subject: [PATCH 2/3] Fix store creation of libimagstore The macro may not prefix the StoreId internal PathBuf with '/', so this path is not absolute. This way we can introduce `storify_id()` which creates a proper PathBuf into the store out of our StoreId object. Does not yet do verification whether the path is inside the store, actually. --- libimagstore/src/store.rs | 12 ++++++++++++ libimagstore/src/storeid.rs | 2 +- 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/libimagstore/src/store.rs b/libimagstore/src/store.rs index bb4e6f0d..7a11b8c2 100644 --- a/libimagstore/src/store.rs +++ b/libimagstore/src/store.rs @@ -129,8 +129,17 @@ impl Store { }) } + fn storify_id(&self, id: StoreId) -> StoreId { + debug!("Create new store id out of: {:?} and {:?}", self.location, id); + let mut new_id = self.location.clone(); + new_id.push(id); + debug!("Created: '{:?}'", new_id); + new_id + } + /// Creates the Entry at the given location (inside the entry) pub fn create<'a>(&'a self, id: StoreId) -> Result> { + let id = self.storify_id(id); let hsmap = self.entries.write(); if hsmap.is_err() { return Err(StoreError::new(StoreErrorKind::LockPoisoned, None)) @@ -150,6 +159,7 @@ impl Store { /// Borrow a given Entry. When the `FileLockEntry` is either `update`d or /// dropped, the new Entry is written to disk pub fn retrieve<'a>(&'a self, id: StoreId) -> Result> { + let id = self.storify_id(id); self.entries .write() .map_err(|_| StoreError::new(StoreErrorKind::LockPoisoned, None)) @@ -201,6 +211,7 @@ impl Store { /// Retrieve a copy of a given entry, this cannot be used to mutate /// the one on disk pub fn retrieve_copy(&self, id: StoreId) -> Result { + let id = self.storify_id(id); let entries_lock = self.entries.write(); if entries_lock.is_err() { return Err(StoreError::new(StoreErrorKind::LockPoisoned, None)) @@ -218,6 +229,7 @@ impl Store { /// Delete an entry pub fn delete(&self, id: StoreId) -> Result<()> { + let id = self.storify_id(id); let entries_lock = self.entries.write(); if entries_lock.is_err() { return Err(StoreError::new(StoreErrorKind::LockPoisoned, None)) diff --git a/libimagstore/src/storeid.rs b/libimagstore/src/storeid.rs index 12813973..35979fe8 100644 --- a/libimagstore/src/storeid.rs +++ b/libimagstore/src/storeid.rs @@ -44,7 +44,7 @@ macro_rules! module_entry_path_mod { /// Path has to be a valid UTF-8 string or this will panic! pub fn new>(pa: P) -> ModuleEntryPath { let mut path = PathBuf::new(); - path.push(format!("/{}", $name)); + path.push(format!("{}", $name)); path.push(pa.as_ref().clone()); let version = Version::parse($version).unwrap(); let name = pa.as_ref().file_name().unwrap() From 6d7065d10efd6414227e201b5b4b27aed50f92f6 Mon Sep 17 00:00:00 2001 From: Matthias Beyer Date: Thu, 10 Mar 2016 19:12:00 +0100 Subject: [PATCH 3/3] Fix test: StoreId does not start with "/" anymore --- libimagstore/src/storeid.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/libimagstore/src/storeid.rs b/libimagstore/src/storeid.rs index 35979fe8..92eaec49 100644 --- a/libimagstore/src/storeid.rs +++ b/libimagstore/src/storeid.rs @@ -99,8 +99,7 @@ mod test { fn correct_path() { let p = module_path::ModuleEntryPath::new("test"); - assert_eq!(p.into_storeid().to_str().unwrap(), - "/test/test~0.2.0-alpha+leet1337"); + assert_eq!(p.into_storeid().to_str().unwrap(), "test/test~0.2.0-alpha+leet1337"); } }