bug#28156: Emacs quietly munges symlink contents

bug#28156: Emacs quietly munges symlink contents

[Paul Eggert]

[-- Attachment #1: Type: text/plain, Size: 961 bytes --]

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.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Do-not-munge-contents-of-local-symbolic-links.patch --]
[-- Type: text/x-patch; name="0001-Do-not-munge-contents-of-local-symbolic-links.patch", Size: 6077 bytes --]

From faf68f4979509cd61b5f666479b66ec5b37594d6 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
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

bug#28156: Emacs quietly munges symlink contents

[Michael Albinus]

Paul Eggert <eggert@cs.ucla.edu> writes:

Hi Paul,

> 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.

I'll have a look on this. Pls give me some days.

Best regards, Michael.

bug#28156: Emacs quietly munges symlink contents

[Eli Zaretskii]

> From: Paul Eggert <eggert@cs.ucla.edu>
> Date: Sun, 20 Aug 2017 03:28:04 -0700
> Cc: Michael Albinus <michael.albinus@gmx.de>
> 
> 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.

Sorry, I'm probably missing something here, but doesn't Emacs behave
here like Unix shell commands do?  For example, I just did

  $ ln -s ~/bin/etags ttt

and the following 'ls' command shows this:

  $ ls -l ttt
  lrwxrwxrwx 1 eliz eliz 22 Aug 20 10:27 ttt -> /home/e/eliz/bin/etags*

AFAIU, this means the shell expanded "~" when it passed it to 'ln'.
And Emacs tries to behave like the shell does in this case (and in
other similar cases).  Why is that wrong?

Moreover, unless I again misunderstand something important, if
make-symbolic-link would create a link like this:

  ttt -> ~/bin/etags

(which is what your proposed change does, right?), then programs which
follow the link will probably fail, because AFAIK most programs don't
expand "~" (with the notable exception of the shell).

What am I missing here?

>  +++
> +** '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.

Regardless of the issue at hand, if the change is installed, this text
should be clarified.  As written, I find it very hard to understand
what is the nature of the change, and how does it change the old
behavior.  For example, the reference to "some targets" could benefit
from a couple of examples showing the old and the new behavior in each
case.

Thanks.

bug#28156: Emacs quietly munges symlink contents

[Philipp Stephani]

