/bin/cp: overwrite symlink with file pointed to by symlink

[Christopher Zimmermann-2]

Hi,

I admit this is a very special case, but anyway this is what I hit:

$ touch blub; ln -s blub blab; ls -l; cp -f blub blab
total 0
lrwxr-xr-x  1 madroach  wheel  4 Oct 30 17:39 blab -> blub
-rw-r--r--  1 madroach  wheel  0 Oct 30 17:39 blub
cp: blab and blub are identical (not copied).

Now I had a look at our cp.c, our cp(1) and at POSIX:

 -f For each existing destination pathname, remove it and create a
        new file, without prompting for confirmation, regardless of its
        permissions.  The -f option overrides any previous -i options.

POSIX says cp must always first try to open(2)(O_TRUNC) the destination
and only if this fails AND -f was specified should it try unlink(2).

At least regarding this special case our cp behaves according to
POSIX, not according to our manpage.

As long as we cannot change the POSIX standard I don't know whether our
manpage should mention this or cp.c be changed to use lstat(), too.
Maybe this is such a special case we could just ignore it...


Christopher


--
http://gmerlin.de
OpenPGP: http://gmerlin.de/christopher.pub
CB07 DA40 B0B6 571D 35E2  0DEF 87E2 92A7 13E5 DEE1

[Quentin Rameau-2]

> Hi,

Hello Christopher,

> I admit this is a very special case, but anyway this is what I hit:
>
> $ touch blub; ln -s blub blab; ls -l; cp -f blub blab
> total 0
> lrwxr-xr-x  1 madroach  wheel  4 Oct 30 17:39 blab -> blub
> -rw-r--r--  1 madroach  wheel  0 Oct 30 17:39 blub
> cp: blab and blub are identical (not copied).

This is expected behaviour, blab and blub are indeed the same file and
so copy should be skipped.

From POSIX:

“If source_file references the same file as dest_file, cp may write a
diagnostic message to standard error; it shall do nothing more with
source_file and shall go on to any remaining files.”

> Now I had a look at our cp.c, our cp(1) and at POSIX:
>
>  -f For each existing destination pathname, remove it and
> create a new file, without prompting for confirmation, regardless of
> its permissions.  The -f option overrides any previous -i options.
>
> POSIX says cp must always first try to open(2)(O_TRUNC) the
> destination and only if this fails AND -f was specified should it try
> unlink(2).
>
> At least regarding this special case our cp behaves according to
> POSIX, not according to our manpage.
>
> As long as we cannot change the POSIX standard I don't know whether
> our manpage should mention this or cp.c be changed to use lstat(),
> too. Maybe this is such a special case we could just ignore it...

If you're interested in adapting to POSIX, here's a proposition patch:

Index: bin/cp//cp.1
===================================================================
RCS file: /var/cvs/src/bin/cp/cp.1,v
retrieving revision 1.40
diff -u -r1.40 cp.1
--- bin/cp//cp.1 28 Jan 2019 18:58:42 -0000 1.40
+++ bin/cp//cp.1 1 Nov 2019 19:41:26 -0000
@@ -79,9 +79,8 @@
 Same as
 .Fl RpP .
 .It Fl f
-For each existing destination pathname, remove it and
-create a new file, without prompting for confirmation,
-regardless of its permissions.
+For each existing destination pathname which cannot be opened, remove
+it and create a new file, without prompting for confirmation.
 The
 .Fl f
 option overrides any previous
Index: bin/cp//utils.c
===================================================================
RCS file: /var/cvs/src/bin/cp/utils.c,v
retrieving revision 1.47
diff -u -r1.47 utils.c
--- bin/cp//utils.c 28 Jan 2019 18:58:42 -0000 1.47
+++ bin/cp//utils.c 1 Nov 2019 19:25:02 -0000
@@ -55,7 +55,7 @@
  static char *buf;
  static char *zeroes;
  struct stat to_stat, *fs;
- int from_fd, rcount, rval, to_fd, wcount;
+ int from_fd, rcount, rval, to_fd, wcount, create = 1;
 #ifdef VM_AND_BUFFER_CACHE_SYNCHRONIZED
  char *p;
 #endif
@@ -79,26 +79,24 @@
  fs = entp->fts_statp;
 
  /*
- * In -f (force) mode, we always unlink the destination first
- * if it exists.  Note that -i and -f are mutually exclusive.
- */
- if (exists && fflag)
- (void)unlink(to.p_path);
-
- /*
  * If the file DNE, set the mode to be the from file, minus setuid
  * bits, modified by the umask; arguably wrong, but it makes copying
  * executables work right and it's been that way forever.  (The
  * other choice is 666 or'ed with the execute bits on the from file
  * modified by the umask.)
  */
