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 "delete before copy" logic. Add tests. #6712

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

andrewliebenow
Copy link
Contributor

Also: fix "delete before copy" verbose mode output.

Also: fix "delete before copy" verbose mode output.
@sylvestre
Copy link
Sponsor Contributor

Could you please be more explicit about the issue you are fixing ?
Fix "delete before copy" logic isn't super clear

thanks

Copy link

GNU testsuite comparison:

GNU test failed: tests/mv/update. tests/mv/update is passing on 'main'. Maybe you have to rebase?
Skipping an intermittent issue tests/rm/rm1 (passes in this run but fails in the 'main' branch)
Congrats! The gnu test tests/timeout/timeout is no longer failing!

@andrewliebenow
Copy link
Contributor Author

Sorry. The issue is kind of explained in this deleted comment:

coreutils/src/uu/cp/src/cp.rs

Lines 1775 to 1779 in b8150f5

// TODO
// Using `readonly` here to check if `dest` needs to be deleted is not correct:
// "On Unix-based platforms this checks if any of the owner, group or others write permission bits are set. It does not check if the current user is in the file's assigned group. It also does not check ACLs. Therefore the return value of this function cannot be relied upon to predict whether attempts to read or write the file will actually succeed."
// This results in some copy operations failing, because this necessary deletion is being skipped.
is_symlink_loop(dest) || fs::metadata(dest)?.permissions().readonly()

If -f is used, or -i (and the user accepts the prompt), a non-writable file (a file with the user write bit set to 0) will be deleted by cp before the copy operation begins. This check was using the readonly function from the Rust Standard Library (https://doc.rust-lang.org/std/fs/struct.Permissions.html#method.readonly), but the documentation for readonly clearly states that it returns false if any write bit is set. On "target_family" = "unix" platforms, we can use the access API to check if the current user is actually able to write to the file.

This incorrect check was causing some necessary pre-copy deletions to be skipped, which then caused the copy operation to fail due to lack of write permission. I added tests for this edge case (the ones where the destination file mode is 077, as opposed to 000 in the other "delete before copy" tests).

Copy link

GNU testsuite comparison:

GNU test failed: tests/mv/update. tests/mv/update is passing on 'main'. Maybe you have to rebase?
Skipping an intermittent issue tests/rm/rm1 (passes in this run but fails in the 'main' branch)
Congrats! The gnu test tests/timeout/timeout is no longer failing!

Copy link

GNU testsuite comparison:

Skipping an intermittent issue tests/rm/rm1 (passes in this run but fails in the 'main' branch)
Congrats! The gnu test tests/timeout/timeout is no longer failing!

Copy link

GNU testsuite comparison:

Skip an intermittent issue tests/tail/inotify-dir-recreate (fails in this run but passes in the 'main' branch)

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.

2 participants