[-- Attachment #1: Type: text/plain, Size: 2128 bytes --]

Eli Zaretskii <eliz@gnu.org> schrieb am So., 20. Aug. 2017 um 16:38 Uhr:

> > From: Paul Eggert <eggert@cs.ucla.edu>
> > Date: Sun, 20 Aug 2017 03:28:04 -0700
> > Cc: Michael Albinus <michael.albinus@gmx.de>
> >
> > 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.
>
> Sorry, I'm probably missing something here, but doesn't Emacs behave
> here like Unix shell commands do?  For example, I just did
>
>   $ ln -s ~/bin/etags ttt
>
> and the following 'ls' command shows this:
>
>   $ ls -l ttt
>   lrwxrwxrwx 1 eliz eliz 22 Aug 20 10:27 ttt -> /home/e/eliz/bin/etags*
>
> AFAIU, this means the shell expanded "~" when it passed it to 'ln'.
> And Emacs tries to behave like the shell does in this case (and in
> other similar cases).  Why is that wrong?
>

It makes it impossible to use '~' as a directory name. Higher-level
interactive interfaces such as Shell and Dired can live with such a
restriction, but library APIs need to be able to work with arbitrary inputs.


>
> Moreover, unless I again misunderstand something important, if
> make-symbolic-link would create a link like this:
>
>   ttt -> ~/bin/etags
>
> (which is what your proposed change does, right?), then programs which
> follow the link will probably fail, because AFAIK most programs don't
> expand "~" (with the notable exception of the shell).
>
> What am I missing here?


Callers of `make-symbolic-link' will need to expand the target themselves.

[-- Attachment #2: Type: text/html, Size: 2909 bytes --]

bug#28156: Emacs quietly munges symlink contents

[Eli Zaretskii]

> From: Philipp Stephani <p.stephani2@gmail.com>
> Date: Sun, 20 Aug 2017 15:09:01 +0000
> Cc: michael.albinus@gmx.de, 28156@debbugs.gnu.org
> 
> It makes it impossible to use '~' as a directory name. Higher-level interactive interfaces such as Shell and
> Dired can live with such a restriction, but library APIs need to be able to work with arbitrary inputs.

We have quoting for these cases.

> Callers of `make-symbolic-link' will need to expand the target themselves. 

make-symbolic-link is an interactive command, not just a function.

bug#28156: Emacs quietly munges symlink contents

[Paul Eggert]

[-- Attachment #1: Type: text/plain, Size: 1441 bytes --]

Eli Zaretskii wrote:

> We have quoting for these cases.

Quoting does not work for these cases. If I try to rename a symlink to the 
literal string '~eggert' on my machine, Emacs will misbehave as described and 
there is no way to quote the string naming the symlink to fix this.

> doesn't Emacs behave here like Unix shell commands do?

They differ in many ways. Two examples. First, the Unix shell command 'ln -s 
~/$$ def' expands both the "~" and the "$$", whereas (make-symbolic-link "~/$$" 
"def") expands only the "~". Second, the Unix shell command "ln -s def" creates 
a symlink to itself, whereas (make-symbolic-link "def") is an error. Briefly, 
the Unix shell commands are higher-level than the Emacs primitives.

The main points here are (1) Emacs functions should let users create and 
manipulate whatever symlinks they want to, and (2) the documentation has long 
said that symlinks are not expanded.

> If make-symbolic-link would create a link like this:
> 
>   ttt -> ~/bin/etags
> 
> (which is what your proposed change does, right?), then programs which
> follow the link will probably fail

Yes, they will fail unless there is a directory named '~'. That is the intent. 
If I want to create a symlink to my home directory, I can use expand-file-name 
on the link target, before calling make-symbolic-link.

> this text should be clarified.

Sure, that's easy. Revised patch attached.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Do-not-munge-contents-of-local-symbolic-links.patch --]
[-- Type: text/x-patch; name="0001-Do-not-munge-contents-of-local-symbolic-links.patch", Size: 6547 bytes --]

From 6a3f716feac756a6e80ba4c571155377bd1cba28 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
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                     | 15 +++++++++++++++
 src/fileio.c                 | 18 ------------------
 test/lisp/net/tramp-tests.el |  9 ++++++---
 test/src/fileio-tests.el     | 21 ++++++++++-----------
 4 files changed, 31 insertions(+), 32 deletions(-)

diff --git a/etc/NEWS b/etc/NEWS
index 7774d75..3bafb87 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -1204,6 +1204,21 @@ 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, so that
+Emacs can access and copy them reliably regardless of their contents.
+Two changes are involved.  First, 'file-attributes' and
+'file-symlink-p' no longer prepend "/:" to symbolic links whose
+targets begin with "/" and contain ":".  For example, if a symbolic
+link "x" has a target "/y:z", (file-symlink-p "x") now returns "/y:z"
+rather than "/:/y:z".  Second, 'make-symbolic-link' no longer expands
+"~" at the start of a link target before creating the link.  For
+example, (make-symbolic-link "~y" "x") now creates a symbolic link
+with target being the string "~y" and not the home directory of the
+user y.  To create a symbolic link to y's home directory, use
+(make-symbolic-link (expand-file-name "~y") "x").
+
++++
 ** 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

bug#28156: Emacs quietly munges symlink contents

[Michael Albinus]

Paul Eggert <eggert@cs.ucla.edu> writes:

>> We have quoting for these cases.
>
> Quoting does not work for these cases. If I try to rename a symlink to
> the literal string '~eggert' on my machine, Emacs will misbehave as
> described and there is no way to quote the string naming the symlink
> to fix this.

(make-symbolic-link "~/.emacs" "/:/tmp/~eggert")

# ls -l /tmp
[...]
lrwxrwxrwx  1 albinus albinus     20 Aug 20 20:24 ~eggert -> /home/albinus/.emacs

(rename-file "/:/tmp/~eggert" "/:/tmp/~albinus")

# ls -l /tmp
[...]
lrwxrwxrwx  1 albinus albinus     20 Aug 20 20:24 ~albinus -> /home/albinus/.emacs

Best regards, Michael.

bug#28156: Emacs quietly munges symlink contents

[Paul Eggert]

Michael Albinus wrote:
>>> We have quoting for these cases.
>> Quoting does not work for these cases. If I try to rename a symlink to
>> the literal string '~eggert' on my machine, Emacs will misbehave as
>> described and there is no way to quote the string naming the symlink
>> to fix this.
> (make-symbolic-link "~/.emacs" "/:/tmp/~eggert") ...

That's not the problem I was referring to. Sorry, I should have given more 
detail. By "misbehave as described" I was referring to an earlier-mentioned 
scenario where rename-file copies because the source and destination are on 
different filesystems. For example, suppose the current directory is on a 
different filesystem from /tmp, and I execute the following in my *scratch* buffer:

(shell-command "ln -s '~' symlink")
0
(file-symlink-p "symlink")
"~"
(rename-file "symlink" "/tmp/symlink")
nil
(file-symlink-p "/tmp/symlink")
"/home/eggert"

Here, rename-file quietly expands the symlink contents, which is a bug. As far 
as I can see, one cannot work around the bug by using Tramp quoting; for 
example, (rename-file "/:symlink" "/:/tmp/symlink") does the same thing that 
(rename-file "symlink" "/tmp/symlink") does.

bug#28156: Emacs quietly munges symlink contents

[Michael Albinus]

Paul Eggert <eggert@cs.ucla.edu> writes:

Hi Paul,

> Here, rename-file quietly expands the symlink contents, which is a
> bug. As far as I can see, one cannot work around the bug by using
> Tramp quoting; for example, (rename-file "/:symlink" "/:/tmp/symlink")
> does the same thing that (rename-file "symlink" "/tmp/symlink") does.

Just to understand what you are speaking about: what do you mean with
"Tramp quoting"? A file name starting with "/:" has nothing to do with
Tramp.

(For the rest of your email I need to run some tests)

Best regards, Michael.

bug#28156: Emacs quietly munges symlink contents

[Paul Eggert]

Michael Albinus wrote:
> Just to understand what you are speaking about: what do you mean with
> "Tramp quoting"? A file name starting with "/:" has nothing to do with
> Tramp.

Sorry, I'm afraid I don't understand Tramp at all but I don't think it matters. 
By "Tramp quoting" I was referring to the technique you used in 
https://debbugs.gnu.org/cgi/bugreport.cgi?bug=28156#23 to create a symlink whose 
name starts with "~", by prefixing the name with "/:/tmp/". I don't know why the 
"/:" is there and it doesn't really matter, since the bug in question is not 
with the name of the symlink, it is with the contents of the symlink.

bug#28156: Emacs quietly munges symlink contents

[Michael Albinus]

Paul Eggert <eggert@cs.ucla.edu> writes:

Hi Paul,

> By "Tramp quoting" I was referring to the technique you used in
> https://debbugs.gnu.org/cgi/bugreport.cgi?bug=28156#23 to create a
> symlink whose name starts with "~", by prefixing the name with
> "/:/tmp/". I don't know why the "/:" is there ...

See the lower part of (info "(elisp) File Name Expansion")

Best regards, Michael.

bug#28156: Emacs quietly munges symlink contents

[Eli Zaretskii]

> Cc: Eli Zaretskii <eliz@gnu.org>, Philipp Stephani <p.stephani2@gmail.com>,
>  28156@debbugs.gnu.org
> From: Paul Eggert <eggert@cs.ucla.edu>
> Date: Sun, 20 Aug 2017 11:53:55 -0700
> 
> (shell-command "ln -s '~' symlink")
> 0
> (file-symlink-p "symlink")
> "~"
> (rename-file "symlink" "/tmp/symlink")
> nil
> (file-symlink-p "/tmp/symlink")
> "/home/eggert"

If this is the problem, then let's solve it without affecting
make-symbolic-link.  The problem you show is in file-symlink-p and/or
in rename-file.  Let's solve it there, and let's solve it without
affecting the interactive callers of those functions.

> Here, rename-file quietly expands the symlink contents, which is a bug. As far 
> as I can see, one cannot work around the bug by using Tramp quoting; for 
> example, (rename-file "/:symlink" "/:/tmp/symlink") does the same thing that 
> (rename-file "symlink" "/tmp/symlink") does.

But AFAIU, file-symlink-p can return a quoted name if its argument is
quoted.  Doesn't this allow to solve the problem?

bug#28156: Emacs quietly munges symlink contents

[Paul Eggert]

Eli Zaretskii wrote:

>> (shell-command "ln -s '~' symlink")
>> 0
>> (file-symlink-p "symlink")
>> "~"
>> (rename-file "symlink" "/tmp/symlink")
>> nil
>> (file-symlink-p "/tmp/symlink")
>> "/home/eggert"
> 
> If this is the problem,

It's just one instance of the problem.

> then let's solve it without affecting
> make-symbolic-link.

This instance of the problem occurs because rename-file calls 
make-symbolic-link, and make-symbolic-link silently alters the link target. The 
real problem here is with make-symbolic-link: the rename-file bug is just a symptom.

>> Here, rename-file quietly expands the symlink contents, which is a bug. As far
>> as I can see, one cannot work around the bug by using Tramp quoting; for
>> example, (rename-file "/:symlink" "/:/tmp/symlink") does the same thing that
>> (rename-file "symlink" "/tmp/symlink") does.
> 
> But AFAIU, file-symlink-p can return a quoted name if its argument is
> quoted.

I don't know what you mean by "quoted". file-symlink-returns a string, and the 
string is supposed to be the contents of the symlink. file-symlink-p is working 
correctly here. It's make-symbolic-link (called from rename-file) that is 
screwing up.

bug#28156: Emacs quietly munges symlink contents

[Eli Zaretskii]

> Cc: michael.albinus@gmx.de, p.stephani2@gmail.com, 28156@debbugs.gnu.org
> From: Paul Eggert <eggert@cs.ucla.edu>
> Date: Sun, 20 Aug 2017 14:31:40 -0700
> 
> This instance of the problem occurs because rename-file calls 
> make-symbolic-link, and make-symbolic-link silently alters the link target. The 
> real problem here is with make-symbolic-link: the rename-file bug is just a symptom.

Then you want a way of invoking make-symbolic-link non-interactively
that doesn't expand "~", AFAIU.  No need to affect its interactive
invocation.

> >> Here, rename-file quietly expands the symlink contents, which is a bug. As far
> >> as I can see, one cannot work around the bug by using Tramp quoting; for
> >> example, (rename-file "/:symlink" "/:/tmp/symlink") does the same thing that
> >> (rename-file "symlink" "/tmp/symlink") does.
> > 
> > But AFAIU, file-symlink-p can return a quoted name if its argument is
> > quoted.
> 
> I don't know what you mean by "quoted".

I mean this:

  (file-symlink-p "/:/tmp/symlink")

There's special code in file-symlink-p to support this and return a
quoted name of the symlink target.  For some reason, you wanted to
remove that code.

bug#28156: Emacs quietly munges symlink contents

[Paul Eggert]

Eli Zaretskii wrote:

> No need to affect its interactive invocation.

Yes, that's what npostavs suggested too. I plan to look into it.


>    (file-symlink-p "/:/tmp/symlink")
> 
> There's special code in file-symlink-p to support this and return a
> quoted name of the symlink target.  For some reason, you wanted to
> remove that code.

The proposed change to file-symlink-p doesn't affect support for file name 
quoting in its argument string. All it affects is the returned string. There is 
no need for it to quote the returned string, just as there is no need for 
read-file-name to quote the string that it returns. Neither function is 
documented to quote its return value, and neither should do so.

bug#28156: Emacs quietly munges symlink contents

[Eli Zaretskii]

> Cc: michael.albinus@gmx.de, p.stephani2@gmail.com, 28156@debbugs.gnu.org
> From: Paul Eggert <eggert@cs.ucla.edu>
> Date: Mon, 21 Aug 2017 01:34:20 -0700
> 
> The proposed change to file-symlink-p doesn't affect support for file name 
> quoting in its argument string. All it affects is the returned string. There is 
> no need for it to quote the returned string

What if readlink returns a name such as "/ssh:foo@bar:/quux"?  A
symlink cannot have a remote file name as its target, can it?  So
quoting it sounds like a good way to avoid triggering a slow and
erroneous connection to some possibly non-existent host.  If we don't
quote, the caller might be in for a surprise.

> just as there is no need for read-file-name to quote the string that
> it returns.

read-file-name gets the string from the user, so it's an entirely
different context.  In that context, it's user's responsibility to
quote the file name if needed.

> Neither function is documented to quote its return value, and
> neither should do so.

Documentation in Emacs was never 110% accurate, so reading it as a
kind of formal requirements is not a good idea, IME.  This behavior is
in file-symlink-p since more than 17 years ago, so it's a de-facto
standard by now; if we decide it is what we want, we could document it
and move on.

bug#28156: Emacs quietly munges symlink contents

[Paul Eggert]

Eli Zaretskii wrote:
> What if readlink returns a name such as "/ssh:foo@bar:/quux"?  A
> symlink cannot have a remote file name as its target, can it?

A symlink target is a string, and can have any bytes in it (other than NUL). So 
if FOO is a remote file name then it can have FOO as a target. Of course the OS 
won't interpret the remote file name on its own, just as it won't interpret ~ or 
$ or whatever, but that is OK and expected.

> read-file-name gets the string from the user, so it's an entirely
> different context.

It was just one example, though it remains a good one. Another example is 
(directory-files dir), which does not escape its results even when they begin 
with ~. Really, there is no need or good precedent for the sort of escaping that 
you propose. It is much simpler (and agrees with the documentation and 
intuition) for file-symlink-p to return the results as-is, like directory-files 
does.

bug#28156: Emacs quietly munges symlink contents

[Eli Zaretskii]

> Cc: michael.albinus@gmx.de, 28156@debbugs.gnu.org
> From: Paul Eggert <eggert@cs.ucla.edu>
> Date: Sun, 20 Aug 2017 10:54:45 -0700
> 
> > We have quoting for these cases.
> 
> Quoting does not work for these cases. If I try to rename a symlink to the 
> literal string '~eggert' on my machine, Emacs will misbehave as described and 
> there is no way to quote the string naming the symlink to fix this.

That is but one use case of many.  In other cases, quoting does work.

> > doesn't Emacs behave here like Unix shell commands do?
> 
> They differ in many ways. Two examples. First, the Unix shell command 'ln -s 
> ~/$$ def' expands both the "~" and the "$$", whereas (make-symbolic-link "~/$$" 
> "def") expands only the "~". Second, the Unix shell command "ln -s def" creates 
> a symlink to itself, whereas (make-symbolic-link "def") is an error. Briefly, 
> the Unix shell commands are higher-level than the Emacs primitives.