- if (exists && !fflag) {
- if (!copy_overwrite()) {
+ if (exists) {
+ /* Note that -i and -f are mutually exclusive. */
+ if (!fflag && !copy_overwrite()) {
  (void)close(from_fd);
  return 2;
  }
  to_fd = open(to.p_path, O_WRONLY | O_TRUNC, 0);
- } else
+ if (to_fd != -1 || fflag && unlink(to.p_path) == -1)
+ create = 0;
+ }
+
+ if (create)
  to_fd = open(to.p_path, O_WRONLY | O_TRUNC | O_CREAT,
     fs->st_mode & ~(S_ISTXT | S_ISUID | S_ISGID));
 

[Quentin Rameau-2]

Hello,

> > Now I had a look at our cp.c, our cp(1) and at POSIX:
> >
> >  -f For each existing destination pathname, remove it and
> > create a new file, without prompting for confirmation, regardless of
> > its permissions.  The -f option overrides any previous -i options.
> >
> > POSIX says cp must always first try to open(2)(O_TRUNC) the
> > destination and only if this fails AND -f was specified should it try
> > unlink(2).
> >
> > At least regarding this special case our cp behaves according to
> > POSIX, not according to our manpage.
> >
> > As long as we cannot change the POSIX standard I don't know whether
> > our manpage should mention this or cp.c be changed to use lstat(),
> > too. Maybe this is such a special case we could just ignore it...
>
> If you're interested in adapting to POSIX, here's a proposition patch:

Any comment on making cp more “POSIXy” there?

[Christopher Zimmermann-2]

Hi Quentin,

Thank you for having a look.
I know I'm very late, but I still like your patch. So ok chrisz@ if you
want to commit it.

Christopher


On Fri, Nov 01, 2019 at 08:46:51PM +0100, Quentin Rameau wrote:

>If you're interested in adapting to POSIX, here's a proposition patch:
>
>Index: bin/cp//cp.1
>===================================================================
>RCS file: /var/cvs/src/bin/cp/cp.1,v
>retrieving revision 1.40
>diff -u -r1.40 cp.1
>--- bin/cp//cp.1 28 Jan 2019 18:58:42 -0000 1.40
>+++ bin/cp//cp.1 1 Nov 2019 19:41:26 -0000
>@@ -79,9 +79,8 @@
> Same as
> .Fl RpP .
> .It Fl f
>-For each existing destination pathname, remove it and
>-create a new file, without prompting for confirmation,
>-regardless of its permissions.
>+For each existing destination pathname which cannot be opened, remove
>+it and create a new file, without prompting for confirmation.
> The
> .Fl f
> option overrides any previous
>Index: bin/cp//utils.c
>===================================================================
>RCS file: /var/cvs/src/bin/cp/utils.c,v
>retrieving revision 1.47
>diff -u -r1.47 utils.c
>--- bin/cp//utils.c 28 Jan 2019 18:58:42 -0000 1.47
>+++ bin/cp//utils.c 1 Nov 2019 19:25:02 -0000
>@@ -55,7 +55,7 @@
> static char *buf;
> static char *zeroes;
> struct stat to_stat, *fs;
>- int from_fd, rcount, rval, to_fd, wcount;
>+ int from_fd, rcount, rval, to_fd, wcount, create = 1;
> #ifdef VM_AND_BUFFER_CACHE_SYNCHRONIZED
> char *p;
> #endif
>@@ -79,26 +79,24 @@
> fs = entp->fts_statp;
>
> /*
>- * In -f (force) mode, we always unlink the destination first
>- * if it exists.  Note that -i and -f are mutually exclusive.
>- */
>- if (exists && fflag)
>- (void)unlink(to.p_path);
>-
>- /*
> * If the file DNE, set the mode to be the from file, minus setuid
> * bits, modified by the umask; arguably wrong, but it makes copying
> * executables work right and it's been that way forever.  (The
> * other choice is 666 or'ed with the execute bits on the from file
> * modified by the umask.)
> */
>- if (exists && !fflag) {
>- if (!copy_overwrite()) {
>+ if (exists) {
>+ /* Note that -i and -f are mutually exclusive. */
>+ if (!fflag && !copy_overwrite()) {
> (void)close(from_fd);
> return 2;
>   }
> to_fd = open(to.p_path, O_WRONLY | O_TRUNC, 0);
>- } else
>+ if (to_fd != -1 || fflag && unlink(to.p_path) == -1)
>+ create = 0;
>+ }
>+
>+ if (create)
> to_fd = open(to.p_path, O_WRONLY | O_TRUNC | O_CREAT,
>    fs->st_mode & ~(S_ISTXT | S_ISUID | S_ISGID));
>
>

--
http://gmerlin.de
OpenPGP: http://gmerlin.de/christopher.pub
CB07 DA40 B0B6 571D 35E2  0DEF 87E2 92A7 13E5 DEE1

[Quentin Rameau-2]

> Hi Quentin,

Hi Cristopher,

> Thank you for having a look.

You're welcome!

> I know I'm very late, but I still like your patch. So ok chrisz@ if you
> want to commit it.

It didn't raise much passion at the time, we can just hope it gets a bit
more attention now!