2 files changed, 125 insertions, 88 deletions

diff --git a/libbb/copy_file.c b/libbb/copy_file.c
index b68a257b5..3da8a3531 100644
--- a/libbb/copy_file.c
+++ b/libbb/copy_file.c
@@ -19,28 +19,31 @@
 // (or fail, if it points to dir/nonexistent location/etc).
 // This is strange, but POSIX-correct.
 // coreutils cp has --remove-destination to override this...
+//
+// NB: we have special code which still allows for "cp file /dev/node"
+// to work POSIX-ly (the only realistic case where it makes sense)
 
 #define DO_POSIX_CP 0  /* 1 - POSIX behavior, 0 - safe behavior */
 
-
+// errno must be set to relevant value ("why we cannot create dest?")
+// for POSIX mode to give reasonable error message
 static int ask_and_unlink(const char *dest, int flags)
 {
-        // If !DO_POSIX_CP, act as if -f is always in effect - we don't want
-        // "'file' exists" msg, we want unlink to be done (silently unless -i
-        // is also in effect).
-        // This prevents safe way from asking more questions than POSIX does.
 #if DO_POSIX_CP
         if (!(flags & (FILEUTILS_FORCE|FILEUTILS_INTERACTIVE))) {
-                fprintf(stderr, "'%s' exists\n", dest);
+                // Either it exists, or the *path* doesnt exist
+                bb_perror_msg("cannot create '%s'", dest);
                 return -1;
         }
 #endif
+        // If !DO_POSIX_CP, act as if -f is always in effect - we don't want
+        // "cannot create" msg, we want unlink to be done (silently unless -i).
 
         // TODO: maybe we should do it only if ctty is present?
         if (flags & FILEUTILS_INTERACTIVE) {
                 // We would not do POSIX insanity. -i asks,
                 // then _unlinks_ the offender. Presto.
-                // (No opening without O_EXCL, no unlinks only if -f)
+                // (No "opening without O_EXCL", no "unlink only if -f")
                 // Or else we will end up having 3 open()s!
                 fprintf(stderr, "%s: overwrite '%s'? ", applet_name, dest);
                 if (!bb_ask_confirmation())
@@ -60,9 +63,11 @@ static int ask_and_unlink(const char *dest, int flags)
  */
 int copy_file(const char *source, const char *dest, int flags)
 {
+        /* This is a recursive function, try to minimize stack usage */
+        /* NB: each struct stat is ~100 bytes */
         struct stat source_stat;
         struct stat dest_stat;
-        int status = 0;
+        signed char retval = 0;
         signed char dest_exists = 0;
         signed char ovr;
 
@@ -112,6 +117,7 @@ int copy_file(const char *source, const char *dest, int flags)
 
         if (S_ISDIR(source_stat.st_mode)) {
                 DIR *dp;
+                const char *tp;
                 struct dirent *d;
                 mode_t saved_umask = 0;
 
@@ -120,12 +126,23 @@ int copy_file(const char *source, const char *dest, int flags)
                         return -1;
                 }
 
-                /* Create DEST.  */
+                /* Did we ever create source ourself before? */
+                tp = is_in_ino_dev_hashtable(&source_stat);
+                if (tp) {
+                        /* We did! it's a recursion! man the lifeboats... */
+                        bb_error_msg("recursion detected, omitting directory '%s'",
+                                        source);
+                        return -1;
+                }
+
+                /* Create DEST */
                 if (dest_exists) {
                         if (!S_ISDIR(dest_stat.st_mode)) {
                                 bb_error_msg("target '%s' is not a directory", dest);
                                 return -1;
                         }
+                        /* race here: user can substitute a symlink between
+                         * this check and actual creation of files inside dest */
                 } else {
                         mode_t mode;
                         saved_umask = umask(0);
@@ -133,22 +150,29 @@ int copy_file(const char *source, const char *dest, int flags)
                         mode = source_stat.st_mode;
                         if (!(flags & FILEUTILS_PRESERVE_STATUS))
                                 mode = source_stat.st_mode & ~saved_umask;
+                        /* Allow owner to access new dir (at least for now) */
                         mode |= S_IRWXU;
-
                         if (mkdir(dest, mode) < 0) {
                                 umask(saved_umask);
                                 bb_perror_msg("cannot create directory '%s'", dest);
                                 return -1;
                         }
-
                         umask(saved_umask);
+                        /* need stat info for add_to_ino_dev_hashtable */
+                        if (lstat(dest, &dest_stat) < 0) {
+                                bb_perror_msg("cannot stat '%s'", dest);
+                                return -1;
+                        }
                 }
+                /* remember (dev,inode) of each created dir.
+                 * NULL: name is not remembered */
+                add_to_ino_dev_hashtable(&dest_stat, NULL);
 
-                /* Recursively copy files in SOURCE.  */
+                /* Recursively copy files in SOURCE */
                 dp = opendir(source);
                 if (dp == NULL) {
-                        status = -1;
-                        goto preserve_status;
+                        retval = -1;
+                        goto preserve_mode_ugid_time;
                 }
 
                 while ((d = readdir(dp)) != NULL) {
@@ -159,7 +183,7 @@ int copy_file(const char *source, const char *dest, int flags)
                                 continue;
                         new_dest = concat_path_file(dest, d->d_name);
                         if (copy_file(new_source, new_dest, flags) < 0)
-                                status = -1;
+                                retval = -1;
                         free(new_source);
                         free(new_dest);
                 }
@@ -168,11 +192,13 @@ int copy_file(const char *source, const char *dest, int flags)
                 if (!dest_exists
                  && chmod(dest, source_stat.st_mode & ~saved_umask) < 0
                 ) {
-                        bb_perror_msg("cannot change permissions of '%s'", dest);
-                        status = -1;
+                        bb_perror_msg("cannot preserve %s of '%s'", "permissions", dest);
+                        /* retval = -1; - WRONG! copy *WAS* made */
                 }
+                goto preserve_mode_ugid_time;
+        }
 
-        } else if (flags & (FILEUTILS_MAKE_SOFTLINK|FILEUTILS_MAKE_HARDLINK)) {
+        if (flags & (FILEUTILS_MAKE_SOFTLINK|FILEUTILS_MAKE_HARDLINK)) {
                 int (*lf)(const char *oldpath, const char *newpath);
  make_links:
                 // Hmm... maybe
@@ -188,47 +214,52 @@ int copy_file(const char *source, const char *dest, int flags)
                                 return -1;
                         }
                 }
+                /* _Not_ jumping to preserve_mode_ugid_time:
+                 * hard/softlinks don't have those */
                 return 0;
+        }
 