I never said that Emacs does everything the shell does, and does it
exactly like the shell.  I was only talking about "~" expansion, so
let's not muddy the waters by adding more magic characters and
expansions into what is already a tricky issue.

> The main points here are (1) Emacs functions should let users create and 
> manipulate whatever symlinks they want to, and (2) the documentation has long 
> said that symlinks are not expanded.

I agree.  But I think your proposed changes will disallow some of the
users' capabilities to do what you want.  What's more, they seem to
sacrifice a very common interactive use case on behalf of a much more
rare and obscure one.  I'm talking about a user typing this:

  M-x make-symbolic-link RET ~/foo/bar RET /toto/quux RET

AFAIU, with your changes the "~" in the first file name will not be
expanded.  If I'm right, then you will disallow using "~" as the short
for the home directory in this case, which IMO is unacceptable, as
Emacs allows this in any other interactive command that accepts file
names.

At the very least, we should preserve the current ability of the user
to specify "~" shorthands in interactive invocations of
make-symbolic-link.  If you want to allow users to specify a literal
"~" in this scenario, I agree, but it seems like we already allow
that; see Micheal's example.

> > If make-symbolic-link would create a link like this:
> > 
> >   ttt -> ~/bin/etags
> > 
> > (which is what your proposed change does, right?), then programs which
> > follow the link will probably fail
> 
> Yes, they will fail unless there is a directory named '~'. That is the intent. 
> If I want to create a symlink to my home directory, I can use expand-file-name 
> on the link target, before calling make-symbolic-link.

