Fix purge_all and restore_all reading invvalid memory and not exe…

…cuting the operation on the requested items. Add test cases for `purge_all` and `restore_all`. Test are now thread safe.

Cargo.toml

@@ -16,15 +16,19 @@ coinit_multithreaded = []
 coinit_disable_ole1dde = []
 coinit_speed_over_memory = []
 
-[dependencies]
+[dev-dependencies]
+chrono = "0.4.9"
+rand = "0.7.2"
+lazy_static = "1.4.0"
 
+[dependencies]
 
 [target.'cfg(windows)'.dependencies]
 scopeguard = "1.0.0"
 
 [target.'cfg(windows)'.dependencies.winapi]
 git = "https://github.com/ArturKovacs/winapi-rs.git"
-branch = "for-trash"
+rev = "b5c196ad2aa27b26e0ed0ae1f35ac790d677a3b5"
 features = [
     "combaseapi", "objbase", "shlobj", "shellapi", "winerror", "shlobj", "shlwapi", "shobjidl_core",
     "shobjidl", "oleauto", "oaidl", "wtypes", "errhandlingapi", "timezoneapi", "winuser"

src/tests.rs

@@ -1,40 +1,170 @@
-use crate::{remove, remove_all};
+use crate as trash;
 use std::fs::{create_dir, File};
 use std::path::PathBuf;
+use std::collections::{HashMap, hash_map::Entry};
+use std::sync::atomic::{AtomicI64, Ordering};
+
+use chrono;
+use lazy_static::lazy_static;
+
+// WARNING Expecting that `cargo test` won't be invoked on the same computer more than once within
+// a single millisecond
+lazy_static! {
+    static ref INSTANCE_ID: i64 = chrono::Local::now().timestamp_millis();
+    static ref ID_OFFSET: AtomicI64 = AtomicI64::new(0);
+}
+fn get_unique_name() -> String {
+    let id = ID_OFFSET.fetch_add(1, Ordering::SeqCst);
+    format!("trash-test-{}-{}", *INSTANCE_ID, id)
+}
 
 #[test]
 fn create_remove() {
-    let path = "test_file_to_remove";
-    File::create(path).unwrap();
-
-    remove(path).unwrap();
-    assert!(File::open(path).is_err());
+    let file_name = get_unique_name();
+    File::create(&file_name).unwrap();
+    trash::remove(&file_name).unwrap();
+    assert!(File::open(&file_name).is_err());
 }
 
 #[test]
 fn create_remove_folder() {
-    let path = PathBuf::from("test_folder_to_remove");
+    let folder_name = get_unique_name();
+    let path = PathBuf::from(folder_name);
     create_dir(&path).unwrap();
     File::create(path.join("file_in_folder")).unwrap();
 
     assert!(path.exists());
-    remove(&path).unwrap();
+    trash::remove(&path).unwrap();
     assert!(path.exists() == false);
 }
 
 #[test]
 fn create_multiple_remove_all() {
+    let file_name_prefix = get_unique_name();
     let count: usize = 3;
-
     let paths: Vec<_> = (0..count)
-        .map(|i| format!("test_file_to_remove_{}", i))
+        .map(|i| format!("{}#{}", file_name_prefix, i))
         .collect();
     for path in paths.iter() {
         File::create(path).unwrap();
     }
 
-    remove_all(&paths).unwrap();
+    trash::remove_all(&paths).unwrap();
     for path in paths.iter() {
         assert!(File::open(path).is_err());
     }
 }
+
+#[test]
+fn list() {
+    let file_name_prefix = get_unique_name();
+    let batches: usize = 2;
+    let files_per_batch: usize = 3;
+    let names: Vec<_> = (0..files_per_batch)
+        .map(|i| format!("{}#{}", file_name_prefix, i))
+        .collect();
+    for _ in 0..batches {
+        for path in names.iter() {
+            File::create(path).unwrap();
+        }
+        trash::remove_all(&names).unwrap();
+    }
+    let items = trash::list().unwrap();
+    let items: HashMap<_, Vec<_>> =
+        items.into_iter().filter(|x| x.name.starts_with(&file_name_prefix))
+        .fold(HashMap::new(), |mut map, x| {
+            match map.entry(x.name.clone()) {
+                Entry::Occupied(mut entry) => {
+                    entry.get_mut().push(x);
+                }
+                Entry::Vacant(entry) => {
+                    entry.insert(vec![x]);
+                }
+            }
+            map
+        });
+    for name in names {
+        assert_eq!(items.get(&name).unwrap().len(), batches);
+    }
+
+    // Let's try to purge all the items we just created but ignore any errors
+    // as this test should succeed as long as `list` works properly.
+    let _ = trash::purge_all(items.into_iter().map(|(_name, item)| item).flatten());
+}
+
+#[test]
+fn purge() {
+    let file_name_prefix = get_unique_name();
+    let batches: usize = 2;
+    let files_per_batch: usize = 3;
+    let names: Vec<_> = (0..files_per_batch)
+        .map(|i| format!("{}#{}", file_name_prefix, i))
+        .collect();
+    for _ in 0..batches {
+        for path in names.iter() {
+            File::create(path).unwrap();
+        }
+        trash::remove_all(&names).unwrap();
+    }
+
+    // Collect it because we need the exact number of items gathered.
+    let targets: Vec<_> = trash::list()
+        .unwrap()
+        .into_iter()
+        .filter(|x| x.name.starts_with(&file_name_prefix))
+        .collect();
+    assert_eq!(targets.len(), batches * files_per_batch);
+    trash::purge_all(targets).unwrap();
+    // Ugly hack but need to wait for example on Windows one must wait a bit
+    // before the items acctually leave the trash
+    //std::thread::sleep(std::time::Duration::from_secs(8));
+    let remaining = trash::list()
+        .unwrap()
+        .into_iter()
+        .filter(|x| x.name.starts_with(&file_name_prefix))
+        .count();
+    assert_eq!(remaining, 0);
+}
+
+#[test]
+fn restore() {
+    let file_name_prefix = get_unique_name();
+    let file_count: usize = 3;
+    let names: Vec<_> = (0..file_count)
+        .map(|i| format!("{}#{}", file_name_prefix, i))
+        .collect();
+    for path in names.iter() {
+        File::create(path).unwrap();
+    }
+    trash::remove_all(&names).unwrap();
+
+    // Collect it because we need the exact number of items gathered.
+    let targets: Vec<_> = trash::list()
+        .unwrap()
+        .into_iter()
+        .filter(|x| x.name.starts_with(&file_name_prefix))
+        .collect();
+    assert_eq!(targets.len(), file_count);
+    trash::restore_all(targets).unwrap();
+
+    // Ugly hack but need to wait for example on Windows one must wait a bit
+    // before the items acctually leave the trash
+    //std::thread::sleep(std::time::Duration::from_secs(8));
+
+    let remaining = trash::list()
+        .unwrap()
+        .into_iter()
+        .filter(|x| x.name.starts_with(&file_name_prefix))
+        .count();
+    assert_eq!(remaining, 0);
+
+    // They are not in the trash anymore but they should be at their original location
+    for path in names.iter() {
+        assert!(File::open(path).is_ok());
+    }
+
+    // Good ol' remove to clean up
+    for path in names.iter() {
+        std::fs::remove_file(path).unwrap();
+    }
+}

src/windows.rs

@@ -153,6 +153,7 @@ pub fn list() -> Result<Vec<TrashItem>, Error> {
         let mut item_vec = Vec::new();
         let mut item: PITEMID_CHILD = std::mem::uninitialized();
         while (*peidl).Next(1, &mut item as *mut _, std::ptr::null_mut()) == S_OK {
+            defer! {{ CoTaskMemFree(item as LPVOID); }}
             let id = get_display_name(recycle_bin as *mut _, item, SHGDN_FORPARSING)?;
             let name = get_display_name(recycle_bin as *mut _, item, SHGDN_INFOLDER)?;
 
@@ -167,8 +168,6 @@ pub fn list() -> Result<Vec<TrashItem>, Error> {
                 original_parent: PathBuf::from(orig_loc),
                 time_deleted: date_deleted,
             });
-            //PrintDetail(psfRecycleBin, pidlItem, &PKEY_Size, TEXT("Size"));
-            CoTaskMemFree(item as LPVOID);
         }
         return Ok(item_vec);
     }
@@ -377,7 +376,7 @@ fn execute_verb_on_all(
             }
         });
         for item in items {
-            let mut id_wstr: Vec<_> = item.id.encode_wide().collect();
+            let mut id_wstr: Vec<_> = item.id.encode_wide().chain(std::iter::once(0)).collect();
             let mut pidl = MaybeUninit::<PIDLIST_RELATIVE>::uninit();
             return_err_on_fail! {
                 (*recycle_bin).ParseDisplayName(