-        } else if (S_ISREG(source_stat.st_mode)
-         /* Huh? DEREF uses stat, which never returns links! */
+        if (S_ISREG(source_stat.st_mode)
+         /* DEREF uses stat, which never returns S_ISLNK() == true. */
          /* || (FLAGS_DEREF && S_ISLNK(source_stat.st_mode)) */
         ) {
                 int src_fd;
                 int dst_fd;
-                if (ENABLE_FEATURE_PRESERVE_HARDLINKS) {
-                        char *link_target;
 
-                        if (!FLAGS_DEREF) {
-                                link_target = is_in_ino_dev_hashtable(&source_stat);
-                                if (link_target) {
+                if (ENABLE_FEATURE_PRESERVE_HARDLINKS && !FLAGS_DEREF) {
+                        const char *link_target;
+                        link_target = is_in_ino_dev_hashtable(&source_stat);
+                        if (link_target) {
+                                if (link(link_target, dest) < 0) {
+                                        ovr = ask_and_unlink(dest, flags);
+                                        if (ovr <= 0)
+                                                return ovr;
                                         if (link(link_target, dest) < 0) {
-                                                ovr = ask_and_unlink(dest, flags);
-                                                if (ovr <= 0)
-                                                        return ovr;
-                                                if (link(link_target, dest) < 0) {
-                                                        bb_perror_msg("cannot create link '%s'", dest);
-                                                        return -1;
-                                                }
+                                                bb_perror_msg("cannot create link '%s'", dest);
+                                                return -1;
                                         }
-                                        return 0;
                                 }
+                                return 0;
                         }
                         add_to_ino_dev_hashtable(&source_stat, dest);
                 }
 
                 src_fd = open_or_warn(source, O_RDONLY);
-                if (src_fd < 0) {
+                if (src_fd < 0)
                         return -1;
-                }
 
-#if DO_POSIX_CP  /* POSIX way (a security problem versus symlink attacks!): */
-                dst_fd = open(dest, (flags & FILEUTILS_INTERACTIVE)
-                                ? O_WRONLY|O_CREAT|O_EXCL
-                                : O_WRONLY|O_CREAT|O_TRUNC, source_stat.st_mode);
-#else  /* safe way: */
-                dst_fd = open(dest, O_WRONLY|O_CREAT|O_EXCL, source_stat.st_mode);
-#endif
+                /* POSIX way is a security problem versus symlink attacks,
+                 * we do it only for non-symlinks, and only for non-recursive,
+                 * non-interactive cp. NB: it is still racy
+                 * for "cp file /home/bad_user/file" case
+                 * (user can rm file and create a link to /etc/passwd) */
+                if (DO_POSIX_CP
+                 || (dest_exists && !(flags & (FILEUTILS_RECUR|FILEUTILS_INTERACTIVE))
+                     && !S_ISLNK(dest_stat.st_mode))
+                ) {
+                        dst_fd = open(dest, O_WRONLY|O_CREAT|O_TRUNC, source_stat.st_mode);
+                } else  /* safe way: */
+                        dst_fd = open(dest, O_WRONLY|O_CREAT|O_EXCL, source_stat.st_mode);
                 if (dst_fd == -1) {
                         ovr = ask_and_unlink(dest, flags);
                         if (ovr <= 0) {
@@ -264,60 +295,55 @@ int copy_file(const char *source, const char *dest, int flags)
                 }
 #endif
                 if (bb_copyfd_eof(src_fd, dst_fd) == -1)
-                        status = -1;
+                        retval = -1;
+                /* Ok, writing side I can understand... */
                 if (close(dst_fd) < 0) {
                         bb_perror_msg("cannot close '%s'", dest);
-                        status = -1;
-                }
-                if (close(src_fd) < 0) {
-                        bb_perror_msg("cannot close '%s'", source);
-                        status = -1;
+                        retval = -1;
                 }
+                /* ...but read size is already checked by bb_copyfd_eof */
+                close(src_fd);
+                goto preserve_mode_ugid_time;
+        }
 
-        } else if (S_ISBLK(source_stat.st_mode) || S_ISCHR(source_stat.st_mode)
-         || S_ISSOCK(source_stat.st_mode) || S_ISFIFO(source_stat.st_mode)
-         || S_ISLNK(source_stat.st_mode)
-        ) {
-                // We are lazy here, a bit lax with races...
-                if (dest_exists) {
-                        ovr = ask_and_unlink(dest, flags);
-                        if (ovr <= 0)
-                                return ovr;
-                }
-                if (S_ISFIFO(source_stat.st_mode)) {
-                        if (mkfifo(dest, source_stat.st_mode) < 0) {
-                                bb_perror_msg("cannot create fifo '%s'", dest);
-                                return -1;
-                        }
-                } else if (S_ISLNK(source_stat.st_mode)) {
-                        char *lpath;
-
-                        lpath = xmalloc_readlink_or_warn(source);
-                        if (lpath && symlink(lpath, dest) < 0) {
+        /* Source is a symlink or a special file */
+        /* We are lazy here, a bit lax with races... */
+        if (dest_exists) {
+                errno = EEXIST;
+                ovr = ask_and_unlink(dest, flags);
+                if (ovr <= 0)
+                        return ovr;
+        }
+        if (S_ISLNK(source_stat.st_mode)) {
+                char *lpath = xmalloc_readlink_or_warn(source);
+                if (lpath) {
+                        int r = symlink(lpath, dest);
+                        free(lpath);
+                        if (r < 0) {
                                 bb_perror_msg("cannot create symlink '%s'", dest);
-                                free(lpath);
                                 return -1;
                         }
-                        free(lpath);
-
                         if (flags & FILEUTILS_PRESERVE_STATUS)
                                 if (lchown(dest, source_stat.st_uid, source_stat.st_gid) < 0)
                                         bb_perror_msg("cannot preserve %s of '%s'", "ownership", dest);
-
-                        return 0;
-
-                } else {
-                        if (mknod(dest, source_stat.st_mode, source_stat.st_rdev) < 0) {
-                                bb_perror_msg("cannot create '%s'", dest);
-                                return -1;
-                        }
+                }
+                /* _Not_ jumping to preserve_mode_ugid_time:
+                 * symlinks don't have those */
+                return 0;
+        }
+        if (S_ISBLK(source_stat.st_mode) || S_ISCHR(source_stat.st_mode)
+         || S_ISSOCK(source_stat.st_mode) || S_ISFIFO(source_stat.st_mode)
+        ) {
+                if (mknod(dest, source_stat.st_mode, source_stat.st_rdev) < 0) {
+                        bb_perror_msg("cannot create '%s'", dest);
+                        return -1;
                 }
         } else {
-                bb_error_msg("internal error: unrecognized file type");
+                bb_error_msg("unrecognized file '%s' with mode %x", source, source_stat.st_mode);
                 return -1;
         }
 
- preserve_status:
+ preserve_mode_ugid_time:
 
         if (flags & FILEUTILS_PRESERVE_STATUS
         /* Cannot happen: */
@@ -327,6 +353,7 @@ int copy_file(const char *source, const char *dest, int flags)
 
                 times.actime = source_stat.st_atime;
                 times.modtime = source_stat.st_mtime;
+                /* BTW, utimes sets usec-precision time - just FYI */
                 if (utime(dest, &times) < 0)
                         bb_perror_msg("cannot preserve %s of '%s'", "times", dest);
                 if (chown(dest, source_stat.st_uid, source_stat.st_gid) < 0) {
@@ -337,5 +364,5 @@ int copy_file(const char *source, const char *dest, int flags)
                         bb_perror_msg("cannot preserve %s of '%s'", "permissions", dest);
         }
 
-        return status;
+        return retval;
 }
diff --git a/libbb/xreadlink.c b/libbb/xreadlink.c
index 18a8b9467..4d87b944d 100644
--- a/libbb/xreadlink.c
+++ b/libbb/xreadlink.c
@@ -10,8 +10,7 @@
  * NOTE: This function returns a malloced char* that you will have to free
  * yourself. You have been warned.
  */
-
-char *xmalloc_readlink_or_warn(const char *path)
+char *xmalloc_readlink(const char *path)
 {
         enum { GROWBY = 80 }; /* how large we will grow strings by */
 
@@ -20,20 +19,30 @@ char *xmalloc_readlink_or_warn(const char *path)
 
         do {
                 buf = xrealloc(buf, bufsize += GROWBY);
-                readsize = readlink(path, buf, bufsize); /* 1st try */
+                readsize = readlink(path, buf, bufsize);
                 if (readsize == -1) {
-                        bb_perror_msg("%s", path);
                         free(buf);
                         return NULL;
                 }
-        }
-        while (bufsize < readsize + 1);
+        } while (bufsize < readsize + 1);
 
         buf[readsize] = '\0';
 
         return buf;
 }
 
+char *xmalloc_readlink_or_warn(const char *path)
+{
+        char *buf = xmalloc_readlink(path);
+        if (!buf) {
+                /* EINVAL => "file: Invalid argument" => puzzled user */
+                bb_error_msg("%s: cannot read link (not a symlink?)", path);
+        }
+        return buf;
+}
+
+/* UNUSED */
+#if 0
 char *xmalloc_realpath(const char *path)
 {
 #if defined(__GLIBC__) && !defined(__UCLIBC__)
@@ -46,3 +55,4 @@ char *xmalloc_realpath(const char *path)
         return xstrdup(realpath(path, buf));
 #endif
 }
+#endif