We cannot force users call expand-file-name in interactive use of
these commands.  It's simply unacceptable.  I don't even know how
users could do that in practice -- do you expect them to type TAB or
something?  How can this work?

bug#28156: Emacs quietly munges symlink contents

[Paul Eggert]

[-- Attachment #1: Type: text/plain, Size: 1049 bytes --]

>> Quoting does not work for these cases. If I try to rename a symlink to the
>> literal string '~eggert' on my machine, Emacs will misbehave as described and
>> there is no way to quote the string naming the symlink to fix this.
> 
> That is but one use case of many.  In other cases, quoting does work.

My point is that there is a bug in some cases. I agree that in other cases, 
make-symbolic-link does work.

> your proposed changes will disallow some of the
> users' capabilities to do what you want.

With the proposed changes a user can create a local symlink to an any string 
supported by the operating system. This change increases users' capabilities, 
compared to what they can do now. This change does not take away any capabilities.

> they seem to
> sacrifice a very common interactive use case on behalf of a much more
> rare and obscure one.  I'm talking about a user typing this:
> 
>    M-x make-symbolic-link RET ~/foo/bar RET /toto/quux RET

Good suggestion, also made by others. Revised patch attached.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Do-not-munge-contents-of-local-symbolic-links.patch --]
[-- Type: text/x-patch; name="0001-Do-not-munge-contents-of-local-symbolic-links.patch", Size: 9029 bytes --]

From 868a212ba636a7cab27e1ee88495a7f7dcf71b0d Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Mon, 21 Aug 2017 09:46:34 -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 unless interactive, 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.
---
 doc/emacs/files.texi         |  7 ++++---
 doc/lispref/files.texi       |  3 ++-
 etc/NEWS                     | 15 +++++++++++++++
 src/fileio.c                 | 21 +++------------------
 test/lisp/net/tramp-tests.el |  9 ++++++---
 test/src/fileio-tests.el     | 21 ++++++++++-----------
 6 files changed, 40 insertions(+), 36 deletions(-)

diff --git a/doc/emacs/files.texi b/doc/emacs/files.texi
index 9195bc4..56e3d2f 100644
--- a/doc/emacs/files.texi
+++ b/doc/emacs/files.texi
@@ -1609,9 +1609,10 @@ Copying and Naming
 @var{new}, which points at @var{target}.  The effect is that future
 attempts to open file @var{new} will refer to whatever file is named
 @var{target} at the time the opening is done, or will get an error if
-the name @var{target} is nonexistent at that time.  This command does
-not expand the argument @var{target}, so that it allows you to specify
-a relative name as the target of the link.  On MS-Windows, this
+the name @var{target} is nonexistent at that time.  This command
+expands the argument @var{target} only if it begins with @samp{~}, so
+that it allows you to specify most relative names as the target of the
+link.  On MS-Windows, this
 command works only on MS Windows Vista and later.  On remote systems,
 it works depending on the system type.
 
diff --git a/doc/lispref/files.texi b/doc/lispref/files.texi
index 5a52765..11e4b43 100644
--- a/doc/lispref/files.texi
+++ b/doc/lispref/files.texi
@@ -1725,7 +1725,8 @@ Changing Files
 This command makes a symbolic link to @var{filename}, named
 @var{newname}.  This is like the shell command @samp{ln -s
 @var{filename} @var{newname}}.  The @var{filename} argument
-is treated only as a string; it need not name an existing file.
+need not name an existing file, and is expanded only if it begins with @samp{~}
+and @var{ok-if-already-exists} is an integer, indicating interactive use.
 If @var{filename} is a relative file name, the resulting symbolic link
 is interpreted relative to the directory containing the symbolic link.
 @xref{Relative File Names}.
