From b774ac0e678223469aebbf9659b88c36e381f44a Mon Sep 17 00:00:00 2001
From: Matthias Beyer <mail@beyermatthias.de>
Date: Fri, 8 Jun 2018 01:42:28 +0200
Subject: [PATCH] Fix: Deleting an Entry could leave artifacts in cache

This patch fixes a bug we did not even hit (yet). It is: When deleting
an Entry from the store, this could potentially leave artifacts in the
cache.

Szenario: An Entry, which was loaded (via `Store::get()` for example),
gets `Store::delete()`ed twice. The first call would work as expected,
but leave the Entry in the Store cache. The second call would then fail,
as the Entry is already removed on the FS, but still in the cache. This
would fail - which is the right thing to do here - but with the wrong
error (with a FileError rather than a FileNotFound error).

This patch fixes this.

First of all, the appropriate `PathBuf` object is calculated in all
cases, as this object is needed to check whether the file is actually
there (which could be the case if the Entry is in the cache and if it is
not).

If the entry is in the cache and is borrowed: error. If not, remove the
entry from the cache. Afterwards the file is deleted on disk.

If the entry is not in the cache, but the file exists, the file is removed.
If the file does not exist: error.

Signed-off-by: Matthias Beyer <mail@beyermatthias.de>
---
 lib/core/libimagstore/src/store.rs | 61 ++++++++++++++++--------------
 1 file changed, 32 insertions(+), 29 deletions(-)

diff --git a/lib/core/libimagstore/src/store.rs b/lib/core/libimagstore/src/store.rs
index bc6d4d2e..032f2c3e 100644
--- a/lib/core/libimagstore/src/store.rs
+++ b/lib/core/libimagstore/src/store.rs
@@ -444,7 +444,7 @@ impl Store {
         StoreEntry::new(id, &self.backend)?.get_entry()
     }
 
-    /// Delete an entry
+    /// Delete an entry and the corrosponding file on disk
     ///
     /// # Return value
     ///
@@ -460,6 +460,12 @@ impl Store {
 
         debug!("Deleting id: '{}'", id);
 
+        // Small optimization: We need the pathbuf for deleting, but when calling
+        // StoreId::exists(), a PathBuf object gets allocated. So we simply get a
+        // PathBuf here, check whether it is there and if it is, we can re-use it to
+        // delete the filesystem file.
+        let pb = id.clone().into_pathbuf()?;
+
         {
             let mut entries = self
                 .entries
@@ -467,44 +473,41 @@ impl Store {
                 .map_err(|_| SE::from_kind(SEK::LockPoisoned))
                 .chain_err(|| SEK::DeleteCallError(id.clone()))?;
 
-            // if the entry is currently modified by the user, we cannot drop it
-            match entries.get(&id) {
+            let do_remove = match entries.get(&id) {
+                Some(e) => if e.is_borrowed() { // entry is currently borrowed, we cannot delete it
+                    return Err(SE::from_kind(SEK::IdLocked)).chain_err(|| SEK::DeleteCallError(id));
+                    // false
+                } else { // Entry is in the cache
+                    // Remove Entry from the cache
+                    true
+                },
+
                 None => {
                     // The entry is not in the internal cache. But maybe on the filesystem?
                     debug!("Seems like {:?} is not in the internal cache", id);
 
-                    // Small optimization: We need the pathbuf for deleting, but when calling
-                    // StoreId::exists(), a PathBuf object gets allocated. So we simply get a
-                    // PathBuf here, check whether it is there and if it is, we can re-use it to
-                    // delete the filesystem file.
-                    let pb = id.clone().into_pathbuf()?;
-
-                    if self.backend.exists(&pb)? {
-                        // looks like we're deleting a not-loaded file from the store.
-                        debug!("Seems like {:?} is on the FS", pb);
-                        return self.backend.remove_file(&pb)
-                    } else {
+                    if !self.backend.exists(&pb)? {
                         debug!("Seems like {:?} is not even on the FS", pb);
                         return Err(SE::from_kind(SEK::FileNotFound))
                             .chain_err(|| SEK::DeleteCallError(id))
-                    }
-                },
-                Some(e) => if e.is_borrowed() {
-                    return Err(SE::from_kind(SEK::IdLocked))
-                        .chain_err(|| SEK::DeleteCallError(id))
-                }
-            }
+                    } // else { continue }
 
-            // remove the entry first, then the file
-            entries.remove(&id);
-            let pb = id.clone().with_base(self.path().clone()).into_pathbuf()?;
-            let _ = self
-                .backend
-                .remove_file(&pb)
-                .chain_err(|| SEK::FileError)
-                .chain_err(|| SEK::DeleteCallError(id))?;
+                    false
+                },
+            };
+
+            if do_remove {
+                let _ = entries.remove(&id);
+            }
         }
 
+        debug!("Seems like {:?} is on the FS", pb);
+        let _ = self
+            .backend
+            .remove_file(&pb)
+            .chain_err(|| SEK::FileError)
+            .chain_err(|| SEK::DeleteCallError(id))?;
+
         debug!("Deleted");
         Ok(())
     }