Subject: bug#28156: closed (Re: bug#28156: Emacs quietly munges symlink contents)

From: [email-masked] (GNU bug Tracking System)
Date: Sun, 27 Aug 2017 01:54:02 +0000

Your bug report

#28156: Emacs quietly munges symlink contents

which was filed against the emacs package, has been closed.

The explanation is attached below, along with your original report.
If you require more details, please reply to [email-masked].

-- 
28156: http://debbugs.gnu.org/cgi/bugreport.cgi?bug=28156
GNU Bug Tracking System
Contact [email-masked] with problems

Subject: Re: bug#28156: Emacs quietly munges symlink contents
From: Paul Eggert <[email-masked]>
To: Michael Albinus <[email-masked]>
Cc: Eli Zaretskii <[email-masked]>, [email-masked], [email-masked]
Date: Sat, 26 Aug 2017 18:53:25 -0700
List-Id: <debbugs-submit.debbugs.gnu.org>

Michael Albinus wrote:

> What about file-truename? I believe it still must quote the result,
> otherwise we run into problems. See
> 
> (make-symbolic-link "/x:y:" "/tmp/foo")
> (file-truename  "/tmp/foo")
> 
> The latter shall return "/:/x:y:" (as of today) instead of
> "/x:y:".

Thanks for catching that. While looking into it, I noticed that file-truename 
has long mishandled symlink contents starting with ~ as in the following example:

$ ln -s '~nosuchuser' /tmp/foo
$ ls -l /tmp/foo
lrwxrwxrwx. 1 eggert eggert 11 Aug 26 18:51 /tmp/foo -> ~nosuchuser
$ src/emacs -Q -batch -eval '(message "%S" (file-truename "/tmp/foo"))'
"/home/eggert/src/gnu/emacs/master-tmp/~nosuchuser"

I fixed both problems.

