bug#28264: Accessing source directory through symlink produces false warnings

bug#28264: Accessing source directory through symlink produces false warnings

[Glenn Morris]

Package: emacs
Version: 26.0.50

Current master (cd0360f) on rhel7.

Emacs sources are in (eg) /tmp/emacs/git/master
cd ~
ln -s /tmp/emacs/git/master
~/master/src/emacs -Q

This results in the following new, spurious warnings:
    
    Warning: arch-dependent data dir '/:/tmp/emacs/git/master/lib-src/':
       No such file or directory
    Warning: Lisp directory '/:/tmp/emacs/git/master/lisp':
       No such file or directory
    
I would suspect recent changes to file name / symlink handling.

bug#28264: Accessing source directory through symlink produces false warnings

[Paul Eggert]

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

Thanks for the bug report. I reproduced the problem and installed the attached 
patch to fix it. It strikes me, though, that the code still won't work in other 
cases (e.g., symlinks containing ":"), and that Emacs probably has other 
instances of confusion between how it treats symlinks and how the OS treats 
them. I'll CC: this to Michael to give him a heads-up about this particular 
issue, as I'm not really up to speed on magic file names.

[-- Attachment #2: 0001-Silence-false-alarms-for-symlinks-to-sources.patch --]
[-- Type: text/x-patch, Size: 2516 bytes --]

From c1854b1d31e1b0a3a9e91ef41110a5fa77bedb31 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Mon, 28 Aug 2017 21:50:09 -0700
Subject: [PATCH] Silence false alarms for symlinks to sources

Problem reported by Glenn Morris (Bug#28264).
* lisp/files.el (files--splice-dirname-file): New function.
(file-truename, file-chase-links): Use it.
---
 lisp/files.el | 26 ++++++++++++++++++--------
 1 file changed, 18 insertions(+), 8 deletions(-)

diff --git a/lisp/files.el b/lisp/files.el
index b3eab29..5f55aa7 100644
--- a/lisp/files.el
+++ b/lisp/files.el
@@ -1153,6 +1153,21 @@ files--name-absolute-system-p
   (and (file-name-absolute-p file)
        (not (eq (aref file 0) ?~))))
 
+(defun files--splice-dirname-file (dirname file)
+  "Splice DIRNAME to FILE like the operating system would.
+If FILENAME is relative, return DIRNAME concatenated to FILE.
+Otherwise return FILE, quoted with `/:' if DIRNAME and FILE have
+different handlers; although this quoting is dubious if DIRNAME
+is remote, it is not clear what would be better.  This function
+differs from `expand-file-name' in that DIRNAME must be a
+directory name and leading `~' and `/:' are not special in FILE."
+  (if (files--name-absolute-system-p file)
+      (if (eq (find-file-name-handler dirname 'file-symlink-p)
+	      (find-file-name-handler file 'file-symlink-p))
+	  file
+	(concat "/:" file))
+    (concat dirname file)))
+
 (defun file-truename (filename &optional counter prev-dirs)
   "Return the truename of FILENAME.
 If FILENAME is not absolute, first expands it against `default-directory'.
@@ -1253,10 +1268,7 @@ file-truename
 		    ;; We can't safely use expand-file-name here
 		    ;; since target might look like foo/../bar where foo
 		    ;; is itself a link.  Instead, we handle . and .. above.
-		    (setq filename
-			  (concat (if (files--name-absolute-system-p target)
-				      "/:" dir)
-				  target)
+		    (setq filename (files--splice-dirname-file dir target)
 			  done nil)
 		  ;; No, we are done!
 		  (setq done t))))))))
@@ -1291,10 +1303,8 @@ file-chase-links
 		 (directory-file-name (file-name-directory newname))))
 	  ;; Now find the parent of that dir.
 	  (setq newname (file-name-directory newname)))
-	(setq newname (concat (if (files--name-absolute-system-p tem)
-				  "/:"
-				(file-name-directory newname))
-			      tem))
+	(setq newname (files--splice-dirname-file (file-name-directory newname)
+						  tem))
 	(setq count (1+ count))))
     newname))
 
-- 
2.7.4

bug#28264: Accessing source directory through symlink produces false warnings

[Michael Albinus]

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

Hi Paul,

> Thanks for the bug report. I reproduced the problem and installed the
> attached patch to fix it. It strikes me, though, that the code still
> won't work in other cases (e.g., symlinks containing ":"), and that
> Emacs probably has other instances of confusion between how it treats
> symlinks and how the OS treats them. I'll CC: this to Michael to give
> him a heads-up about this particular issue, as I'm not really up to
> speed on magic file names.

This special problem does not seem to affect Tramp, tramp-tests tell.

However, the following code is unclear to me:

> +	(concat "/:" file))

What, if file is already quoted? Shouldn't this be

        (file-name-quote file)

Note, that a similar problem I have fixed some days ago in commit
cc7530cae0.

Best regards, Michael.

bug#28264: Accessing source directory through symlink produces false warnings

[Paul Eggert]

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

Michael Albinus wrote:

> This special problem does not seem to affect Tramp, tramp-tests tell.

The problem I was thinking of does not seem to be covered by Tramp tests. If, 
for example, I do these shell commands:

$ ln -s "../penguin:motd" /tmp/foo
$ ls -l /tmp/foo
lrwxrwxrwx 1 eggert eggert 15 Aug 29 19:00 /tmp/foo -> ../penguin:motd

then (file-truename "/tmp/foo") returns "/penguin:motd" which is not /tmp/foo's 
true name as far as Emacs file-oriented commands are concerned. Admittedly this 
is an improvement over Emacs 25.2 where the same file-truename call ssh'es into 
penguin to resolve the name, which is a clear security issue. Still, it doesn't 
seem right, if file-truename is expected to quote its result if necessary.

Sorry about all this confusion, but I do not know the general principle that 
Emacs is supposed to be using with file names, and to some extent I fear that 
there isn't one alas.

> However, the following code is unclear to me:
> 
>> +	(concat "/:" file))
> 
> What, if file is already quoted? Shouldn't this be
> 
>          (file-name-quote file)

Quite possibly, and I'll take your word for it. I installed the attached.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Prefer-file-name-quote-to-concat.patch --]
[-- Type: text/x-patch; name="0001-Prefer-file-name-quote-to-concat.patch", Size: 1590 bytes --]

From 0eb9f21ded1b6aefcf0ac33f393b07ab6a543a05 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Tue, 29 Aug 2017 19:17:54 -0700
Subject: [PATCH] Prefer file-name-quote to concat "/:"

Suggested by Michael Albinus (Bug#28264#13).
* lisp/files.el (files--splice-dirname-file): Use file-name-quote
rather than attempting to do it by hand.
---
 lisp/files.el | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/lisp/files.el b/lisp/files.el
index 5f55aa7..7754be2 100644
--- a/lisp/files.el
+++ b/lisp/files.el
@@ -1156,16 +1156,16 @@ files--name-absolute-system-p
 (defun files--splice-dirname-file (dirname file)
   "Splice DIRNAME to FILE like the operating system would.
 If FILENAME is relative, return DIRNAME concatenated to FILE.
-Otherwise return FILE, quoted with `/:' if DIRNAME and FILE have
+Otherwise return FILE, quoted as needed if DIRNAME and FILE have
 different handlers; although this quoting is dubious if DIRNAME
-is remote, it is not clear what would be better.  This function
+is magic, it is not clear what would be better.  This function
 differs from `expand-file-name' in that DIRNAME must be a
 directory name and leading `~' and `/:' are not special in FILE."
   (if (files--name-absolute-system-p file)
       (if (eq (find-file-name-handler dirname 'file-symlink-p)
 	      (find-file-name-handler file 'file-symlink-p))
 	  file
-	(concat "/:" file))
+	(file-name-quote file))
     (concat dirname file)))
 
 (defun file-truename (filename &optional counter prev-dirs)
-- 
2.7.4

bug#28264: Accessing source directory through symlink produces false warnings

[Michael Albinus]

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

Hi Paul,

> The problem I was thinking of does not seem to be covered by Tramp
> tests. If, for example, I do these shell commands:
>
> $ ln -s "../penguin:motd" /tmp/foo
> $ ls -l /tmp/foo
> lrwxrwxrwx 1 eggert eggert 15 Aug 29 19:00 /tmp/foo -> ../penguin:motd
>
> then (file-truename "/tmp/foo") returns "/penguin:motd" which is not
> /tmp/foo's true name as far as Emacs file-oriented commands are
> concerned.

It is. "/penguin:motd" is not a remote file name by default (it would be
only with Tramp's simplified syntax). But this is rather academical;
"/penguin:motd:" would be a remote file name. So let's continue with
this example.

I've changed the link:

# ls -l /tmp/foo
lrwxrwxrwx 1 albinus albinus 16 Aug 30 10:10 /tmp/foo -> ../penguin:motd:

The problem in returning a remote file name still exists, maybe you
could check your change, again? (file-truename "/tmp/foo") goes remote
to "/penguin:motd:".

For Tramp, we have

(file-truename "/ssh::/tmp/foo") => "/ssh:hostname:/penguin:motd:"

This is not wrong, but I'd prefer to get its quoted variant
"/ssh:hostname:/:/penguin:motd:". Will work on this.

And I'll add your test to tramp-tests.el.

Best regards, Michael.