Re: coreutils bug with "ln x d/"

[Paul Eggert]

Jim Meyering <address@hidden> writes:

>> If you like, I think I can split
>> up the patch into two parts: a code- and diagnostic-cleanup only, and
>> a bug fix.  (I would do this by modifying my version to have the
>> bug.... :-)
>
> I would indeed appreciate that.

OK, I split the patch into two.  This takes a more conservative
approach than what I wrote above: the first patch fixes the bug, and
the second patch (which I'll send in my next message) is the cleanup
patch.

These two patches supersedes the long patch I sent you Friday
<http://lists.gnu.org/archive/html/bug-coreutils/2004-06/msg00146.html>
but it's independent of the patch to "install" that I sent yesterday
<http://lists.gnu.org/archive/html/bug-coreutils/2004-06/msg00153.html>.

Here's the first (bugfix) patch.

2004-06-27  Paul Eggert  <address@hidden>

        Fix a bug: formerly, if d/x was a directory and x a file, "ln x
        d/" incorrectly created a link d/x/x.  It also saves some system
        calls.

        * NEWS: Document the fix.

        * src/ln.c (main): Don't append basename to dest if this
        results in an existing directory name.
        * tests/ln/misc: See whether a trailing slash is followed too far.

Index: NEWS
===================================================================
RCS file: /home/meyering/coreutils/cu/NEWS,v
retrieving revision 1.219
diff -p -u -r1.219 NEWS
--- NEWS        22 Jun 2004 15:02:59 -0000      1.219
+++ NEWS        27 Jun 2004 07:05:35 -0000
@@ -42,6 +42,9 @@ GNU coreutils NEWS                      
   when first encountering a directory, `rm -r' would mistakenly fail
   to remove files under that directory.
 
+  If d/x is a directory and x a file, "ln x d/" now reports an error
+  instead of incorrectly creating a link to d/x/x.
+
   ptx now diagnoses invalid values for its --width=N (-w)
   and --gap-size=N (-g) options.
 
Index: src/ln.c
===================================================================
RCS file: /home/meyering/coreutils/cu/src/ln.c,v
retrieving revision 1.137
diff -p -u -r1.137 ln.c
--- src/ln.c    25 Jun 2004 06:51:51 -0000      1.137
+++ src/ln.c    27 Jun 2004 04:50:48 -0000
@@ -550,25 +550,31 @@ main (int argc, char **argv)
   else
     {
       struct stat source_stats;
-      char *new_dest;
       char const *source = file[0];
       char *dest = file[1];
       size_t destlen = strlen (dest);
+      char *new_dest = dest;
 
       /* When the destination is specified with a trailing slash and the
         source exists but is not a directory, convert the user's command
-        `ln source dest/' to `ln source dest/basename(source)'.  */
+        `ln source dest/' to `ln source dest/basename(source)'.
+         However, skip this step if dest/basename(source) is a directory.  */
 
       if (destlen != 0
          && dest[destlen - 1] == '/'
          && lstat (source, &source_stats) == 0
          && !S_ISDIR (source_stats.st_mode))
        {
-         PATH_BASENAME_CONCAT (new_dest, dest, source);
-       }
-      else
-       {
-         new_dest = dest;
+         struct stat dest_stats;
+         char *dest_plus_source_basename;
+
+         PATH_BASENAME_CONCAT (dest_plus_source_basename, dest, source);
+
+         if (! ((((dereference_dest_dir_symlinks ? stat : lstat)
+                  (dest_plus_source_basename, &dest_stats))
+                 == 0)
+                && S_ISDIR (dest_stats.st_mode)))
+           new_dest = dest_plus_source_basename;
        }
 
       errors = do_link (source, new_dest);
Index: tests/ln/misc
===================================================================
RCS file: /home/meyering/coreutils/cu/tests/ln/misc,v
retrieving revision 1.13
diff -p -u -r1.13 misc
--- tests/ln/misc       25 Dec 2000 18:31:18 -0000      1.13
+++ tests/ln/misc       27 Jun 2004 04:54:18 -0000
@@ -44,6 +44,14 @@ ln -s ../$f $d || fail=1
 test -f $d/$f || fail=1
 rm -rf $d $f
 
+# See whether a trailing slash is followed too far.
+touch $f || framework_failure=1
+rm -rf $d || framework_failure=1
+mkdir $d $d/$f || framework_failure=1
+ln $f $d/ 2> /dev/null && fail=1
+ln -s $f $d/ 2> /dev/null && fail=1
+rm -rf $d $f
+
 # Make sure we get a failure with existing dest without -f option
 touch $t || framework_failure=1
 # FIXME: don't ignore the error message but rather test