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/
Responsible Changed From-To: freebsd-bugs->markj I'll take it.
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
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.
State Changed From-To: open->feedback Ask for submitter approval.
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
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): 'cp -a -n' fails when > 'over-writing' 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
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
State Changed From-To: feedback->analyzed The reported issues are fixed with the attached patch.
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
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"
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.
Mark, can this issue be closed now? Thanks
^Triage: feedback timeout (several years).