diff --git a/etc/NEWS b/etc/NEWS
index 0939033..97462ec 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -1215,6 +1215,21 @@ 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, so that
+Emacs can access and copy them reliably regardless of their contents.
+Two changes are involved.  First, 'file-attributes' and
+'file-symlink-p' no longer prepend "/:" to symbolic links whose
+targets begin with "/" and contain ":".  For example, if a symbolic
+link "x" has a target "/y:z", (file-symlink-p "x") now returns "/y:z"
+rather than "/:/y:z".  Second, 'make-symbolic-link' now expands a link
+target with leading "~" only when the optional third arg is an
+integer, as when invoked interactively.  For example, with
+(make-symbolic-link "~y" "x") the link target is now the string "~y";
+to get the old behavior, use (make-symbolic-link (expand-file-name
+"~y") "x").
+
++++
 ** 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..ad2aab8 100644
--- a/src/fileio.c
+++ b/src/fileio.c
@@ -2415,7 +2415,8 @@ DEFUN ("make-symbolic-link", Fmake_symbolic_link, Smake_symbolic_link, 2, 3,
 Both args must be strings.
 Signal a `file-already-exists' error if a file LINKNAME already exists
 unless optional third argument OK-IF-ALREADY-EXISTS is non-nil.
-An integer third arg means request confirmation if LINKNAME already exists.
+An integer third arg means request confirmation if LINKNAME
+already exists, and expand TARGET if it begins with "~".
 This happens for interactive use with M-x.  */)
   (Lisp_Object target, Lisp_Object linkname, Lisp_Object ok_if_already_exists)
 {
@@ -2423,21 +2424,10 @@ 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) == '~')
+  if (INTEGERP (ok_if_already_exists) && 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 +2625,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 129bc1d..8c0c8c8 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

bug#28156: Emacs quietly munges symlink contents

[Eli Zaretskii]

> From: Paul Eggert <eggert@cs.ucla.edu>
> Cc: p.stephani2@gmail.com, michael.albinus@gmx.de, 28156@debbugs.gnu.org
> Date: Mon, 21 Aug 2017 10:34:27 -0700
> 
> -  /* 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);
> -

Why remove this part?  You also did not mentioned that in NEWS.

Thanks.

bug#28156: Emacs quietly munges symlink contents

[Paul Eggert]

[-- Attachment #1: Type: text/plain, Size: 287 bytes --]

Eli Zaretskii wrote:
> Why remove this part?  You also did not mentioned that in NEWS.

The symlink target is a string not a file name, so giving it to file name 
handler can cause the make-symbolic-link call to be misinterpreted. Revised NEWS 
in attached patch, with an example.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Do-not-munge-contents-of-local-symbolic-links.patch --]
[-- Type: text/x-patch; name="0001-Do-not-munge-contents-of-local-symbolic-links.patch", Size: 9250 bytes --]

From 6c8d19b8ab7db320db844c14b034923cf43a194c Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Mon, 21 Aug 2017 09:46:34 -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 unless interactive, 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.
---
 doc/emacs/files.texi         |  7 ++++---
 doc/lispref/files.texi       |  3 ++-
 etc/NEWS                     | 21 +++++++++++++++++++++
 src/fileio.c                 | 21 +++------------------
 test/lisp/net/tramp-tests.el |  9 ++++++---
 test/src/fileio-tests.el     | 21 ++++++++++-----------
 6 files changed, 46 insertions(+), 36 deletions(-)

diff --git a/doc/emacs/files.texi b/doc/emacs/files.texi
index 9195bc4..56e3d2f 100644
--- a/doc/emacs/files.texi
+++ b/doc/emacs/files.texi
@@ -1609,9 +1609,10 @@ Copying and Naming
 @var{new}, which points at @var{target}.  The effect is that future
 attempts to open file @var{new} will refer to whatever file is named
 @var{target} at the time the opening is done, or will get an error if
-the name @var{target} is nonexistent at that time.  This command does
-not expand the argument @var{target}, so that it allows you to specify
-a relative name as the target of the link.  On MS-Windows, this
+the name @var{target} is nonexistent at that time.  This command
+expands the argument @var{target} only if it begins with @samp{~}, so
+that it allows you to specify most relative names as the target of the
+link.  On MS-Windows, this
 command works only on MS Windows Vista and later.  On remote systems,
 it works depending on the system type.
 
diff --git a/doc/lispref/files.texi b/doc/lispref/files.texi
index 5a52765..11e4b43 100644
--- a/doc/lispref/files.texi
+++ b/doc/lispref/files.texi
@@ -1725,7 +1725,8 @@ Changing Files
 This command makes a symbolic link to @var{filename}, named
 @var{newname}.  This is like the shell command @samp{ln -s
 @var{filename} @var{newname}}.  The @var{filename} argument
-is treated only as a string; it need not name an existing file.
+need not name an existing file, and is expanded only if it begins with @samp{~}
+and @var{ok-if-already-exists} is an integer, indicating interactive use.
 If @var{filename} is a relative file name, the resulting symbolic link
 is interpreted relative to the directory containing the symbolic link.
 @xref{Relative File Names}.
diff --git a/etc/NEWS b/etc/NEWS
index 0939033..39c34fa 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -1215,6 +1215,27 @@ 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, so that
+Emacs can access and copy them reliably regardless of their contents.
+The following changes are involved.
+
+*** 'file-attributes' and 'file-symlink-p' no longer prepend "/:" to
+symbolic links whose targets begin with "/" and contain ":".  For
+example, if a symbolic link "x" has a target "/y:z", (file-symlink-p
+"x") now returns "/y:z" rather than "/:/y:z".
+
+*** 'make-symbolic-link' no longer looks for file name handlers when
+creating a local symbolic link.  For example, (make-symbolic-link
+"/y:z" "x") now creates a symlink to "/y:z" instead of failing.
+
+*** 'make-symbolic-link' now expands a link target with leading "~"
+only when the optional third arg is an integer, as when invoked
+interactively.  For example, with (make-symbolic-link "~y" "x") the
+link target is now the string "~y"; to get the old behavior, use
+(make-symbolic-link (expand-file-name "~y") "x").
+
++++
 ** 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..ad2aab8 100644
--- a/src/fileio.c
+++ b/src/fileio.c
@@ -2415,7 +2415,8 @@ DEFUN ("make-symbolic-link", Fmake_symbolic_link, Smake_symbolic_link, 2, 3,
 Both args must be strings.
 Signal a `file-already-exists' error if a file LINKNAME already exists
 unless optional third argument OK-IF-ALREADY-EXISTS is non-nil.
-An integer third arg means request confirmation if LINKNAME already exists.
+An integer third arg means request confirmation if LINKNAME
+already exists, and expand TARGET if it begins with "~".
 This happens for interactive use with M-x.  */)
   (Lisp_Object target, Lisp_Object linkname, Lisp_Object ok_if_already_exists)
 {
@@ -2423,21 +2424,10 @@ 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) == '~')
+  if (INTEGERP (ok_if_already_exists) && 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 +2625,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 129bc1d..8c0c8c8 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

bug#28156: Emacs quietly munges symlink contents

[Michael Albinus]

Paul Eggert <eggert@cs.ucla.edu> writes:

Hi Paul,

> The symlink target is a string not a file name, so giving it to file
> name handler can cause the make-symbolic-link call to be
> misinterpreted.

If you want to have an absolute filename as LINKNAME, you typically call

(make-symbolic-link (expand-file-name target) (expand-file-name linkname))

In case of remote file names, (expand-file-name linkname) will always
return something like "/method:user@host:/path/to/linkname". I doubt,
that a user wants to see this literal string as symbolic link.

Furthermore, there is the OK-IF-ALREADY-EXISTS argument of
make-symbolic-string. This requires to regard LINKNAME as a file name,
and not as a literal string.

Best regards, Michael.

bug#28156: Emacs quietly munges symlink contents

[Paul Eggert]

On 08/22/2017 12:28 AM, Michael Albinus wrote:
> If you want to have an absolute filename as LINKNAME, you typically call
>
> (make-symbolic-link (expand-file-name target) (expand-file-name linkname))
>
> In case of remote file names, (expand-file-name linkname) will always
> return something like "/method:user@host:/path/to/linkname". I doubt,
> that a user wants to see this literal string as symbolic link.

There seems to be some confusion here, as Bug#28156 does not propose any 
change to how make-symbolic-link's 2nd argument works, (expand-file-name 
linkname) in this case. If the 2nd argument specifies a remote name, as 
in your example, then Tramp will take over and do whatever it wants with 
both arguments, so Bug#28156 is not proposing any change there (although 
perhaps some changes would be helpful for consistency, that's a 
different matter).

