Panic when deleting file with a non-UTF-8 name

[sxyazi]

When deleting a file created by touch ''$'\250', the program panics:

thread 'main' panicked at /home/ika/.cargo/registry/src/index.crates.io-6f17d22bba15001f/trash-4.1.0/src/freedesktop.rs:448:31:
called `Option::unwrap()` on a `None` value
stack backtrace:
   0: rust_begin_unwind
             at /rustc/82e1608dfa6e0b5569232559e3d385fea5a93112/library/std/src/panicking.rs:645:5
   1: core::panicking::panic_fmt
             at /rustc/82e1608dfa6e0b5569232559e3d385fea5a93112/library/core/src/panicking.rs:72:14
   2: core::panicking::panic
             at /rustc/82e1608dfa6e0b5569232559e3d385fea5a93112/library/core/src/panicking.rs:127:5
   3: core::option::Option<T>::unwrap
             at /rustc/82e1608dfa6e0b5569232559e3d385fea5a93112/library/core/src/option.rs:931:21
   4: trash::platform::move_to_trash
             at /home/ika/.cargo/registry/src/index.crates.io-6f17d22bba15001f/trash-4.1.0/src/freedesktop.rs:448:13
   5: trash::platform::<impl trash::TrashContext>::delete_all_canonicalized::{{closure}}
             at /home/ika/.cargo/registry/src/index.crates.io-6f17d22bba15001f/trash-4.1.0/src/freedesktop.rs:49:21
   6: trash::platform::execute_on_mounted_trash_folders
             at /home/ika/.cargo/registry/src/index.crates.io-6f17d22bba15001f/trash-4.1.0/src/freedesktop.rs:408:9
   7: trash::platform::<impl trash::TrashContext>::delete_all_canonicalized
             at /home/ika/.cargo/registry/src/index.crates.io-6f17d22bba15001f/trash-4.1.0/src/freedesktop.rs:48:17
   8: trash::TrashContext::delete_all
             at /home/ika/.cargo/registry/src/index.crates.io-6f17d22bba15001f/trash-4.1.0/src/lib.rs:112:9
   9: trash::TrashContext::delete
             at /home/ika/.cargo/registry/src/index.crates.io-6f17d22bba15001f/trash-4.1.0/src/lib.rs:85:9
  10: trash::delete
             at /home/ika/.cargo/registry/src/index.crates.io-6f17d22bba15001f/trash-4.1.0/src/lib.rs:120:5
  11: test_trash::main
             at ./src/main.rs:9:2
  12: core::ops::function::FnOnce::call_once
             at /rustc/82e1608dfa6e0b5569232559e3d385fea5a93112/library/core/src/ops/function.rs:250:5
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.

Here's a minimal reproducing code:

use std::ffi::OsStr;

fn main() {
	// touch ''$'\250'
	let s = unsafe { OsStr::from_encoded_bytes_unchecked(&[168]) };

	std::fs::File::create(s).unwrap();

	trash::delete(s).ok();

	println!("Hello, world!");
}

I think trash should handle non-UTF-8 characters, or at least return an error instead of panicking.

[Byron]

Thanks a lot for reporting!

The example will probably serve as a perfect test to add to the suite, allowing a fix to be put in place easily. Generally the codebase understands the difference between String and Path, so I'd hope the issue isn't wide-spread and maybe just in this one place, facilitating a fix.

[markus-bauer]

It's this:

trash-rs/src/freedesktop.rs

Lines 445 to 448 in 46585ce

	let in_trash_name = if appendage > 1 {
	format!("{}.{}", filename.to_str().unwrap(), appendage)
	} else {
	filename.to_str().unwrap().into()

You should do this with OsString. It doesn't seem like you actually need Strings. Example:

diff --git a/src/freedesktop.rs b/src/freedesktop.rs
index 8373ef9..0344935 100644
--- a/src/freedesktop.rs
+++ b/src/freedesktop.rs
@@ -7,8 +7,9 @@
 //!

 use std::{
-    borrow::Borrow,
+    borrow::{Borrow, Cow},
     collections::HashSet,
+    ffi::OsString,
     fs::{self, File, OpenOptions},
     io::{BufRead, BufReader, Write},
     os::unix::{ffi::OsStrExt, fs::PermissionsExt},
@@ -443,11 +444,18 @@ fn move_to_trash(
     loop {
         appendage += 1;
         let in_trash_name = if appendage > 1 {
-            format!("{}.{}", filename.to_str().unwrap(), appendage)
+            let mut buf = filename.to_os_string();
+            buf.push(".");
+            buf.push(appendage.to_string());
+            Cow::Owned(buf)
         } else {
-            filename.to_str().unwrap().into()
+            Cow::Borrowed(filename)
+        };
+        let info_name = {
+            let mut buf = OsString::from(&in_trash_name);
+            buf.push(".trashinfo");
+            buf
         };
-        let info_name = format!("{in_trash_name}.trashinfo");
         let info_file_path = info_folder.join(&info_name);
         let info_result = OpenOptions::new().create_new(true).write(true).open(&info_file_path);
         match info_result {

There are many other to_str().unwrap() in this code!

[Byron]

Thanks for researching this, a PR would definitely be welcome. Thanks again for your help.

[markus-bauer]

I wouldn't be comfortable making the PR, because I don't know the code well enough. But you should be able to fix this easily. It was just a mistake from the start to use Strings for this.