bug#27871: 26.0.50; Bad handling of unmounted directory

classic Classic list List threaded Threaded
15 messages Options
Reply | Threaded
Open this post in threaded view
|

bug#27871: 26.0.50; Bad handling of unmounted directory

Philipp Stephani

Create a FUSE mount point, e.g.

cd /tmp
mkdir a a/a b
touch a/a/a
bindfs -f a b

While bindfs(1) is running, cd to the mounted directory in a second
shell:

cd /tmp/b/a

Now kill bindfs, e.g. by hitting ^C in the first shell.  The second
shell will now be in an unmounted directory.  From that directory, start
Emacs:

$ emacs -Q -batch a
Apparent cycle of symbolic links for (unreachable)

or, with stack trace

$ emacs -Q -batch -f toggle-debug-on-error a
Debug on Error enabled globally
...
  file-truename("(unreachable)" (-1) (nil))
  file-truename("(unreachable)/a" (-1) (nil))
  file-truename("(unreachable)" (-1) (nil))
  file-truename("(unreachable)/a" (-1) (nil))
  file-truename("(unreachable)" (-1) (nil))
  file-truename("(unreachable)/a" (-1) (nil))
  file-truename("(unreachable)" (-1) (nil))
  file-truename("(unreachable)/a" (-1) (nil))
  file-truename("(unreachable)" (-1) (nil))
  file-truename("(unreachable)/a" (-1) (nil))
  file-truename("(unreachable)" (-1) (nil))
  file-truename("(unreachable)/a" (-1) (nil))
  file-truename("(unreachable)" (-1) (nil))
  file-truename("(unreachable)/a" (-1) (nil))
  file-truename("(unreachable)" (-1) (nil))
  file-truename("(unreachable)/a" (-1) (nil))
  file-truename("(unreachable)" (-1) (nil))
  file-truename("(unreachable)/a" (-1) (nil))
  file-truename("(unreachable)" (-1) (nil))
  file-truename("(unreachable)/a" (-1) (nil))
  file-truename("(unreachable)" (-1) (nil))
  file-truename("(unreachable)/a" (-1) (nil))
  file-truename("(unreachable)" (-1) (nil))
  file-truename("(unreachable)/a" (-1) (nil))
  file-truename("(unreachable)" (-1) (nil))
  file-truename("(unreachable)/a" (-1) (nil))
  file-truename("(unreachable)" (-1) (nil))
  file-truename("(unreachable)/a" (-1) (nil))
  file-truename("(unreachable)/a/(unreachable)" (-1) (nil))
  file-truename("(unreachable)/a/(unreachable)/a" (-1) (nil))
  file-truename("(unreachable)/a/(unreachable)/a/(unreachable)" (-1) (nil))
  file-truename("(unreachable)/a/(unreachable)/a/(unreachable)/a" (-1) (nil))
  file-truename("(unreachable)/a/(unreachable)/a/(unreachable)/a/(unreachable)" (-1) (nil))
  file-truename("(unreachable)/a/(unreachable)/a/(unreachable)/a/(unreachable)/a" (-1) (nil))
  file-truename("(unreachable)/a/(unreachable)/a/(unreachable)/a/(unreachable)/a/a")
  find-file-noselect("(unreachable)/a/(unreachable)/a/a")
  #f(compiled-function (displayable-buffers dir line column name) #<bytecode>)((nil) "(unreachable)/a/" (0) (0) "a")
  command-line-1(("-f" "toggle-debug-on-error" "a"))
  command-line()
  normal-top-level()

It seems like the logic for `default-directory' and/or `file-truename'
should be improved for unmounted filesystems.


In GNU Emacs 26.0.50 (build 1, x86_64-pc-linux-gnu, GTK+ Version 3.10.8)
 of 2017-07-29 built on unknown
Repository revision: 2c930d15f541761422a268cd2b5a7f5c11c9a00e
Windowing system distributor 'The X.Org Foundation', version 11.0.11803000
System Description: Ubuntu 14.04.5 LTS

Recent messages:
For information about GNU Emacs and the GNU system, type C-h C-a.

Configured using:
 'configure --with-modules --without-pop --with-mailutils
 --enable-checking --enable-check-lisp-object-type --enable-gcc-warnings
 'CFLAGS=-ggdb3 -O0''

Configured features:
XPM JPEG TIFF GIF PNG SOUND GSETTINGS NOTIFY GNUTLS FREETYPE XFT ZLIB
TOOLKIT_SCROLL_BARS GTK3 X11 MODULES

Important settings:
  value of $LANG: en_US.UTF-8
  value of $XMODIFIERS: @im=ibus
  locale-coding-system: utf-8-unix