Bug#28156 is proposing changes only when the new linkname is local, and 
in those cases, if I'm not mistaken, Emacs currently errors out when the 
target looks like it is remote, so Bug#28156 is merely proposing an 
extension. Emacs routinely creates dangling symlinks with targets 
containing unusual characters like ":", and it wouldn't be surprising if 
users wanted to do something similar on their own.

> Furthermore, there is the OK-IF-ALREADY-EXISTS argument of
> make-symbolic-string. This requires to regard LINKNAME as a file name,
> and not as a literal string.

That's fine, as Bug#28156 is not proposing any change here. LINKNAME 
continues to be treated as a file name under the proposed change. The 
proposed change affects only symlink contents, not symlink names.

bug#28156: Emacs quietly munges symlink contents

[Michael Albinus]

Paul Eggert <eggert@cs.ucla.edu> writes:

> On 08/22/2017 12:28 AM, Michael Albinus wrote:
>> If you want to have an absolute filename as LINKNAME, you typically call
>>
>> (make-symbolic-link (expand-file-name target) (expand-file-name linkname))
>>
>> In case of remote file names, (expand-file-name linkname) will always
>> return something like "/method:user@host:/path/to/linkname". I doubt,
>> that a user wants to see this literal string as symbolic link.
>
> There seems to be some confusion here, as Bug#28156 does not propose
> any change to how make-symbolic-link's 2nd argument works,
> (expand-file-name linkname) in this case. If the 2nd argument
> specifies a remote name, as in your example, then Tramp will take over
> and do whatever it wants with both arguments, so Bug#28156 is not
> proposing any change there (although perhaps some changes would be
> helpful for consistency, that's a different matter).

Sorry, I have confused the 1st and 2nd argument of make-symbolic-link.
Tramp's docstring is wrong, because it does the same error, for decades.
Will fix it.

However, it is Tramp's (almost undocumented) make-symbolic-link
implementation to handle TARGET as well, and to strip the remote part of
this. I wouldn't be surprised if existing packages will be broken if we
change this.

Coming to your examples, I believe the existing implementation gives you
almost all what is needed, with quoting. Using "~" in TARGET:

--8<---------------cut here---------------start------------->8---
M-x make-symbolic-link RET /:~eggert RET /tmp/foo RET

# ls -l /tmp/foo
lrwxrwxrwx 1 albinus albinus 21 Aug 24 13:19 /tmp/foo -> /home/albinus/~eggert
--8<---------------cut here---------------end--------------->8---

It is questionable whether "/:~eggert" shall be expanded to
"/home/albinus/~eggert". I agree with you, that it shouldn't, the target
shall be just  "~eggert".

Using ":" in TARGET would look like:

--8<---------------cut here---------------start------------->8---
M-x make-symbolic-link RET /:/x:y RET /tmp/foo RET

# ls -l /tmp/foo
lrwxrwxrwx 1 albinus albinus 4 Aug 24 13:23 /tmp/foo -> /x:y
--8<---------------cut here---------------end--------------->8---

For a TARGET which looks like a remote file name, I propose to keep the
current behaviour (stripping the remote part). It shall be equivalent to
just using the local file name of TARGET. So we would have:

--8<---------------cut here---------------start------------->8---
M-x make-symbolic-link RET /sudo::/:~eggert RET /sudo::/tmp/bar RET

# ls -l /tmp/bar
lrwxrwxrwx 1 root root 7 Aug 24 13:26 /tmp/bar -> ~eggert

;; This example does not work yet.
M-x make-symbolic-link RET /:~eggert RET /sudo::/tmp/bar RET

# ls -l /tmp/bar
lrwxrwxrwx 1 root root 7 Aug 24 13:26 /tmp/bar -> ~eggert
--8<---------------cut here---------------end--------------->8---



--8<---------------cut here---------------start------------->8---
M-x make-symbolic-link RET /sudo::/:/x:y RET /sudo::/tmp/bar RET

# ls -l /tmp/foo
lrwxrwxrwx 1 root root 4 Aug 24 13:30 /tmp/bar -> /x:y

;; This example does not work yet.
M-x make-symbolic-link RET /:/x:y RET /sudo::/tmp/bar RET

# ls -l /tmp/foo
lrwxrwxrwx 1 root root 4 Aug 24 13:30 /tmp/bar -> /x:y
--8<---------------cut here---------------end--------------->8---

By this, you don't need to handle "~" special in make-symbolic-link, as
your patch proposes. After all, it is just proper file name quoting, and
fixing the bug mentioned above.

Best regards, Michael.

bug#28156: Emacs quietly munges symlink contents

[Paul Eggert]

[-- Attachment #1: Type: text/plain, Size: 2098 bytes --]

Michael Albinus wrote:

> it is Tramp's (almost undocumented) make-symbolic-link
> implementation to handle TARGET as well, and to strip the remote part of
> this. I wouldn't be surprised if existing packages will be broken if we
> change this.

That should not be a problem with the proposed patch, as it does not affect how 
Tramp handles TARGET when making a remote symlink.

> --8<---------------cut here---------------start------------->8---
> M-x make-symbolic-link RET /:~eggert RET /tmp/foo RET
> 
> # ls -l /tmp/foo
> lrwxrwxrwx 1 albinus albinus 21 Aug 24 13:19 /tmp/foo -> /home/albinus/~eggert
> --8<---------------cut here---------------end--------------->8---
> 
> It is questionable whether "/:~eggert" shall be expanded to
> "/home/albinus/~eggert". I agree with you, that it shouldn't, the target
> shall be just  "~eggert".

Yes, adding support for /: for interactive use sounds reasonable. I updated the 
patch to do that; please see attached.

> For a TARGET which looks like a remote file name, I propose to keep the
> current behaviour (stripping the remote part).

That's fine if LINKNAME is also remote, since symlinks act locally. That is, if 
TARGET and LINKNAME are both remote to the same filesystem, Tramp can continue 
to munge TARGET so that it works on that filesystem. However, Tramp should not 
be in the business of specifying symlink behavior for local symbolic links. It 
should let the OS do that. If LINKNAME is local, TARGET should just act as 
itself without Tramp getting in the way.

> it is just proper file name quoting

The target of a local symbolic link is a string. It need not name a file, and 
often does not name a file. Trying to apply some file name rules to it, and not 
others, is confusing and leads to broken code. For backward compatibility we 
appear to need to handle leading ~ in interactive use and to provide an escape 
for leading ~, but let's not try to inflict these complex rules on code. For 
code, make-symbolic-link should just treat a local target as a string, the way 
the OS does.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Do-not-munge-contents-of-local-symbolic-links.patch --]
[-- Type: text/x-patch; name="0001-Do-not-munge-contents-of-local-symbolic-links.patch", Size: 10153 bytes --]

From 743ab11913b9caa979f9dca8ab2578d9e3de9022 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Thu, 24 Aug 2017 16:22:12 -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.
* src/fileio.c (Fmake_symbolic_link): Do not expand leading "~" in the
target unless interactive.  Strip leading "/:" if interactive.
(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.
---
 doc/emacs/files.texi         |  8 ++++++--
 doc/lispref/files.texi       | 11 +++++++----
 etc/NEWS                     | 24 ++++++++++++++++++++++++
 src/fileio.c                 | 28 +++++++++-------------------
 test/lisp/net/tramp-tests.el |  9 ++++++---
 test/src/fileio-tests.el     | 21 ++++++++++-----------
 6 files changed, 62 insertions(+), 39 deletions(-)

diff --git a/doc/emacs/files.texi b/doc/emacs/files.texi
index 9195bc4..fa1f9e5 100644
--- a/doc/emacs/files.texi
+++ b/doc/emacs/files.texi
@@ -1611,8 +1611,12 @@ Copying and Naming
 @var{target} at the time the opening is done, or will get an error if
 the name @var{target} is nonexistent at that time.  This command does
 not expand the argument @var{target}, so that it allows you to specify
-a relative name as the target of the link.  On MS-Windows, this
-command works only on MS Windows Vista and later.  On remote systems,
+a relative name as the target of the link.  However, this command
+does expand leading @samp{~} in @var{target} so that you can easily
+specify home directories, and strips leading @samp{/:} so that you can
+specify relative names beginning with literal @samp{~} or @samp{/:}.
+@xref{Quoted File Names}.  On MS-Windows, this command works only on
+MS Windows Vista and later.  When @var{new} is remote,
 it works depending on the system type.
 
 @node Misc File Ops
diff --git a/doc/lispref/files.texi b/doc/lispref/files.texi
index 5a52765..054036d 100644
--- a/doc/lispref/files.texi
+++ b/doc/lispref/files.texi
@@ -1719,14 +1719,17 @@ Changing Files
 SELinux context are not copied over in either case.
 @end deffn
 
-@deffn Command make-symbolic-link filename newname &optional ok-if-already-exists
+@deffn Command make-symbolic-link target newname &optional ok-if-already-exists
 @pindex ln
 @kindex file-already-exists
-This command makes a symbolic link to @var{filename}, named
+This command makes a symbolic link to @var{target}, named
 @var{newname}.  This is like the shell command @samp{ln -s
-@var{filename} @var{newname}}.  The @var{filename} argument
+@var{target} @var{newname}}.  The @var{target} argument
 is treated only as a string; it need not name an existing file.
-If @var{filename} is a relative file name, the resulting symbolic link
+If @var{ok-if-already-exists} is an integer, indicating interactive
+use, then leading @samp{~} is expanded and leading @samp{/:} is
+stripped in the @var{target} string.
+If @var{target} is a relative file name, the resulting symbolic link
 is interpreted relative to the directory containing the symbolic link.
 @xref{Relative File Names}.
 
diff --git a/etc/NEWS b/etc/NEWS
index bf59749..60ddf54 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -1228,6 +1228,30 @@ 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, so that
+Emacs can access and copy them reliably regardless of their contents.
+The following changes are involved.
+
+*** 'file-attributes' and 'file-symlink-p' no longer prepend "/:" to
+symbolic links whose targets begin with "/" and contain ":".  For
+example, if a symbolic link "x" has a target "/y:z", (file-symlink-p
+"x") now returns "/y:z" rather than "/:/y:z".
+
+*** 'make-symbolic-link' no longer looks for file name handlers when
+creating a local symbolic link.  For example, (make-symbolic-link
+"/y:z" "x") now creates a symlink to "/y:z" instead of failing.
+
+*** 'make-symbolic-link' now expands a link target with leading "~"
+only when the optional third arg is an integer, as when invoked
+interactively.  For example, (make-symbolic-link "~y" "x") now creates
+a link with target the literal string "~y"; to get the old behavior,
+use (make-symbolic-link (expand-file-name "~y") "x").  To avoid this
+expansion in interactive use, you can now prefix the link target with
+"/:".  For example, (make-symbolic-link "/:~y" "x" 1) now creates a
+link to literal "~y".
+
++++
 ** 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 ca1bc50..1e4ccd4 100644
--- a/src/fileio.c
+++ b/src/fileio.c
@@ -2415,7 +2415,8 @@ DEFUN ("make-symbolic-link", Fmake_symbolic_link, Smake_symbolic_link, 2, 3,
 Both args must be strings.
 Signal a `file-already-exists' error if a file LINKNAME already exists
 unless optional third argument OK-IF-ALREADY-EXISTS is non-nil.
-An integer third arg means request confirmation if LINKNAME already exists.
+An integer third arg means request confirmation if LINKNAME already
+exists, and expand leading "~" or strip leading "/:" in TARGET.
 This happens for interactive use with M-x.  */)
   (Lisp_Object target, Lisp_Object linkname, Lisp_Object ok_if_already_exists)
 {
@@ -2423,21 +2424,15 @@ 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);
-
+  if (INTEGERP (ok_if_already_exists))
+    {
+      if (SREF (target, 0) == '~')
+	target = Fexpand_file_name (target, Qnil);
+      else if (SREF (target, 0) == '/' && SREF (target, 1) == ':')
+	target = Fsubstring_no_properties (target, make_number (2), 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 +2630,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 55f4b52..99a21f3 100644
--- a/test/lisp/net/tramp-tests.el
+++ b/test/lisp/net/tramp-tests.el
@@ -2590,13 +2590,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

bug#28156: Emacs quietly munges symlink contents

[Michael Albinus]

Paul Eggert <eggert@cs.ucla.edu> writes:

Hi Paul,

>> For a TARGET which looks like a remote file name, I propose to keep the
>> current behaviour (stripping the remote part).
>
> That's fine if LINKNAME is also remote, since symlinks act
> locally. That is, if TARGET and LINKNAME are both remote to the same
> filesystem, Tramp can continue to munge TARGET so that it works on
> that filesystem. However, Tramp should not be in the business of
> specifying symlink behavior for local symbolic links. It should let
> the OS do that. If LINKNAME is local, TARGET should just act as itself
> without Tramp getting in the way.

Tramp checks already in this case, that LINKNAME and TARGET belong to
the same remote filesystem. I just need to add the case, that TARGET is
used literally otherwise.

> diff --git a/etc/NEWS b/etc/NEWS
> index bf59749..60ddf54 100644
> --- a/etc/NEWS
> +++ b/etc/NEWS
> @@ -1228,6 +1228,30 @@ 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, so that
> +Emacs can access and copy them reliably regardless of their contents.
> +The following changes are involved.
> +
> +*** 'file-attributes' and 'file-symlink-p' no longer prepend "/:" to
> +symbolic links whose targets begin with "/" and contain ":".  For
> +example, if a symbolic link "x" has a target "/y:z", (file-symlink-p
> +"x") now returns "/y:z" rather than "/:/y:z".

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:". Otherwise, we would allow symbolic links over filesystem
borders inside Emacs, which would be a security problem.

Maybe we shall document it as well. Neither the docstring of
`file-truename', nor the Lisp reference say something about.

> diff --git a/test/lisp/net/tramp-tests.el b/test/lisp/net/tramp-tests.el
> index 55f4b52..99a21f3 100644
> --- a/test/lisp/net/tramp-tests.el
> +++ b/test/lisp/net/tramp-tests.el

You don't need to do this, I'll care for Tramp. Likely, I will add
further tests here.

Best regards, Michael.

bug#28156: Emacs quietly munges symlink contents

[Michael Albinus]

Michael Albinus <michael.albinus@gmx.de> writes:

Hi Paul,

>> That's fine if LINKNAME is also remote, since symlinks act
>> locally. That is, if TARGET and LINKNAME are both remote to the same
>> filesystem, Tramp can continue to munge TARGET so that it works on
>> that filesystem. However, Tramp should not be in the business of
>> specifying symlink behavior for local symbolic links. It should let
>> the OS do that. If LINKNAME is local, TARGET should just act as itself
>> without Tramp getting in the way.
>
> Tramp checks already in this case, that LINKNAME and TARGET belong to
> the same remote filesystem. I just need to add the case, that TARGET is
> used literally otherwise.

I've commited a patch to the repo which does it for Tramp. Well, there
are still some minor oddities when testing with the smb method, will fix
this the next days.

>> diff --git a/test/lisp/net/tramp-tests.el b/test/lisp/net/tramp-tests.el
>> index 55f4b52..99a21f3 100644
>> --- a/test/lisp/net/tramp-tests.el
>> +++ b/test/lisp/net/tramp-tests.el
>
> You don't need to do this, I'll care for Tramp. Likely, I will add
> further tests here.

Done also.

Best regards, Michael.

bug#28156: Emacs quietly munges symlink contents

[Paul Eggert]

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.

bug#28156: Emacs quietly munges symlink contents

[npostavs]

Eli Zaretskii <eliz@gnu.org> writes:

>> Callers of `make-symbolic-link' will need to expand the target themselves. 
>
> make-symbolic-link is an interactive command, not just a function.

Can we not just do the expansion in the interactive form then?

bug#28156: Emacs quietly munges symlink contents

[Paul Eggert]

npostavs@users.sourceforge.net wrote:
> Can we not just do the expansion in the interactive form then?

Yes, that makes sense. I'll modify the patch along those lines.