GNU bug report logs - #31364
cp -rfs: Fails to overwrite a symlink when it is on a different device

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 31364 in the body.
You can then email your comments to 31364 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Illia Bobyr <ibobyr <at> google.com>
To: bug-coreutils <at> gnu.org
Cc: Jason Smith <jasonmsmith <at> google.com>
Subject: cp -rfs: Fails to overwrite a symlink when it is on a different device
Date: Fri, 04 May 2018 23:37:55 +0000

[Message part 1 (text/plain, inline)]

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

[Message part 2 (text/html, inline)]

[0002-cp-Overwrite-symlinks-on-another-device.patch (text/x-patch, attachment)]

[0001-cp-No-dup-check-for-unlink_dest_after_failed_open.patch (text/x-patch, attachment)]

Message #8 received at 31364 <at> debbugs.gnu.org (full text, mbox):

From: Pádraig Brady <P <at> draigBrady.com>
To: Illia Bobyr <ibobyr <at> google.com>, 31364 <at> debbugs.gnu.org
Cc: Jason Smith <jasonmsmith <at> google.com>
Subject: Re: bug#31364: cp -rfs: Fails to overwrite a symlink when it is on a
 different device
Date: Sat, 5 May 2018 20:18:32 -0700

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

Message #11 received at 31364 <at> debbugs.gnu.org (full text, mbox):

From: Jason Smith <jasonmsmith <at> google.com>
To: P <at> draigbrady.com
Cc: Illia Bobyr <ibobyr <at> google.com>, 31364 <at> debbugs.gnu.org
Subject: Re: bug#31364: cp -rfs: Fails to overwrite a symlink when it is on a
 different device
Date: Mon, 07 May 2018 01:58:38 +0000

[Message part 1 (text/plain, inline)]

Hello, Pádraig,

I was actually the one to identify the bug and analyze the code underlying
it (Illia, my colleague, was kind enough to volunteer to file the bug
report and attempt a patch), and so I may be in the better position to
follow up on this. I appreciate your addressing the matter so promptly.

We are indeed provisionally using --remove-destination; however, we would
prefer to use --force instead for the atomicity it affords.

Regarding the proposed solution, I think that it well addresses the present
issue. But there are related issues that I should raise while we are
discussing it and that may influence its resolution.

To begin, it seems problematic to make the return value in the case of
hard-link creation dependent on whether the source and the destination are
associated with the same device ID, as the purpose of the same_file_ok
function seems to be to indicate whether it matters that the files may
somehow be the same. If their file systems are different, that is not a
problem with the files' being the same; it is a problem with their being on
different file systems. The resulting error message (that the operation
cannot be performed because the files are the same) is therefore incorrect
and misleading. Perhaps, as it seems to me, this check should rather be
performed outside this function and associated with its own error message.

In fact, even if the source and destination are completely unrelated, this
function may still return false and cause the same-file error message to be
displayed. For example, let [source] be a regular file or directory on one
file system, and let [destination] be a symbolic link on another. Then the
following command will result in a same-file error, even if [destination]
does not refer to [source]:

cp --recursive --link --no-dereference [source] [destination]

Moreover, the name and description of the function seem misleading. One
would think from reading these that the source and destination files passed
to it have already been determined to be equivalent in some way, and that
the function is meant to determine whether it is all right in that case to
overwrite the destination; however, it appears that the function is called
whenever the destination already exists, regardless of its relation to the
source. This discrepancy can lead to misunderstandings and problems such as
the kind last described. Ideally, the function's name and description would
be made more accurate and precise, and the function used strictly
accordingly.

Assuming we were to move the file-system check outside the same_file_ok
function, we would be left with the following code within the function:

/* It's ok to remove a destination symbolic link when creating a symbolic
link or hard link. */
if (S_ISLNK (dst_sb_link->st_mode) && (x->symbolic_link || x->hard_link))
  return true;

But as we earlier in the function handle the cases when both the source and
the destination are symbolic links, and there does not seem to be a reason
to constrain overwriting of symbolic links otherwise (at least within this
function), we can perhaps simplify this even further to the following:

/* At this point, it's ok to remove a destination symbolic link. */
if (S_ISLNK (dst_sb_link->st_mode))
  return true;

But I have not analyzed all the relevant code to determine the full impact
of this change.

Finally, it would seem to make sense for the same_file_ok function to
return true immediately if the source and destination files passed to it
have different inode numbers or are on different file systems (and are thus
not the same file). In fact, this is done in lines 1488-89 if DEREF_NEVER
is not set; however, it is not done if DEREF_NEVER is set. For reference,
those lines read as follows:

if (!same)
  return true;

Cursory analysis suggests that if these lines were unconditionally executed
near the top of the function, certain problems might be avoided.

Cheers,
Jason

On Sat, May 5, 2018 at 8:18 PM Pádraig Brady <P <at> draigbrady.com> wrote:

> 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
>

[Message part 2 (text/html, inline)]

Message #14 received at 31364 <at> debbugs.gnu.org (full text, mbox):

From: Pádraig Brady <P <at> draigBrady.com>
To: Jason Smith <jasonmsmith <at> google.com>
Cc: Illia Bobyr <ibobyr <at> google.com>, 31364 <at> debbugs.gnu.org
Subject: Re: bug#31364: cp -rfs: Fails to overwrite a symlink when it is on a
 different device
Date: Mon, 14 May 2018 03:10:36 -0700

[Message part 1 (text/plain, inline)]

On 06/05/18 18:58, Jason Smith wrote:
> Hello, Pádraig,
> 
> I was actually the one to identify the bug and analyze the code underlying it (Illia, my colleague, was kind enough to volunteer to file the bug report and attempt a patch), and so I may be in the better position to follow up on this. I appreciate your addressing the matter so promptly.
> 
> We are indeed provisionally using --remove-destination; however, we would prefer to use --force instead for the atomicity it affords.
> 
> Regarding the proposed solution, I think that it well addresses the present issue. But there are related issues that I should raise while we are discussing it and that may influence its resolution.
> 
> To begin, it seems problematic to make the return value in the case of hard-link creation dependent on whether the source and the destination are associated with the same device ID, as the purpose of the same_file_ok function seems to be to indicate whether it matters that the files may somehow be the same. If their file systems are different, that is not a problem with the files' being the same; it is a problem with their being on different file systems. The resulting error message (that the operation cannot be performed because the files are the same) is therefore incorrect and misleading. Perhaps, as it seems to me, this check should rather be performed outside this function and associated with its own error message.
> 
> In fact, even if the source and destination are completely unrelated, this function may still return false and cause the same-file error message to be displayed. For example, let [source] be a regular file or directory on one file system, and let [destination] be a symbolic link on another. Then the following command will result in a same-file error, even if [destination] does not refer to [source]:
> 
> cp --recursive --link --no-dereference [source] [destination]
> 
> Moreover, the name and description of the function seem misleading. One would think from reading these that the source and destination files passed to it have already been determined to be equivalent in some way, and that the function is meant to determine whether it is all right in that case to overwrite the destination; however, it appears that the function is called whenever the destination already exists, regardless of its relation to the source. This discrepancy can lead to misunderstandings and problems such as the kind last described. Ideally, the function's name and description would be made more accurate and precise, and the function used strictly accordingly.
> 
> Assuming we were to move the file-system check outside the same_file_ok function, we would be left with the following code within the function:
> 
> /* It's ok to remove a destination symbolic link when creating a symbolic link or hard link. */
> if (S_ISLNK (dst_sb_link->st_mode) && (x->symbolic_link || x->hard_link))
>   return true;
> 
> But as we earlier in the function handle the cases when both the source and the destination are symbolic links, and there does not seem to be a reason to constrain overwriting of symbolic links otherwise (at least within this function), we can perhaps simplify this even further to the following:
> 
> /* At this point, it's ok to remove a destination symbolic link. */
> if (S_ISLNK (dst_sb_link->st_mode))
>   return true;
> 
> But I have not analyzed all the relevant code to determine the full impact of this change.
> 
> Finally, it would seem to make sense for the same_file_ok function to return true immediately if the source and destination files passed to it have different inode numbers or are on different file systems (and are thus not the same file). In fact, this is done in lines 1488-89 if DEREF_NEVER is not set; however, it is not done if DEREF_NEVER is set. For reference, those lines read as follows:
> 
> if (!same)
>   return true;
> 
> Cursory analysis suggests that if these lines were unconditionally executed near the top of the function, certain problems might be avoided.
> 
> Cheers,
> Jason


same_file_ok() has accreted awkward complexity,
having started out with a series of FIXMEs.
It gained support for `mv hardlink1 hardlink2` in fc6073d6
and then having that removed in 222d7ac0.

As part of the change in fc6073d6 (2003),
it actually made the problematic clause we've
been analyzing in same_file_ok() a noop.
Thus that clause stagnated for 14 years and so when
being converted as part of the recent 37696788 change
it no longer became a noop.

So while I agree this whole area definitely needs a refactor,
let's address the specific issues for now,
so that distributors can fix the issue with minimal risk.

Since this clause was a noop for 14 years,
we should discount it.  However the recent change
also tries to handle this case:

  touch foo
  ln -s foo symlink
  cp -dl foo symlink

Up until the recent 37696788, cp did nothing
but after it errors out saying 'symlink' exists.
This could be seen as correct as -f is not specified.
Now it could also be argued that a noop is ok
as the symlink points to the right place,
and on some systems we simulate hardlinks to symlinks
with symlinks. But on the otherhand `ln foo symlink`
does give EEXIST, so for consistency it would be
good to keep this behavior of 37696788.

The attached patch hopefully handles everything,
and adds tests for the two problematic cases.

thanks again,
Pádraig.

[cp-s-fixes.patch (text/x-patch, attachment)]

Message #19 received at 31364-done <at> debbugs.gnu.org (full text, mbox):

From: Pádraig Brady <P <at> draigBrady.com>
To: Jason Smith <jasonmsmith <at> google.com>
Cc: Illia Bobyr <ibobyr <at> google.com>, 31364-done <at> debbugs.gnu.org
Subject: Re: bug#31364: cp -rfs: Fails to overwrite a symlink when it is on a
 different device
Date: Tue, 15 May 2018 10:01:20 -0700

Pushed at https://git.sv.gnu.org/gitweb/?p=coreutils.git;a=commitdiff;h=v8.29-36-gd3daa95

marking this as done

cheers,
Pádraig.

Message #22 received at 31364-done <at> debbugs.gnu.org (full text, mbox):

From: Jason Smith <jasonmsmith <at> google.com>
To: Pádraig Brady <P <at> draigbrady.com>
Cc: Illia Bobyr <ibobyr <at> google.com>, 31364-done <at> debbugs.gnu.org
Subject: Re: bug#31364: cp -rfs: Fails to overwrite a symlink when it is on a
 different device
Date: Tue, 15 May 2018 10:52:31 -0700

[Message part 1 (text/plain, inline)]

Many thanks, Pádraig. –Jason

On Tue, May 15, 2018 at 10:01 AM Pádraig Brady <P <at> draigbrady.com> wrote:

> Pushed at
> https://git.sv.gnu.org/gitweb/?p=coreutils.git;a=commitdiff;h=v8.29-36-gd3daa95
>
> marking this as done
>
> cheers,
> Pádraig.
>
>

[Message part 2 (text/html, inline)]

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.