[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
bug#31364: cp -rfs: Fails to overwrite a symlink when it is on a differe
bug#31364: cp -rfs: Fails to overwrite a symlink when it is on a different device
Sat, 5 May 2018 20:18:32 -0700
Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.8.0
On 04/05/18 16:37, Illia Bobyr wrote:
> 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 .
> 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
> Specifically this hunk:
> diff --git a/src/copy.c b/src/copy.c
> index e3832c2..9dbd536 100644
> --- a/src/copy.c
> +++ b/src/copy.c
> @@ -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
> The changes are also here:
> 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
@@ -1627,14 +1627,17 @@ same_file_ok (char const *src_name, struct stat const
- /* 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!