Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Invalid original path #47

Closed
aalhitennf opened this issue May 5, 2022 · 9 comments
Closed

Invalid original path #47

aalhitennf opened this issue May 5, 2022 · 9 comments
Labels
help wanted Extra attention is needed question Further information is requested

Comments

@aalhitennf
Copy link

aalhitennf commented May 5, 2022

Running the code from example:

use std::fs::File;

fn main() {
    // Let's create and remove a single file
    File::create("remove-me").unwrap();
    trash::delete("remove-me").unwrap();
    assert!(File::open("remove-me").is_err());
}

File goes to the trash bin but original path is wrong and file cant be restored. Real original path:

/home/myuser/test/remove-me

original path shown in trashed file:

/home/myuser/.local/share/myuser/test/remove-me

platform: linux
trash-rs: 2.0.4
rust: 1.60.0 (7737e0b5c 2022-04-04)

Byron added a commit that referenced this issue May 6, 2022
It seems to work as expected, also on linux.
@Byron Byron added help wanted Extra attention is needed question Further information is requested labels May 6, 2022
@Byron
Copy link
Owner

Byron commented May 6, 2022

Thanks for posting. This issue doesn't reproduce for me though, and I tested it on three platforms. Can you help?

@aalhitennf
Copy link
Author

aalhitennf commented May 6, 2022

I cloned the repo and ran cargo test on Windows 10, Fedora 35 and Arch Linux. On Windows 10 all tests completed successfully, but restore tests failed on both linux distros.

failures:
    tests::os_limited::restore
    tests::os_limited::restore_collision  

I took a quick look at the source, did few println's and noticed that variable value in freedesktop.rs (line 168) is something like this:

println!("key value: {:?} {:?}", key, value);
key value: "Path" "myuser/temp/trash-rs/trash-test-1651831414125-2%231"

And few lines later at line 173 in if statement:

value_path = trash_folder_parent.join(value_path);

Results value_path to be incorrect:

/home/myuser/.local/share/myuser/temp/trash-rs/trash-test-1651831414125-2%231

I don't have more time to put in this right now but i hope it helps. Repo is cloned into /home/myuser/temp/trash-rs to be clear.

@Byron
Copy link
Owner

Byron commented May 7, 2022

I setup a Fedora 34 VM and could reproduce the restore errors, while also noticing the oddly named names for test files. That should make it fixable.

@Byron
Copy link
Owner

Byron commented May 9, 2022

I added a few more debug prints and arrived at the same conclusion: it is unable to calculate the correct original path from the values it sees in the trash info files. This makes any restore generally fail.
The question is how to properly compute the original value given that it seems to change depending on the distro. Maybe ultimately one has to add more heuristics to detect this case but it all seems quite fragile.

Here is the output for completeness.

