bug#7450: cp does not check for errno=EISDIR when target is dangling link

[Jim Meyering]

Alan Curry wrote:
> Марк Коренберг writes:
>>
>> How to reproduce:
>>
>> $ ln -s non-exist tgt
> [...]
>> $ cp /etc/passwd tgt/
>> cp: cannot create regular file `tgt/': Is a directory
>>
>> Novices can not understand this message :)
>
> The same confusing error message also occurs in the simpler case where the
> target is simply any nonexistent name with a trailing slash.
>
> $ ls -ld tgt
> ls: cannot access tgt: No such file or directory
> $ cp /etc/passwd tgt/
> cp: cannot create regular file `tgt/': Is a directory
>
> strace shows this:
>
> open("tgt/", O_WRONLY|O_CREAT|O_EXCL|O_LARGEFILE, 0600) = -1 EISDIR
> (Is a directory)
>
> which I think is just bad kernel behavior. There's no errno (among the
> classical errno values anyway) which completely expresses "you tried to creat
> something with a trailing slash", but I'd rather see ENOENT or ENOTDIR than
> EISDIR.
>
> When this open fails, nothing in sight "Is a directory". The problem is that
> tgt/ _would_ be a directory, but since it doesn't exist and isn't about to be
> created, "Is a directory" is an overstatement.
>
> cp should dodge this issue by never calling creat with a trailing slash. It
> could supply a meaningful error message instead of one that is derived from
> an errno.

Thanks for the suggestions.
Here's the patch I'm considering:
[I first patched copy.c's copy_reg, included at end,
 but didn't like that as much; the core copying code
 should not be encumbered like that, and other users of copy.c
 are not affected. ]

This demonstrates the new diagnostic:

  $ touch k && rm -rf no-such && ./cp k no-such/
  ./cp: invalid destination: `no-such/'
  a destination with a trailing slash must refer to an existing directory
  [Exit 1]

This patch currently tests for EISDIR (linux kernels).
If others systems fail with a different errno value, I'll adjust.

>From cfa35c82106a63658879703a245cad5e6d8b7055 Mon Sep 17 00:00:00 2001
From: Jim Meyering <address@hidden>
Date: Sun, 21 Nov 2010 05:54:55 +0100
Subject: [PATCH] cp: give a better diagnostic for a special type of invalid 
argument
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

* src/cp.c (do_copy): Give a better diagnostic when cp's 2nd and
final argument ends in a slash, but does not name a directory.
Reported by Марк Коренберг and Alan Curry.
* THANKS: Update.
* tests/mv/trailing-slash: Add a test.
---
 THANKS                  |    1 +
 src/cp.c                |   26 +++++++++++++++++++++++---
 tests/mv/trailing-slash |    9 +++++++++
 3 files changed, 33 insertions(+), 3 deletions(-)

diff --git a/THANKS b/THANKS
index 0123c32..9bd78c8 100644
--- a/THANKS
+++ b/THANKS
@@ -362,6 +362,7 @@ M. P. Suzuki                        address@hidden
 Maciej Kwapulinski                  address@hidden
 Manas Garg                          address@hidden
 Manfred Hollstein                   address@hidden
+Марк Коренберг                      address@hidden
 Marc Boucher                        address@hidden
 Marc Haber                          address@hidden
 Marc Lehman                         address@hidden
diff --git a/src/cp.c b/src/cp.c
index 5b14f3a..770add0 100644
--- a/src/cp.c
+++ b/src/cp.c
@@ -610,9 +610,29 @@ do_copy (int n_files, char **file, const char 
*target_directory,
       if (2 <= n_files
           && target_directory_operand (file[n_files - 1], &sb, &new_dst))
         target_directory = file[--n_files];
-      else if (2 < n_files)
-        error (EXIT_FAILURE, 0, _("target %s is not a directory"),
-               quote (file[n_files - 1]));
+      else
+        {
+          char const *dst_name = file[n_files - 1];
+          if (2 < n_files)
+            error (EXIT_FAILURE, 0, _("target %s is not a directory"),
+                   quote (dst_name));
+
+          if ( ! x->recursive)
+            {
+              /* Give a better diagnostic when the 2nd and final argument
+                 of a non-recursive cp command ends in a slash but does not
+                 name a directory. Before, cp FILE no-such/ would evoke
+                 this misleading diagnostic:
+                   cp: cannot create regular file `no-such/': Is a directory
+                 in spite of the fact that "no-such" does not exist.  */
+              size_t dst_len = strlen (dst_name);
+              if (dst_len && dst_name[dst_len - 1] == '/')
+                error (EXIT_FAILURE, 0,
+                       _("invalid destination: %s\na destination with a "
+                         "trailing slash must refer to an existing directory"),
+                       quote (dst_name));
+            }
+        }
     }

   if (target_directory)
diff --git a/tests/mv/trailing-slash b/tests/mv/trailing-slash
index b58c908..c94ebb0 100755
--- a/tests/mv/trailing-slash
+++ b/tests/mv/trailing-slash
@@ -50,4 +50,13 @@ done
 #touch a a2
 #mv a a2/ && fail=1

+# Test for a cp-specific diagnostic introduced after coreutils-8.7:
+printf '%s\n' \
+  "cp: invalid destination: \`no-such/'" \
+  "a destination with a trailing slash must refer to an existing directory" \
+> expected-err
+touch b
+cp b no-such/ 2> err && fail=1
+compare err expected-err || fail=1
+
 Exit $fail
--
1.7.3.2.765.g642a8


For reference, here's the patch I wrote and discarded.
Yes, it's buggy/incomplete, since I hadn't added
the test to detect a zero-length dst_name: as-is,
it would compare dst_name[-1] == '/':

diff --git a/src/copy.c b/src/copy.c
index 07501df..285768c 100644
--- a/src/copy.c
+++ b/src/copy.c
@@ -572,6 +572,16 @@ copy_reg (char const *src_name, char const *dst_name,
                         dst_mode & ~omitted_permissions);
       dest_errno = errno;

+      if (dest_desc < 0 && dest_errno == EISDIR
+          && dst_name[strlen (dst_name) - 1] == '/' && ! x->move_mode)
+        {
+          error (0, 0, _("a destination with a trailing slash must "
+                         "refer to an existing directory %s"),
+                 quote (dst_name));
+          return_val = false;
+          goto close_src_desc;
+        }
+
       /* When trying to copy through a dangling destination symlink,
          the above open fails with EEXIST.  If that happens, and
          lstat'ing the DST_NAME shows that it is a symlink, then we