Major mode: Lisp Interaction

Minor modes in effect:
  tooltip-mode: t
  global-eldoc-mode: t
  eldoc-mode: t
  electric-indent-mode: t
  mouse-wheel-mode: t
  tool-bar-mode: t
  menu-bar-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  font-lock-mode: t
  blink-cursor-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t
  line-number-mode: t
  transient-mark-mode: t

Load-path shadows:
None found.

Features:
(shadow sort mail-extr emacsbug message subr-x puny seq byte-opt gv
bytecomp byte-compile cconv cl-loaddefs cl-lib dired dired-loaddefs
format-spec rfc822 mml easymenu mml-sec password-cache epa derived epg
epg-config gnus-util rmail rmail-loaddefs mm-decode mm-bodies mm-encode
mail-parse rfc2231 mailabbrev gmm-utils mailheader sendmail rfc2047
rfc2045 ietf-drums mm-util mail-prsvr mail-utils time-date mule-util
tooltip eldoc electric uniquify ediff-hook vc-hooks lisp-float-type
mwheel term/x-win x-win term/common-win x-dnd tool-bar dnd fontset image
regexp-opt fringe tabulated-list replace newcomment text-mode elisp-mode
lisp-mode prog-mode register page menu-bar rfn-eshadow isearch timer
select scroll-bar mouse jit-lock font-lock syntax facemenu font-core
term/tty-colors frame cl-generic cham georgian utf-8-lang misc-lang
vietnamese tibetan thai tai-viet lao korean japanese eucjp-ms cp51932
hebrew greek romanian slovak czech european ethiopic indian cyrillic
chinese composite charscript charprop case-table epa-hook jka-cmpr-hook
help simple abbrev obarray minibuffer cl-preloaded nadvice loaddefs
button faces cus-face macroexp files text-properties overlay sha1 md5
base64 format env code-pages mule custom widget hashtable-print-readable
backquote inotify dynamic-setting system-font-setting
font-render-setting move-toolbar gtk x-toolkit x multi-tty
make-network-process emacs)

Memory information:
((conses 16 94115 11926)
 (symbols 48 20077 1)
 (miscs 40 38 119)
 (strings 32 28435 1945)
 (string-bytes 1 754739)
 (vectors 16 13893)
 (vector-slots 8 487465 12899)
 (floats 8 48 97)
 (intervals 56 205 0)
 (buffers 992 11)
 (heap 1024 19129 984))



Reply | Threaded
Open this post in threaded view
|

bug#27871: 26.0.50; Bad handling of unmounted directory

Philipp Stephani


Philipp <[hidden email]> schrieb am Sa., 29. Juli 2017 um 23:08 Uhr:

Create a FUSE mount point, e.g.

cd /tmp
mkdir a a/a b
touch a/a/a
bindfs -f a b

While bindfs(1) is running, cd to the mounted directory in a second
shell:

cd /tmp/b/a

Now kill bindfs, e.g. by hitting ^C in the first shell.  The second
shell will now be in an unmounted directory.  From that directory, start
Emacs:

$ emacs -Q -batch a
Apparent cycle of symbolic links for (unreachable)

or, with stack trace

$ emacs -Q -batch -f toggle-debug-on-error a
Debug on Error enabled globally
...
  file-truename("(unreachable)" (-1) (nil))
  file-truename("(unreachable)/a" (-1) (nil))
  file-truename("(unreachable)" (-1) (nil))
  file-truename("(unreachable)/a" (-1) (nil))
  file-truename("(unreachable)" (-1) (nil))
  file-truename("(unreachable)/a" (-1) (nil))
  file-truename("(unreachable)" (-1) (nil))
  file-truename("(unreachable)/a" (-1) (nil))
  file-truename("(unreachable)" (-1) (nil))
  file-truename("(unreachable)/a" (-1) (nil))
  file-truename("(unreachable)" (-1) (nil))
  file-truename("(unreachable)/a" (-1) (nil))
  file-truename("(unreachable)" (-1) (nil))
  file-truename("(unreachable)/a" (-1) (nil))
  file-truename("(unreachable)" (-1) (nil))
  file-truename("(unreachable)/a" (-1) (nil))
  file-truename("(unreachable)" (-1) (nil))
  file-truename("(unreachable)/a" (-1) (nil))
  file-truename("(unreachable)" (-1) (nil))
  file-truename("(unreachable)/a" (-1) (nil))
  file-truename("(unreachable)" (-1) (nil))
  file-truename("(unreachable)/a" (-1) (nil))
  file-truename("(unreachable)" (-1) (nil))
  file-truename("(unreachable)/a" (-1) (nil))
  file-truename("(unreachable)" (-1) (nil))
  file-truename("(unreachable)/a" (-1) (nil))
  file-truename("(unreachable)" (-1) (nil))
  file-truename("(unreachable)/a" (-1) (nil))
  file-truename("(unreachable)/a/(unreachable)" (-1) (nil))
  file-truename("(unreachable)/a/(unreachable)/a" (-1) (nil))
  file-truename("(unreachable)/a/(unreachable)/a/(unreachable)" (-1) (nil))
  file-truename("(unreachable)/a/(unreachable)/a/(unreachable)/a" (-1) (nil))
  file-truename("(unreachable)/a/(unreachable)/a/(unreachable)/a/(unreachable)" (-1) (nil))
  file-truename("(unreachable)/a/(unreachable)/a/(unreachable)/a/(unreachable)/a" (-1) (nil))
  file-truename("(unreachable)/a/(unreachable)/a/(unreachable)/a/(unreachable)/a/a")
  find-file-noselect("(unreachable)/a/(unreachable)/a/a")
  #f(compiled-function (displayable-buffers dir line column name) #<bytecode>)((nil) "(unreachable)/a/" (0) (0) "a")
  command-line-1(("-f" "toggle-debug-on-error" "a"))
  command-line()
  normal-top-level()