> Maybe we shall document it as well. Neither the docstring of
> `file-truename', nor the Lisp reference say something about.
Yes, the documentation could stand some improving here. I fixed it up a bit, 
though not in the file-truename area as I'm thinking we may need some more 
changes here and would like to cogitate about it first. I'll CC: you on any 
further bug reports in this area.

I rebased the patch to master which simplified it since I no longer had to worry 
about Tramp tests, and installed the result. Closing this bug report.

Subject: Emacs quietly munges symlink contents
From: Paul Eggert <[email-masked]>
To: Emacs bug reports and feature requests <[email-masked]>
Cc: Michael Albinus <[email-masked]>
Date: Sun, 20 Aug 2017 03:28:04 -0700
List-Id: <debbugs-submit.debbugs.gnu.org>

Tags: patch

The attached patch fixes some Emacs behavior that disagrees with the 
documentation.  Although the user manual says that make-symbolic-link "does
not expand the argument TARGET", Emacs expands leading "~" in the target. Also, 
file-symlink-p quietly munges symlink contents if they appear to be a Tramp file 
name. This behavior makes it impossible to write Emacs code that deals with 
arbitrary local symbolic links, and Emacs mishandles copying of some symlinks 
for this reason. At the operating system level, symlink targets are merely 
strings, and are not file names that are interpreted (any interpretation occurs 
later, only when the symlinks are followed), and Emacs should be consistent with 
that.

It strikes me that a similar change probably needs to be made to Tramp, so that 
remote symlinks also can be arbitrary strings too. However, that's outside my 
expertise so this patch affects only local symlink contents.
From faf68f4979509cd61b5f666479b66ec5b37594d6 Mon Sep 17 00:00:00 2001
From: Paul Eggert <[email-masked]>
Date: Sun, 20 Aug 2017 03:13:52 -0700
Subject: [PATCH] Do not munge contents of local symbolic links

This lets Emacs deal with arbitrary local symlinks without
mishandling their contents.  For example,
(progn (shell-command "ln -fs '~' 'x'") (rename-file "x" "/tmp/x"))
now consistently creates a symbolic link from '/tmp/x' to '~'.
Formerly, it did that only if the working directory was on the
same filesystem as /tmp; otherwise, it expanded the '~' to
the user's home directory.
* etc/NEWS: Document the change.
* src/fileio.c (Fmake_symbolic_link): Do not expand leading
"~" in the target, and look for special constructs only in
the new link name, not the target.
(emacs_readlinkat): Do not preprend "/:" to the link target if
it starts with "/" and contains ":" before NUL.
* test/lisp/net/tramp-tests.el (tramp-test21-file-links):
Adjust to new behavior.
* test/src/fileio-tests.el (try-link): Rename from try-char,
and accept a string instead of a char.  All uses changed.
(fileio-tests--symlink-failure): Also test leading ~, and "/:",
to test the new behavior.
---
 etc/NEWS                     |  8 ++++++++
 src/fileio.c                 | 18 ------------------
 test/lisp/net/tramp-tests.el |  9 ++++++---
 test/src/fileio-tests.el     | 21 ++++++++++-----------
 4 files changed, 24 insertions(+), 32 deletions(-)

diff --git a/etc/NEWS b/etc/NEWS
index 7774d75..104ee04 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -1204,6 +1204,14 @@ instead of to utf-8.  Before this change, Emacs would sometimes
 mishandle file names containing these control characters.
 
 +++
+** 'file-attributes', 'file-symlink-p' and 'make-symbolic-link' no
+longer quietly mutate the target of a local symbolic link.  In
+particular, 'file-attributes' and 'file-symlink-p' no longer prepend
+"/:" to some targets of local symbolic links, and 'make-symbolic-link'
+no longer expands '~' at the start of a link target.  This change lets
+Emacs access and copy arbitrary local symbolic links.
+
++++
 ** Module functions are now implemented slightly differently; in
 particular, the function 'internal--module-call' has been removed.
 Code that depends on undocumented internals of the module system might
diff --git a/src/fileio.c b/src/fileio.c
index f954ac1..ce243c3 100644
--- a/src/fileio.c
+++ b/src/fileio.c
@@ -2423,21 +2423,8 @@ This happens for interactive use with M-x.  */)
   Lisp_Object encoded_target, encoded_linkname;
 
   CHECK_STRING (target);
-  /* If the link target has a ~, we must expand it to get
-     a truly valid file name.  Otherwise, do not expand;
-     we want to permit links to relative file names.  */
-  if (SREF (target, 0) == '~')
-    target = Fexpand_file_name (target, Qnil);
-
   linkname = expand_cp_target (target, linkname);
 
-  /* If the file name has special constructs in it,
-     call the corresponding file handler.  */
-  handler = Ffind_file_name_handler (target, Qmake_symbolic_link);
-  if (!NILP (handler))
-    return call4 (handler, Qmake_symbolic_link, target,
-		  linkname, ok_if_already_exists);
-
   /* If the new link name has special constructs in it,
      call the corresponding file handler.  */
   handler = Ffind_file_name_handler (linkname, Qmake_symbolic_link);
@@ -2635,11 +2622,6 @@ emacs_readlinkat (int fd, char const *filename)
     return Qnil;
 
   val = build_unibyte_string (buf);
-  if (buf[0] == '/' && strchr (buf, ':'))
-    {
-      AUTO_STRING (slash_colon, "/:");
-      val = concat2 (slash_colon, val);
-    }
   if (buf != readlink_buf)
     xfree (buf);
   val = DECODE_FILE (val);
diff --git a/test/lisp/net/tramp-tests.el b/test/lisp/net/tramp-tests.el
index 9dc276b..0740996 100644
--- a/test/lisp/net/tramp-tests.el
+++ b/test/lisp/net/tramp-tests.el
@@ -2538,13 +2538,16 @@ tramp--test-backtrace
 	    (should-error (make-symbolic-link tmp-name1 tmp-name2))
 	    (make-symbolic-link tmp-name1 tmp-name2 'ok-if-already-exists)
 	    (should (file-symlink-p tmp-name2))
-	    ;; `tmp-name3' is a local file name.
-	    (should-error (make-symbolic-link tmp-name1 tmp-name3)))
+	    ;; `tmp-name3' is a local file name, so its target is not
+	    ;; interpreted.
+	    (make-symbolic-link tmp-name1 tmp-name3)
+	    (should (equal tmp-name1 (file-symlink-p tmp-name3))))
 
 	;; Cleanup.
 	(ignore-errors
 	  (delete-file tmp-name1)
-	  (delete-file tmp-name2)))
+	  (delete-file tmp-name2)
+	  (delete-file tmp-name3)))
 
       ;; Check `add-name-to-file'.
       (unwind-protect
diff --git a/test/src/fileio-tests.el b/test/src/fileio-tests.el
index 2ef1b55..5103d2f 100644
--- a/test/src/fileio-tests.el
+++ b/test/src/fileio-tests.el
@@ -19,14 +19,13 @@
 
 (require 'ert)
 
-(defun try-char (char link)
-  (let ((target (string char)))
-    (make-symbolic-link target link)
-    (let* ((read-link (file-symlink-p link))
-           (failure (unless (string-equal target read-link)
-                      (list 'string-equal target read-link))))
-      (delete-file link)
-      failure)))
+(defun try-link (target link)
+  (make-symbolic-link target link)
+  (let* ((read-link (file-symlink-p link))
+         (failure (unless (string-equal target read-link)
+                    (list 'string-equal target read-link))))
+    (delete-file link)
+    failure))
 
 (defun fileio-tests--symlink-failure ()
   (let* ((dir (make-temp-file "fileio" t))
@@ -36,9 +35,9 @@ fileio-tests--symlink-failure
               (char 0))
           (while (and (not failure) (< char 127))
             (setq char (1+ char))
-            (unless (= char ?~)
-              (setq failure (try-char char link))))
-          failure)
+            (setq failure (try-link (string char) link)))
+          (or failure
+              (try-link "/:" link)))
       (delete-directory dir t))))
 
 (ert-deftest fileio-tests--odd-symlink-chars ()
-- 
2.7.4