On 04/05/18 16:37, Illia Bobyr wrote: > Hello, > > I have found a bug in "cp -rfs". > > Steps to reproduce: > > 1. Given "path1" and "path2" are on different devices. > 2. $ touch "path1/file" > 3. $ cd path2/; ln -s path1/file > 4. $ cp --symbolic-link --force --recursive path1/file . > > Expected: > The link is overwritten with an exact copy. > > Actual result: > cp shows an error: > cp: 'path1/file' and './file' are the same file > > This bug was introduced in > > http://git.savannah.gnu.org/cgit/coreutils.git/commit/?id=376967889ed7ed561e46ff6d88a66779db62737a > > Specifically this hunk: > > diff --git a/src/copy.c b/src/copy.c > index e3832c2..9dbd536 100644 > --- a/src/copy.c > <http://git.savannah.gnu.org/cgit/coreutils.git/tree/src/copy.c?id=2f69dba5df8caaf9eda658c1808b1379e9949f22> > +++ b/src/copy.c > <http://git.savannah.gnu.org/cgit/coreutils.git/tree/src/copy.c?id=376967889ed7ed561e46ff6d88a66779db62737a> > @@ -46,6 +46,7 @@ > #include "file-set.h" > #include "filemode.h" > #include "filenamecat.h" > +#include "force-link.h" > #include "full-write.h" > #include "hash.h" > #include "hash-triple.h" > @@ -1623,11 +1624,13 @@ same_file_ok (char const *src_name, struct stat > const *src_sb, > } > } > - /* It's ok to remove a destination symlink. But that works only when we > - unlink before opening the destination and when the source and destination > - files are on the same partition. */ > - if (x->unlink_dest_before_opening > - && S_ISLNK (dst_sb_link->st_mode)) > + /* It's ok to remove a destination symlink. But that works only > + when creating symbolic links, or when the source and destination > + are on the same file system and when creating hard links or when > + unlinking before opening the destination. */ > + if (x->symbolic_link > + || ((x->hard_link || x->unlink_dest_before_opening) > + && S_ISLNK (dst_sb_link->st_mode))) > return dst_sb_link->st_dev == src_sb_link->st_dev; > if (x->dereference == DEREF_NEVER) > > Two patches that fix the issue are attached. > They are against the current master in > https://github.com/coreutils/coreutils > The changes are also here: > > https://github.com/coreutils/coreutils/compare/master...ilya-bobyr:master > > Thank you, Illia Bobyr > Thanks for the careful analysis of this hairy code.
Your use case works with --remove, which is similar to the very recent https://bugs.gnu.org/31335 which also requests --force to replace symlinks. Your case is slightly different and a change from previous behavior. Note the specific reason for the change is not the hunk you mentioned, but this hunk in cp.c which used to make --force --symlink also imply --remove. /* If --force (-f) was specified and we're in link-creation mode, first remove any existing destination file. */ if (x.unlink_dest_after_failed_open && (x.hard_link || x.symbolic_link)) x.unlink_dest_before_opening = true; That was also changed in the commit you referenced, as we no longer unconditionally unlink() for atomicity reasons. I.E. we now create a temp symlink and rename it over the existing destination. If you don't require these new guarantees then --remove will work for your use case. So back to the hunk you mentioned. I think the logic in the hunk you referenced was suspect before commit 3769678 and only became significant as now hit due to --remove no longer being set unconditionally for -sf. Your analysis wrt x->symbolic_link being independent of the src and dst devices is sound. Though things aren't quite right as one can now nuke a file like: $ touch file $ ln -s file l1 $ cp -s -f l1 file That would be a regression in commit 3769678 not in your change. What I've currently have to address both these cases is: diff --git a/src/copy.c b/src/copy.c index 4998c83..806323e 100644 --- a/src/copy.c +++ b/src/copy.c @@ -1627,14 +1627,17 @@ same_file_ok (char const *src_name, struct stat const *src_sb, } } - /* It's ok to remove a destination symlink. But that works only - when creating symbolic links, or when the source and destination - are on the same file system and when creating hard links or when - unlinking before opening the destination. */ - if (x->symbolic_link - || ((x->hard_link || x->unlink_dest_before_opening) - && S_ISLNK (dst_sb_link->st_mode))) - return dst_sb_link->st_dev == src_sb_link->st_dev; + if (S_ISLNK (dst_sb_link->st_mode)) + { + /* It's ok to replace a destination symlink. */ + if (x->symbolic_link) + return true; + + /* Or when hard links are possible. + TODO: Analyze this case further. */ + if (x->hard_link) + return dst_sb_link->st_dev == src_sb_link->st_dev; + } if (x->dereference == DEREF_NEVER) { I'll test a few more cases, and add tests and NEWS. Thanks again for pinpointing these issues! Pádraig