---- tests::os_limited::restore_collision stdout ----
[src/tests.rs:197] std::path::Path::new(&path).canonicalize().unwrap() = "/home/parallels/dev-local/trash-rs/trash-test-1652054935801-0#1"
[src/tests.rs:197] std::path::Path::new(&path).canonicalize().unwrap() = "/home/parallels/dev-local/trash-rs/trash-test-1652054935801-0#2"
[src/freedesktop.rs:172] &value_path = "parallels/dev-local/trash-rs/trash-test-1652054935801-0%230"
[src/freedesktop.rs:172] &value_path = "parallels/dev-local/trash-rs/trash-test-1652054935801-0%231"
[src/freedesktop.rs:172] &value_path = "parallels/dev-local/trash-rs/trash-test-1652054935801-0%232"
[src/freedesktop.rs:263] &trash_folder = "/home/parallels/.local/share/Trash"
[src/freedesktop.rs:264] &item = TrashItem {
    id: "/home/parallels/.local/share/Trash/info/trash-test-1652054935801-0#0.trashinfo",
    name: "trash-test-1652054935801-0#0",
    original_parent: "/home/parallels/.local/share/parallels/dev-local/trash-rs",
    time_deleted: 1652054935,
}
[src/freedesktop.rs:303] &file = "/home/parallels/.local/share/Trash/files/trash-test-1652054935801-0#0"
[src/freedesktop.rs:303] &original_path = "/home/parallels/.local/share/parallels/dev-local/trash-rs/trash-test-1652054935801-0#0"
[src/freedesktop.rs:263] &trash_folder = "/home/parallels/.local/share/Trash"
[src/freedesktop.rs:264] &item = TrashItem {
    id: "/home/parallels/.local/share/Trash/info/trash-test-1652054935801-0#1.trashinfo",
    name: "trash-test-1652054935801-0#1",
    original_parent: "/home/parallels/.local/share/parallels/dev-local/trash-rs",
    time_deleted: 1652054935,
}
[src/freedesktop.rs:303] &file = "/home/parallels/.local/share/Trash/files/trash-test-1652054935801-0#1"
[src/freedesktop.rs:303] &original_path = "/home/parallels/.local/share/parallels/dev-local/trash-rs/trash-test-1652054935801-0#1"
[src/freedesktop.rs:263] &trash_folder = "/home/parallels/.local/share/Trash"
[src/freedesktop.rs:264] &item = TrashItem {
    id: "/home/parallels/.local/share/Trash/info/trash-test-1652054935801-0#2.trashinfo",
    name: "trash-test-1652054935801-0#2",
    original_parent: "/home/parallels/.local/share/parallels/dev-local/trash-rs",
    time_deleted: 1652054935,
}
[src/freedesktop.rs:303] &file = "/home/parallels/.local/share/Trash/files/trash-test-1652054935801-0#2"
[src/freedesktop.rs:303] &original_path = "/home/parallels/.local/share/parallels/dev-local/trash-rs/trash-test-1652054935801-0#2"

Byron added a commit that referenced this issue May 10, 2022
For some reason, on some unixes and in some 'trash' folders the
original file paths include the user name and the trash directory
itself is buried in .local/share. This violates the previous assumption
that the trash folder is in the users home and leads to invalid
original paths, causing restore to fail.

Now that I am thinking about it… isn't it use who write the trash info
file?
Byron added a commit that referenced this issue May 10, 2022
…f those were relative (#47)

Previously it would be unable to reconstruct original paths if the trash
directory was on a mount point due to a 'split brain' of sorts.

When trashing files it would create original path information based
on them being relative to a mount point, but when restoring them
it would reconstruct them to be relative to the trash top level
directory.

Now the reconstruction happens against to mount point itself which makes
restoration match.
@Byron
Copy link
Owner

Byron commented May 10, 2022

This is fixed in v2.1.1, maybe you can give it a try.

The cause was improper handling of relative original paths which would occour if the deleted file was on a mount point. This is the case on ubuntu, but not on other systems. Nonetheless, this also means that all restore operations would fail on any mount point, which is probably very likely.

@Byron Byron closed this as completed May 10, 2022
@aalhitennf
Copy link
Author

I ran tests without errors on Fedora, Arch, and EndeavourOS, but the path is still wrong on Fedora and Arch and files restore to ~/.local/share/what-ever. Somehow on Endeavour this doesn't happen and everything works as intended.

@aalhitennf
Copy link
Author

aalhitennf commented May 12, 2022

I think i found the problem

freedesktop.rs line 428:

writeln!(file, "Path={}", path).and_then(|_| {

i dont understand the whole encoding and trimming before this (maybe it isn't needed at all?) but here when writing the info file content, instead of using path, use absolute_uri, and it seems to work properly:

writeln!(file, "Path={}", absolute_uri).and_then(|_| {

@Byron
Copy link
Owner

Byron commented May 13, 2022

Thanks for the hint and for digging in!

Let's try that via v2.1.2.

@aalhitennf
Copy link
Author

Seems to be fixed on all distros i mentioned, thanks for fast updates!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed question Further information is requested
Projects
None yet
Development

No branches or pull requests

2 participants