It seems like the logic for `default-directory' and/or `file-truename'
should be improved for unmounted filesystems.


Here's a patch. With the patch, the output is

 Error getting directory: Transport endpoint is not connected
Warning (initialization): Error setting default-directory
Ignoring relative file name (a) due to nil default-directory


0001-Treat-unreachable-current-directory-as-error.txt (2K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

bug#27871: 26.0.50; Bad handling of unmounted directory

Eli Zaretskii
> From: Philipp Stephani <[hidden email]>
> Date: Sat, 23 Sep 2017 10:19:19 +0000
>
> Here's a patch. With the patch, the output is
>
> Error getting directory: Transport endpoint is not connected
> Warning (initialization): Error setting default-directory
> Ignoring relative file name (a) due to nil default-directory

Thanks.

> +/* Return the current working directory.  The result should be freed
> +   with 'free'.  Return NULL on errors.  */
> +char *
> +emacs_get_current_dir_name (void)
> +{
> +  char *dir = emacs_get_current_dir_name_1 ();
> +  if (dir == NULL)
> +    return NULL;
> +  /* On Linux, getcwd and get_current_dir_name return a string
> +     starting with "(unreachable)" if the current directory doesn't
> +     exist, e.g. because it was unmounted.  Treat that as an error.
> +     See https://debbugs.gnu.org/cgi/bugreport.cgi?bug=27871.  */
> +  const char *prefix = "(unreachable)";
> +  size_t dir_len = strlen (dir);
> +  size_t prefix_len = strlen (prefix);
> +  if (dir_len >= prefix_len && strncmp (dir, prefix, prefix_len) == 0)
> +    {
> +      errno = ENOTCONN;
> +      return NULL;

What if there's a directory called literally "(unreachable)SOMETHING"?
This isn't disallowed on GNU/Linux, is it?  We should probably stat
the result if we see this prefix, before we declare it a failure, IMO.



Reply | Threaded
Open this post in threaded view
|

bug#27871: 26.0.50; Bad handling of unmounted directory

Andreas Schwab-2
On Sep 23 2017, Eli Zaretskii <[hidden email]> wrote:

>> +/* Return the current working directory.  The result should be freed
>> +   with 'free'.  Return NULL on errors.  */
>> +char *
>> +emacs_get_current_dir_name (void)
>> +{
>> +  char *dir = emacs_get_current_dir_name_1 ();
>> +  if (dir == NULL)
>> +    return NULL;
>> +  /* On Linux, getcwd and get_current_dir_name return a string
>> +     starting with "(unreachable)" if the current directory doesn't
>> +     exist, e.g. because it was unmounted.  Treat that as an error.
>> +     See https://debbugs.gnu.org/cgi/bugreport.cgi?bug=27871.  */
>> +  const char *prefix = "(unreachable)";
>> +  size_t dir_len = strlen (dir);
>> +  size_t prefix_len = strlen (prefix);
>> +  if (dir_len >= prefix_len && strncmp (dir, prefix, prefix_len) == 0)
>> +    {
>> +      errno = ENOTCONN;
>> +      return NULL;
>
> What if there's a directory called literally "(unreachable)SOMETHING"?

An absolute file name cannot start with "(unreachable)".

Andreas.

--
Andreas Schwab, [hidden email]
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."



Reply | Threaded
Open this post in threaded view
|

bug#27871: 26.0.50; Bad handling of unmounted directory

Philipp Stephani


Andreas Schwab <[hidden email]> schrieb am Sa., 23. Sep. 2017 um 13:30 Uhr:
On Sep 23 2017, Eli Zaretskii <[hidden email]> wrote:

>> +/* Return the current working directory.  The result should be freed
>> +   with 'free'.  Return NULL on errors.  */
>> +char *
>> +emacs_get_current_dir_name (void)
>> +{
>> +  char *dir = emacs_get_current_dir_name_1 ();
>> +  if (dir == NULL)
>> +    return NULL;
>> +  /* On Linux, getcwd and get_current_dir_name return a string
>> +     starting with "(unreachable)" if the current directory doesn't
>> +     exist, e.g. because it was unmounted.  Treat that as an error.
>> +     See https://debbugs.gnu.org/cgi/bugreport.cgi?bug=27871.  */
>> +  const char *prefix = "(unreachable)";
>> +  size_t dir_len = strlen (dir);
>> +  size_t prefix_len = strlen (prefix);
>> +  if (dir_len >= prefix_len && strncmp (dir, prefix, prefix_len) == 0)
>> +    {
>> +      errno = ENOTCONN;
>> +      return NULL;
>
> What if there's a directory called literally "(unreachable)SOMETHING"?

An absolute file name cannot start with "(unreachable)".


Yes, and getcwd and friends only return absolute filenames, and we only use $PWD if it's absolute, so anything except '/' or a drive letter can't be a prefix in the success case. I'll add a comment to that effect. 
Reply | Threaded
Open this post in threaded view
|

bug#27871: 26.0.50; Bad handling of unmounted directory

Eli Zaretskii
> From: Philipp Stephani <[hidden email]>
> Date: Sat, 23 Sep 2017 11:33:13 +0000
> Cc: [hidden email]
>
>  > What if there's a directory called literally "(unreachable)SOMETHING"?
>
>  An absolute file name cannot start with "(unreachable)".
>
> Yes, and getcwd and friends only return absolute filenames, and we only use $PWD if it's absolute, so
> anything except '/' or a drive letter can't be a prefix in the success case. I'll add a comment to that effect.

Then why not just check that the first character is something other
than '/'?



Reply | Threaded
Open this post in threaded view
|

bug#27871: 26.0.50; Bad handling of unmounted directory

Eli Zaretskii
> From: Eli Zaretskii <[hidden email]>
> Cc: [hidden email], [hidden email]
>
> Then why not just check that the first character is something other
> than '/'?

Or, more general, that the result fails file_name_absolute_p (which
would be more portable)?



Reply | Threaded
Open this post in threaded view
|

bug#27871: 26.0.50; Bad handling of unmounted directory

Philipp Stephani


Eli Zaretskii <[hidden email]> schrieb am Sa., 23. Sep. 2017 um 13:41 Uhr:
> From: Eli Zaretskii <[hidden email]>
> Cc: [hidden email], [hidden email]
>
> Then why not just check that the first character is something other
> than '/'?

Or, more general, that the result fails file_name_absolute_p (which
would be more portable)?

Sounds reasonable, I'll send a new patch. 
Reply | Threaded
Open this post in threaded view
|

bug#27871: [PATCH] Treat unreachable current directory as error

Philipp Stephani
Linux prefixes an unreachable (e.g. unmounted) current directory with
the special string "(unreachable)", cf. Bug#27871.  Treat such
directories as error because they wouldn't work anyway.

* src/sysdep.c (emacs_get_current_dir_name_1): Renamed from
emacs_get_current_dir_name.
(emacs_get_current_dir_name): Check for prefix "(unreachable)".
---
 src/sysdep.c | 27 +++++++++++++++++++++++----
 1 file changed, 23 insertions(+), 4 deletions(-)

diff --git a/src/sysdep.c b/src/sysdep.c
index 1e6e0d011b..efc0396c93 100644
--- a/src/sysdep.c
+++ b/src/sysdep.c
@@ -220,10 +220,8 @@ init_standard_fds (void)
   force_open (STDERR_FILENO, O_RDONLY);
 }
 
-/* Return the current working directory.  The result should be freed
-   with 'free'.  Return NULL on errors.  */
-char *
-emacs_get_current_dir_name (void)
+static char *
+emacs_get_current_dir_name_1 (void)
 {
 # if HAVE_GET_CURRENT_DIR_NAME && !BROKEN_GET_CURRENT_DIR_NAME
 #  ifdef HYBRID_MALLOC
@@ -283,6 +281,27 @@ emacs_get_current_dir_name (void)
   return buf;
 }
 
+/* Return the current working directory.  The result should be freed
+   with 'free'.  Return NULL on errors.  */
+char *
+emacs_get_current_dir_name (void)
+{
+  char *dir = emacs_get_current_dir_name_1 ();
+  if (dir == NULL)
+    return NULL;
+  /* On Linux, getcwd and get_current_dir_name return a string
+     starting with "(unreachable)" if the current directory doesn't
+     exist, e.g. because it was unmounted.  Treat that as an error.
+     See https://debbugs.gnu.org/cgi/bugreport.cgi?bug=27871.  */
+  if (!file_name_absolute_p (dir))
+    {
+      free (dir);
+      errno = ENOTCONN;
+      return NULL;
+    }
+  return dir;
+}
+
 
 /* Discard pending input on all input descriptors.  */
 
--
2.14.1




Reply | Threaded
Open this post in threaded view
|

bug#27871: [PATCH] Treat unreachable current directory as error

Paul Eggert
Philipp Stephani wrote:

> +  if (!file_name_absolute_p (dir))

That doesn't look right here, since leading '~' counts as absolute to
file_name_absolute_p, which is not what is wanted here.

> +      errno = ENOTCONN;

Why ENOTCONN? Shouldn't it be ENOENT? The failure has nothing to do with socket
connections.

Also, I'd feel a bit better if we apply the workaround only to the function that
has the problem.

How about the attached patch instead?

0001-Fix-bug-with-unmounted-directory-on-GNU-Linux.patch (1K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

bug#27871: [PATCH] Treat unreachable current directory as error

Eli Zaretskii
> From: Paul Eggert <[hidden email]>
> Date: Sat, 30 Sep 2017 17:00:06 -0700
> Cc: Philipp Stephani <[hidden email]>
>
> How about the attached patch instead?

Thanks, this LGTM.  I think we want this on the release branch.



Reply | Threaded
Open this post in threaded view
|

bug#27871: [PATCH] Treat unreachable current directory as error

Paul Eggert
On 10/05/2017 03:13 AM, Eli Zaretskii wrote:
> Thanks, this LGTM.  I think we want this on the release branch.

OK, thanks, installed, and closing this bug report.




Reply | Threaded
Open this post in threaded view
|

bug#27871: [PATCH] Treat unreachable current directory as error

Philipp Stephani
In reply to this post by Paul Eggert


Paul Eggert <[hidden email]> schrieb am So., 1. Okt. 2017 um 02:00 Uhr:
Philipp Stephani wrote:

> +  if (!file_name_absolute_p (dir))

That doesn't look right here, since leading '~' counts as absolute to
file_name_absolute_p, which is not what is wanted here.

That shouldn't matter because getcwd never returns a string starting with ~.
 

> +      errno = ENOTCONN;

Why ENOTCONN? Shouldn't it be ENOENT? The failure has nothing to do with socket
connections.

I think ENOTCONN is the error returned by stat(".") if the current directory doesn't exist. But I don't care much about the exact error number, any nonzero value should do the job.
 

Also, I'd feel a bit better if we apply the workaround only to the function that
has the problem.


All of the current directory functions exhibit this behavior, including getwd and getcwd, so you need to make sure they are also covered.
Reply | Threaded
Open this post in threaded view
|

bug#27871: [PATCH] Treat unreachable current directory as error

Paul Eggert
Philipp Stephani wrote:
> All of the current directory functions exhibit this behavior, including
> getwd and getcwd, so you need to make sure they are also covered.

Thanks for letting us know. I installed the 2nd attached patch, which addresses
this by making the patch behave more like what you originally proposed, while
still avoiding the need to use file_name_absolute_p (which is about Emacs file
names, not OS names). Also, I noticed a related memory leak and fixed that by
installing the 1st attached patch.

0001-src-xsmfns.c-x_session_initialize-Fix-memory-leak.patch (958 bytes) Download Attachment
0002-Improve-test-for-unreachable-dirs.patch (2K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

bug#27871: [PATCH] Treat unreachable current directory as error

Philipp Stephani


Paul Eggert <[hidden email]> schrieb am So., 8. Okt. 2017 um 08:06 Uhr:
Philipp Stephani wrote:
> All of the current directory functions exhibit this behavior, including
> getwd and getcwd, so you need to make sure they are also covered.

Thanks for letting us know. I installed the 2nd attached patch, which addresses
this by making the patch behave more like what you originally proposed, while
still avoiding the need to use file_name_absolute_p (which is about Emacs file
names, not OS names).

That should work, thanks. FTR, this behavior is documented in the Linux manpage: http://man7.org/linux/man-pages/man2/getcwd.2.html