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

Fix symlink bugs #70

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
Open

Fix symlink bugs #70

wants to merge 11 commits into from

Conversation

GPlaczek
Copy link

@GPlaczek GPlaczek commented Mar 30, 2023

The problem

When user attempts to move a directory that contains symbolic links, it might result in an error without a proper cleanup.

How to reproduce

Sample code that produces the issue:

use fs_extra::dir::{CopyOptions, move_dir};

fn main() {
    println!("{:?}", move_dir("dir", "dir1", &CopyOptions {
        copy_inside: true,
        ..CopyOptions::new()}));

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

Sample input:

$ tree dir
dir
├── dir1
│   └── link1 -> ../file
├── file
└── link -> file

What happens

Running the above program against the given input leaves dir and dir1 directories in the following state:

$ tree dir1 dir
dir1
├── dir1
│   └── link1
└── file
dir
├── dir1
└── link -> file

What happens here is the file is copied before the link and the exists function returns false because it follows symbolic links (docs reference). After that, the function fails without cleaning up half-moved directory. Also, link1 after the copy holds the same data as file (which it points to) instead of being a symbolic link. I think this is not an expected behavior as well.

My fix

I fixed it by adding a follow field to file::CopyOptions (which defaults to true) and set it to false when invoked from move and copy functions from dir module. Setting this field prevents following symbolic links when moving or copying. Additionally, I fixed another small bug - setting overwrite to false in file::CopyOptions would still overwrite dangling symlinks, now checks are performed to prevent this.

My solution breaks backward compatibility, if you don't want to introduce such changes to your repository now, let me know and I can try to adjust it to be backwards compatible.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant