Bug 174489 - cp(1): 'cp -a -n' fails when 'over-writing' a broken symlink
Summary: cp(1): 'cp -a -n' fails when 'over-writing' a broken symlink
Status: Closed FIXED
Alias: None
Product: Base System
Classification: Unclassified
Component: bin (show other bugs)
Version: 8.2-RELEASE
Hardware: Any Any
: Normal Affects Only Me
Assignee: Mark Johnston
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-12-16 23:20 UTC by Michał Górny
Modified: 2024-01-05 03:19 UTC (History)
2 users (show)

See Also:


Attachments
cp_dash_n_and_symlinks.patch (1.12 KB, patch)
2012-12-21 04:59 UTC, Mark Johnston
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Michał Górny 2012-12-16 23:20:01 UTC
If 'cp -a -n' is used to copy a tree of files containing a 'broken' symlink for the second time, the call fails with the following error:

    cp: symlink: python-exec: File exists

(with 'python-exec' being the non-existent target of the broken symlink, not the symlink name)

I would assume that the no-clobber behavior enforced by '-n' should actually ignore the symlink, broken or not.

The bug was originally reported on Gentoo Bugzilla because of an eclass not working with BSD version of 'cp' [0]. A detailed analysis of the problem has been submitted there by Mike Gilbert, citing:

> Inside of copy() in cp.c [1], the dne variable is set to 1 based on a call
> to stat(2). stat(2) attempts to follow symlinks, and returns -1 when called
> on the easy_install -> python-exec symlink because python-exec doesn't exist
> in ${D}.
> 
> !dne is then passed to copy_link in utils.c [2]. copy_link is supposed to
> remove the destination if it exists (via unlink), but the bad value in dne
> causes this to be skipped.
> 
> Since the destination file is not removed, the call to symlink then fails,
> producing the "cp: symlink: python-exec: File exists" message.
> 
> [1] http://svnweb.freebsd.org/base/head/bin/cp/cp.c?view=markup
> [2] http://svnweb.freebsd.org/base/head/bin/cp/utils.c?view=markup

[0]:https://bugs.gentoo.org/show_bug.cgi?id=447370

Fix: 

Citing Mike Gilbert:

> I think this could be fixed by changing the stat(2) to lstat(2) in cp.c.
> copy_link should probably be changed to obey the "-n" flag (nflag) as well.
How-To-Repeat: mkdir 1 2
ln -s python-exec 1/foo
cp -a -n 1/* 2/
cp -a -n 1/* 2/
Comment 1 Mark Johnston freebsd_committer freebsd_triage 2012-12-21 04:43:17 UTC
Responsible Changed
From-To: freebsd-bugs->markj

I'll take it.
Comment 2 Mark Johnston freebsd_committer freebsd_triage 2012-12-21 04:56:44 UTC
As noted, there are two bugs here:
1. The "exists" flag in copy_link() is set based on whether the target
   can be stat(2)'ed. But if -R is specified and the destination is a
   symlink, the "exists" flag ends up referring to the path that the
   target symlink is pointing to. The result is that the target symlink
   isn't unlink(2)ed before symlink(2) is called in copy_link(), so
   cp(1) errors out.
2. copy_link() doesn't respect -n. So in the following example, baz
   should be left unchanged by cp(1), but we're overwriting it:

  $ touch foo1 foo2
  $ ln -s foo1 bar
  $ ln -s foo2 baz
  $ cp -an bar baz
  $ ls -la bar bar
  lrwxr-xr-x  1 mark  mark  4 Dec 20 23:49 bar -> foo1
  lrwxr-xr-x  1 mark  mark  4 Dec 20 23:49 baz -> foo1
  $

The attached patch fixes both of these issues. I'd also like to add
some regression tests for cp(1) (we have a bunch for mv(1)), but in the
meantime, can you test the patch?

Thanks,
-Mark
Comment 3 Mark Johnston freebsd_committer freebsd_triage 2012-12-21 04:59:52 UTC
On Thu, Dec 20, 2012 at 11:56 PM, Mark Johnston <markj@freebsd.org> wrote:
...
> The attached patch fixes both of these issues. I'd also like to add
> some regression tests for cp(1) (we have a bunch for mv(1)), but in the
> meantime, can you test the patch?

And I of course forgot to actually attach it. Sorry about that.
Comment 4 Mark Johnston freebsd_committer freebsd_triage 2012-12-21 19:38:21 UTC
State Changed
From-To: open->feedback

Ask for submitter approval.
Comment 5 Jilles Tjoelker freebsd_committer freebsd_triage 2012-12-24 20:26:15 UTC
PR bin/174489:
> [overwriting a symlink with cp -an]

The patch appears to fix overwriting a symlink with a symlink, but
breaks creating a symlink's destination by copying a regular file onto
it.

Example:
:>testf1; ln -fs testf2 testl1; cp -R testf1 testl1;
ls -l testf2; rm testf1 testf2 testl1

These commands should all succeed.

I am not entirely sure but it may be appropriate to set
int follow_link = (fts_options & FTS_LOGICAL) ||
    ((fts_options & FTS_COMFOLLOW) && curr->fts_level == 0);
and call
(follow_link ? stat : lstat).

Note that follow_link will always be true if -R was not given.

The FreeBSD kernel also allows creating a symlink's destination as a
directory by passing a pathname ending in a slash (if there is no slash,
POSIX requires mkdir() to fail) but cp does not expose this
functionality.

-- 
Jilles Tjoelker
Comment 6 Mark Johnston freebsd_committer freebsd_triage 2012-12-25 17:37:04 UTC
On Mon, Dec 24, 2012 at 08:30:01PM +0000, Jilles Tjoelker wrote:
> The following reply was made to PR bin/174489; it has been noted by GNATS.
> 
> From: Jilles Tjoelker <jilles@stack.nl>
> To: bug-followup@FreeBSD.org, mgorny@gentoo.org
> Cc:  
> Subject: Re: bin/174489: cp(1): &#39;cp -a -n&#39; fails when
>  &#39;over-writing&#39; a broken symlink
> Date: Mon, 24 Dec 2012 21:26:15 +0100
> 
>  PR bin/174489:
>  > [overwriting a symlink with cp -an]
>  
>  The patch appears to fix overwriting a symlink with a symlink, but
>  breaks creating a symlink's destination by copying a regular file onto
>  it.
>  
>  Example:
>  :>testf1; ln -fs testf2 testl1; cp -R testf1 testl1;
>  ls -l testf2; rm testf1 testf2 testl1
>  
>  These commands should all succeed.

Ok. It's probably worth noting somewhere that this goes against what
POSIX says, since open("<dangling symlink>", O_WRONLY | O_TRUNC) will
fail, and cp(1) should then bail by section 3.a.iii. We need to add
O_CREAT for the above example to work properly.

>  
>  I am not entirely sure but it may be appropriate to set
>  int follow_link = (fts_options & FTS_LOGICAL) ||
>      ((fts_options & FTS_COMFOLLOW) && curr->fts_level == 0);
>  and call
>  (follow_link ? stat : lstat).
>  
>  Note that follow_link will always be true if -R was not given.

That logic doesn't quite work: if -R is given (and -L and -H are not),
then fts_options and thus follow_link will be 0, so lstat(2) will return
0 and we'll thus try to open <dst> without O_CREAT, leading to an error.

I think we just need to add a special check for a broken symlink as the
destination path. I'll try to come up with a patch shortly.

>  
>  The FreeBSD kernel also allows creating a symlink's destination as a
>  directory by passing a pathname ending in a slash (if there is no slash,
>  POSIX requires mkdir() to fail) but cp does not expose this
>  functionality.
>  
>  -- 
>  Jilles Tjoelker
Comment 7 Michał Górny 2013-01-10 14:50:18 UTC
On Thu, 20 Dec 2012 23:56:44 -0500
Mark Johnston <markj@freebsd.org> wrote:

> As noted, there are two bugs here:
> 1. The "exists" flag in copy_link() is set based on whether the target
>    can be stat(2)'ed. But if -R is specified and the destination is a
>    symlink, the "exists" flag ends up referring to the path that the
>    target symlink is pointing to. The result is that the target symlink
>    isn't unlink(2)ed before symlink(2) is called in copy_link(), so
>    cp(1) errors out.
> 2. copy_link() doesn't respect -n. So in the following example, baz
>    should be left unchanged by cp(1), but we're overwriting it:
> 
>   $ touch foo1 foo2
>   $ ln -s foo1 bar
>   $ ln -s foo2 baz
>   $ cp -an bar baz
>   $ ls -la bar bar
>   lrwxr-xr-x  1 mark  mark  4 Dec 20 23:49 bar -> foo1
>   lrwxr-xr-x  1 mark  mark  4 Dec 20 23:49 baz -> foo1
>   $
> 
> The attached patch fixes both of these issues. I'd also like to add
> some regression tests for cp(1) (we have a bunch for mv(1)), but in the
> meantime, can you test the patch?


I've finally got the OP to reply on the patch [1]. Although it
seems to fix the original issue, it seems to break '-v' option:

$ cp -a -l -v -n 1/* 3/
$ cp -a -l -v -n 1/* 3/
3/easy_install not overwritten

(note that first 'cp' invocation didn't print copied files)

$ cp -a -l -v 1/* 3/
1/easy_install -> 3/easy_install

[1]:https://bugs.gentoo.org/show_bug.cgi?id=447370

-- 
Best regards,
MichaŠGórny
Comment 8 Mark Johnston freebsd_committer freebsd_triage 2013-01-23 18:17:19 UTC
State Changed
From-To: feedback->analyzed

The reported issues are fixed with the attached patch.
Comment 9 Mark Johnston freebsd_committer freebsd_triage 2013-01-23 19:15:53 UTC
On Thu, Jan 10, 2013 at 03:50:18PM +0100, MichaŠGórny wrote:
> On Thu, 20 Dec 2012 23:56:44 -0500
> Mark Johnston <markj@freebsd.org> wrote:
> 
> > As noted, there are two bugs here:
> > 1. The "exists" flag in copy_link() is set based on whether the target
> >    can be stat(2)'ed. But if -R is specified and the destination is a
> >    symlink, the "exists" flag ends up referring to the path that the
> >    target symlink is pointing to. The result is that the target symlink
> >    isn't unlink(2)ed before symlink(2) is called in copy_link(), so
> >    cp(1) errors out.
> > 2. copy_link() doesn't respect -n. So in the following example, baz
> >    should be left unchanged by cp(1), but we're overwriting it:
> > 
> >   $ touch foo1 foo2
> >   $ ln -s foo1 bar
> >   $ ln -s foo2 baz
> >   $ cp -an bar baz
> >   $ ls -la bar bar
> >   lrwxr-xr-x  1 mark  mark  4 Dec 20 23:49 bar -> foo1
> >   lrwxr-xr-x  1 mark  mark  4 Dec 20 23:49 baz -> foo1
> >   $
> > 
> > The attached patch fixes both of these issues. I'd also like to add
> > some regression tests for cp(1) (we have a bunch for mv(1)), but in the
> > meantime, can you test the patch?
> 
> I've finally got the OP to reply on the patch [1]. Although it
> seems to fix the original issue, it seems to break '-v' option:
> 
> $ cp -a -l -v -n 1/* 3/
> $ cp -a -l -v -n 1/* 3/
> 3/easy_install not overwritten
> 
> (note that first 'cp' invocation didn't print copied files)
> 
> $ cp -a -l -v 1/* 3/
> 1/easy_install -> 3/easy_install

Ok, thanks for the feedback. My current patches for this problem are
here: http://people.freebsd.org/~markj/patches/cp/
Specifically, the first two patches resolve the original issues reported
in this PR (the last is for another bug). I haven't looked into the
problem with the -v option yet, but I'll do that now.

Thanks,
-Mark
Comment 10 dfilter service freebsd_committer freebsd_triage 2013-01-27 05:59:36 UTC
Author: markj
Date: Sun Jan 27 05:59:28 2013
New Revision: 245960
URL: http://svnweb.freebsd.org/changeset/base/245960

Log:
  Return with an error from copy_link(), copy_fifo() and copy_special() if
  the -n option is specified and the destination file exists.
  
  PR:		bin/174489
  Approved by:	rstone (co-mentor)
  MFC after:	2 weeks

Modified:
  head/bin/cp/utils.c

Modified: head/bin/cp/utils.c
==============================================================================
--- head/bin/cp/utils.c	Sun Jan 27 05:45:55 2013	(r245959)
+++ head/bin/cp/utils.c	Sun Jan 27 05:59:28 2013	(r245960)
@@ -266,6 +266,11 @@ copy_link(const FTSENT *p, int exists)
 	int len;
 	char llink[PATH_MAX];
 
+	if (exists && nflag) {
+		if (vflag)
+			printf("%s not overwritten\n", to.p_path);
+		return (1);
+	}
 	if ((len = readlink(p->fts_path, llink, sizeof(llink) - 1)) == -1) {
 		warn("readlink: %s", p->fts_path);
 		return (1);
@@ -285,6 +290,12 @@ copy_link(const FTSENT *p, int exists)
 int
 copy_fifo(struct stat *from_stat, int exists)
 {
+
+	if (exists && nflag) {
+		if (vflag)
+			printf("%s not overwritten\n", to.p_path);
+		return (1);
+	}
 	if (exists && unlink(to.p_path)) {
 		warn("unlink: %s", to.p_path);
 		return (1);
@@ -299,6 +310,12 @@ copy_fifo(struct stat *from_stat, int ex
 int
 copy_special(struct stat *from_stat, int exists)
 {
+
+	if (exists && nflag) {
+		if (vflag)
+			printf("%s not overwritten\n", to.p_path);
+		return (1);
+	}
 	if (exists && unlink(to.p_path)) {
 		warn("unlink: %s", to.p_path);
 		return (1);
_______________________________________________
svn-src-all@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscribe@freebsd.org"
Comment 11 Eitan Adler freebsd_committer freebsd_triage 2018-05-28 19:41:24 UTC
batch change:

For bugs that match the following
-  Status Is In progress 
AND
- Untouched since 2018-01-01.
AND
- Affects Base System OR Documentation

DO:

Reset to open status.


Note:
I did a quick pass but if you are getting this email it might be worthwhile to double check to see if this bug ought to be closed.
Comment 12 Oleksandr Tymoshenko freebsd_committer freebsd_triage 2019-01-20 03:25:13 UTC
Mark, can this issue be closed now?

Thanks
Comment 13 Mark Linimon freebsd_committer freebsd_triage 2024-01-05 03:19:42 UTC
^Triage: feedback timeout (several years).