bug#30243: 26.0.91; Infinite recursion in `make-auto-save-file-name' for quoted filenames

bug#30243: 26.0.91; Infinite recursion in `make-auto-save-file-name' for quoted filenames

[phst]

emacs -Q -nw -batch --eval=3D'(with-temp-buffer (let ((buffer-file-name "/:=
/tmp/x")) (make-auto-save-file-name)))'
Variable binding depth exceeds max-specpdl-size

Since find-file calls after-find-file, which calls
make-auto-save-file-name, this can be trivially reproduced by visiting
any quoted file in Emacs 26, e.g. using C-x f /:/tmp/a.
I think this should block the Emacs 26 release because it makes quoted
file names essentially unusable.


In GNU Emacs 26.0.91 (build 3, x86_64-pc-linux-gnu, GTK+ Version 3.22.24)
  of 2018-01-24 built on localhost
Repository revision: 59db8dca030ba6a34d143c3cc6715f02beba1068
Windowing system distributor 'The X.Org Foundation', version 11.0.11903000
System Description:	Debian GNU/Linux

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

Configured using:
  'configure --without-threads --enable-gcc-warnings=3Dwarn-only
  --enable-gtk-deprecation-warnings --without-pop --with-mailutils
  --enable-checking --enable-check-lisp-object-type --with-modules
  'CFLAGS=3D-O0 -ggdb3''

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

Important settings:
   value of $LANG: en_US.UTF-8
   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 rmc 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 elec-pair 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 dbusbind 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 95204 8643)
  (symbols 48 20381 1)
  (miscs 40 41 145)
  (strings 32 28348 1660)
  (string-bytes 1 755236)
  (vectors 16 14065)
  (vector-slots 8 497282 8868)
  (floats 8 49 68)
  (intervals 56 223 0)
  (buffers 992 12))

--=20
Google Germany GmbH
Erika-Mann-Stra=C3=9Fe 33
80636 M=C3=BCnchen

Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg
Gesch=C3=A4ftsf=C3=BChrer: Paul Manicle, Halimah DeLaine Prado

If you received this communication by mistake, please don=E2=80=99t forward=
  it to
anyone else (it may contain confidential or privileged information), please
erase all copies of it, including all attachments, and please let the sender
know it went to the wrong person.  Thanks.

bug#30243: 26.0.91; Infinite recursion in `make-auto-save-file-name' for quoted filenames

[Philipp Stephani]

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

<phst@a.muc.corp.google.com> schrieb am Mi., 24. Jan. 2018 um 23:23 Uhr:

>
> emacs -Q -nw -batch --eval=3D'(with-temp-buffer (let ((buffer-file-name
> "/:=
> /tmp/x")) (make-auto-save-file-name)))'
> Variable binding depth exceeds max-specpdl-size
>
>
According to git bisect, the problematic commit is:

commit a1bbc490155b61a634a6d0b165000ce35b93aa35 (HEAD, refs/bisect/bad)
Author: Michael Albinus <michael.albinus@gmx.de>
Date:   Wed Dec 6 20:49:30 2017 +0100

    Fix Bug#29579

    * lisp/files.el (file-name-non-special):
    Inhibit `file-name-handler-alist' only for some operations.
    Add missing operations.  (Bug#29579)

[...]

which makes sense because it touches the part of the code that's causing
issues.
Since this commit was a bug fix for a related issue with quoted file names,
reverting it is probably not the best way forward. We should push a fix and
make a new pretest.

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

bug#30243: 26.0.91; Infinite recursion in `make-auto-save-file-name' for quoted filenames

[Noam Postavsky]

Philipp Stephani <p.stephani2@gmail.com> writes:

> Since this commit was a bug fix for a related issue with quoted file
> names, reverting it is probably not the best way forward. We should
> push a fix and make a new pretest.

The following seems to fix it.  We should review other file handler
operations of course, but can we really expect to learn anything from
another pretest?

--- i/lisp/files.el
+++ w/lisp/files.el
@@ -7004,6 +7004,11 @@ file-name-non-special
 	        (expand-file-name
 		 (unhandled-file-name-directory default-directory)))
 	     default-directory))
+         (buffer-file-name
+          (if (and (memq operation '(make-auto-save-file-name))
+                   (string-match "\\`/:" buffer-file-name))
+              (substring buffer-file-name (match-end 0))
+            buffer-file-name))
 	 ;; Get a list of the indices of the args which are file names.
 	 (file-arg-indices
 	  (cdr (or (assq operation

bug#30243: 26.0.91; Infinite recursion in `make-auto-save-file-name' for quoted filenames

[Philipp Stephani]

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

Noam Postavsky <npostavs@users.sourceforge.net> schrieb am Do., 25. Jan.
2018 um 00:04 Uhr:

> Philipp Stephani <p.stephani2@gmail.com> writes:
>
> > Since this commit was a bug fix for a related issue with quoted file
> > names, reverting it is probably not the best way forward. We should
> > push a fix and make a new pretest.
>
> The following seems to fix it.  We should review other file handler
> operations of course, but can we really expect to learn anything from
> another pretest?
>
> --- i/lisp/files.el
> +++ w/lisp/files.el
> @@ -7004,6 +7004,11 @@ file-name-non-special
>                 (expand-file-name
>                  (unhandled-file-name-directory default-directory)))
>              default-directory))
> +         (buffer-file-name
> +          (if (and (memq operation '(make-auto-save-file-name))
> +                   (string-match "\\`/:" buffer-file-name))
> +              (substring buffer-file-name (match-end 0))
> +            buffer-file-name))
>          ;; Get a list of the indices of the args which are file names.
>          (file-arg-indices
>           (cdr (or (assq operation
>

Thanks, when you commit this, could you please also add a regression test
so we don't reintroduce the bug later?

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

bug#30243: 26.0.91; Infinite recursion in `make-auto-save-file-name' for quoted filenames

[Noam Postavsky]

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

Philipp Stephani <p.stephani2@gmail.com> writes:

> Noam Postavsky <npostavs@users.sourceforge.net> schrieb am Do., 25.
> Jan. 2018 um 00:04 Uhr:

>     +         (buffer-file-name
>     +          (if (and (memq operation '(make-auto-save-file-name))
>     +                   (string-match "\\`/:" buffer-file-name))
>     +              (substring buffer-file-name (match-end 0))
>     +            buffer-file-name))

> Thanks, when you commit this, could you please also add a regression
> test so we don't reintroduce the bug later? 

Yeah, we need to test more than just that operation though.  I've added
a test for all the operations listed in `(elisp) Magic File Names'.  I
discovered several other similar problems.  With
`file-name-all-completions' I even got Emacs to crash (I presume due to
stack corruption during the recursion).  Also note that the above patch
breaks find-file for /: quoted files, buffer-file-name needs to be bound
more selectively (as in the new patch, attached below).

`file-notify-rm-watch' and `file-notify-valid-p' are still broken, I
can't immediately see how to fix them and it's getting past my bedtime.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: patch --]
[-- Type: text/x-diff, Size: 13580 bytes --]

From 5adeec4f3367ac5406e14c3b9c376a2cabc6d5ea Mon Sep 17 00:00:00 2001
From: Noam Postavsky <npostavs@gmail.com>
Date: Thu, 25 Jan 2018 00:37:50 -0500
Subject: [PATCH v2] Test and fix "/:" quoted file name handlers (Bug#30243)

* lisp/files.el (file-name-non-special): Strip the "/:" from
`default-directory' for `temporary-file-directory' operation; both
arguments to `file-name-completion', `file-name-all-completion', and
`file-equal-p' operations; `buffer-file-name' for
`make-auto-save-file-name' and 'set-visited-file-modtime' operations.
Don't touch any operands of `file-notify-rm-watch' and
`file-notify-valid-p' as they receive descriptors; not file
names (this is not sufficient to fix these operations for "/:" quoted
file names though).
* test/lisp/files-tests.el (files-tests--with-temp-dir): New macro.
(files-file-name-non-special-notify-handlers)
(files-file-name-non-special-handlers): New tests.
---
 lisp/files.el            |  21 +++++-
 test/lisp/files-tests.el | 186 +++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 203 insertions(+), 4 deletions(-)

diff --git a/lisp/files.el b/lisp/files.el
index 66420e7259..576640393f 100644
--- a/lisp/files.el
+++ b/lisp/files.el
@@ -6999,7 +6999,7 @@ file-name-non-special
            ;; Bug#25949.
 	   (if (memq operation
                      '(insert-directory process-file start-file-process
-                                        shell-command))
+                                        shell-command temporary-file-directory))
 	       (directory-file-name
 	        (expand-file-name
 		 (unhandled-file-name-directory default-directory)))
@@ -7023,15 +7023,22 @@ file-name-non-special
 			    ;; temporarily to unquoted filename.
 			    (verify-visited-file-modtime unquote-then-quote)
 			    ;; List the arguments which are filenames.
-			    (file-name-completion 1)
-			    (file-name-all-completions 1)
+			    (file-name-completion 0 1)
+			    (file-name-all-completions 0 1)
+                            (file-equal-p 0 1)
 			    (write-region 2 5)
 			    (rename-file 0 1)
 			    (copy-file 0 1)
 			    (copy-directory 0 1)
 			    (file-in-directory-p 0 1)
 			    (make-symbolic-link 0 1)
-			    (add-name-to-file 0 1))))
+			    (add-name-to-file 0 1)
+                            (make-auto-save-file-name buffer-file-name)
+                            (set-visited-file-modtime buffer-file-name)
+                            ;; These file-notify-* operations take a
+                            ;; descriptor.
+                            (file-notify-rm-watch . nil)
+                            (file-notify-valid-p . nil))))
 		   ;; For all other operations, treat the first argument only
 		   ;; as the file name.
 		   '(nil 0))))
@@ -7054,6 +7061,12 @@ file-name-non-special
     (pcase method
       (`identity (car arguments))
       (`add (file-name-quote (apply operation arguments)))
+      (`buffer-file-name
+       (let ((buffer-file-name
+              (if (string-match "\\`/:" buffer-file-name)
+                  (substring buffer-file-name (match-end 0))
+                buffer-file-name)))
+         (apply operation arguments)))
       (`insert-file-contents
        (let ((visit (nth 1 arguments)))
          (unwind-protect
diff --git a/test/lisp/files-tests.el b/test/lisp/files-tests.el
index 8dbfc2965c..4738a50b43 100644
--- a/test/lisp/files-tests.el
+++ b/test/lisp/files-tests.el
@@ -21,6 +21,9 @@
 
 (require 'ert)
 (require 'nadvice)
+(require 'bytecomp) ; `byte-compiler-base-file-name'.
+(require 'dired) ; `dired-uncache'.
+(require 'filenotify) ; `file-notify-add-watch'.
 
 ;; Set to t if the local variable was set, `query' if the query was
 ;; triggered.
@@ -286,6 +289,14 @@ files-tests--with-temp-file
          (progn ,@body)
        (delete-file ,name))))
 
+(defmacro files-tests--with-temp-dir (name &rest body)
+  (declare (indent 1))
+  (cl-check-type name symbol)
+  `(let ((,name (make-temp-file "emacs" t)))
+     (unwind-protect
+         (progn ,@body)
+       (delete-directory ,name t))))
+
 (ert-deftest files-tests--file-name-non-special--buffers ()
   "Check that Bug#25951 is fixed.
 We call `verify-visited-file-modtime' on a buffer visiting a file
@@ -327,6 +338,181 @@ files-tests--with-temp-file
                       `((verify-visited-file-modtime ,buffer-visiting-file)
                         (verify-visited-file-modtime nil))))))))
 
+(ert-deftest files-file-name-non-special-notify-handlers ()
+  :expected-result :failed
+  (files-tests--with-temp-file tmpfile
+    (let* ((nospecial (concat "/:" tmpfile))
+           (watch (file-notify-add-watch nospecial '(change) #'ignore)))
+      (should (file-notify-valid-p watch))
+      (file-notify-rm-watch watch))))
+
+
+(ert-deftest files-file-name-non-special-handlers ()
+  (files-tests--with-temp-file tmpfile
+    (files-tests--with-temp-dir tmpdir
+      (let ((nospecial (concat "/:" tmpfile))
+            (nospecial-dir (concat "/:" tmpdir)))
+        (should (null (access-file nospecial "test")))
+        (let ((newname (concat nospecial "add-name")))
+          (add-name-to-file nospecial newname)
+          (should (file-exists-p newname)))
+        (should (equal (byte-compiler-base-file-name nospecial)
+                       (byte-compiler-base-file-name tmpfile)))
+        (let ((newname (concat (directory-file-name nospecial-dir)
+                               "copy-dir")))
+          (copy-directory nospecial-dir newname)
+          (should (file-directory-p newname))
+          (delete-directory newname)
+          (should-not (file-directory-p newname)))
+        (let ((newname (concat (directory-file-name nospecial)
+                               "copy-file")))
+          (copy-file nospecial newname)
+          (should (file-exists-p newname))
+          (delete-file newname)
+          (should-not (file-exists-p newname)))
+        (should (equal (diff-latest-backup-file nospecial)
+                       (diff-latest-backup-file tmpfile)))
+        (should (equal (directory-file-name nospecial-dir)
+                       (concat "/:" (directory-file-name tmpdir))))
+        (should (equal (directory-files nospecial-dir)
+                       (directory-files tmpdir)))
+        (should (equal (directory-files-and-attributes nospecial-dir)
+                       (directory-files-and-attributes tmpdir)))
+        (dired-compress-file (dired-compress-file nospecial))
+        (dired-uncache nospecial-dir)
+        (should (equal (expand-file-name nospecial)
+                       nospecial))
+        (should (file-accessible-directory-p nospecial-dir))
+        (should (equal (file-acl nospecial)
+                       (file-acl tmpfile)))
+        (should (equal (file-attributes nospecial)
+                       (file-attributes tmpfile)))
+        (should (equal (file-directory-p nospecial-dir)
+                       (file-directory-p tmpdir)))
+        (should (file-equal-p nospecial tmpfile))
+        (should (file-equal-p tmpfile nospecial))
+        (should-not (file-executable-p nospecial))
+        (should (file-exists-p nospecial))
+        (should (file-in-directory-p nospecial temporary-file-directory))
+        (should-not (file-in-directory-p nospecial nospecial-dir))
+        (should-not (file-in-directory-p tmpfile nospecial-dir))
+        (should-not (file-local-copy nospecial)) ; Already local.
+        (should (equal (file-modes nospecial)
+                       (file-modes tmpfile)))
+        (should (equal (file-name-all-completions nospecial nospecial-dir)
+                       (file-name-all-completions tmpfile tmpdir)))
+        (file-name-as-directory nospecial)
+        (should (equal (file-name-case-insensitive-p nospecial)
+                       (file-name-case-insensitive-p tmpfile)))
+        (should (equal (file-name-completion nospecial nospecial-dir)
+                       (file-name-completion tmpfile tmpdir)))
+        (should (equal (file-name-directory nospecial)
+                       (concat "/:" temporary-file-directory)))
+        (should (equal (file-name-nondirectory nospecial)
+                       (file-name-nondirectory tmpfile)))
+        (should (equal (file-name-sans-versions nospecial)
+                       nospecial))
+        (should-not (file-newer-than-file-p nospecial tmpfile))
+        (should (equal (file-ownership-preserved-p nospecial)
+                       (file-ownership-preserved-p tmpfile)))
+        (should (file-readable-p nospecial))
+        (should (file-regular-p nospecial))
+        (should-not (file-remote-p nospecial))
+        (should (equal (file-selinux-context nospecial)
+                       (file-selinux-context tmpfile)))
+        (should-not (file-symlink-p nospecial))
+        (file-truename nospecial)
+        (should (file-writable-p nospecial))
+        (should (equal (find-backup-file-name nospecial)
+                       (mapcar (lambda (f) (concat "/:" f))
+                               (find-backup-file-name tmpfile))))
+        (should-not (get-file-buffer nospecial))
+        (should (equal (with-temp-buffer
+                         (insert-directory nospecial-dir nil)
+                         (buffer-string))
+                       (with-temp-buffer
+                         (insert-directory tmpdir nil)
+                         (buffer-string))))
+        (with-temp-buffer
+          (insert-file-contents nospecial)
+          (should (zerop (buffer-size))))
+        (should (load nospecial))
+        (save-current-buffer
+          (should (equal (prog2 (set-buffer (find-file-noselect nospecial))
+                             (make-auto-save-file-name)
+                           (kill-buffer))
+                         (prog2 (set-buffer (find-file-noselect tmpfile))
+                             (make-auto-save-file-name)
+                           (kill-buffer)))))
+        (let ((default-directory nospecial-dir))
+          (make-directory "dir")
+          (should (file-directory-p "dir"))
+          (delete-directory "dir")
+          (make-directory-internal "dir")
+          (should (file-directory-p "dir"))
+          (delete-directory "dir")
+          (let ((near-tmpfile (make-nearby-temp-file "file")))
+            (should (file-exists-p near-tmpfile))
+            (delete-file near-tmpfile)))
+        (let* ((linkname (expand-file-name "link" tmpdir))
+               (may-symlink (ignore-errors (make-symbolic-link tmpfile linkname)
+                                           t)))
+          (when may-symlink
+            (should (file-symlink-p linkname))
+            (delete-file linkname)
+            (let ((linkname (expand-file-name "link" nospecial-dir)))
+              (make-symbolic-link tmpfile linkname)
+              (should (file-symlink-p linkname))
+              (delete-file linkname))))
+        ;; `files-tests--file-name-non-special--subprocess' already
+        ;; tests `process-file'.
+        (rename-file nospecial (concat nospecial "x"))
+        (rename-file (concat nospecial "x") nospecial)
+        (rename-file tmpfile (concat nospecial "x"))
+        (rename-file (concat nospecial "x") nospecial)
+        (rename-file nospecial (concat tmpfile "x"))
+        (rename-file (concat nospecial "x") nospecial)
+        (set-file-acl nospecial (file-acl nospecial))
+        (set-file-modes nospecial (file-modes nospecial))
+        (set-file-selinux-context nospecial (file-selinux-context nospecial))
+        (set-file-times nospecial)
+        ;; `files-tests--file-name-non-special--buffers' already tests
+        ;; `verify-visited-file-modtime'.
+        (with-temp-buffer
+          (write-region nil nil nospecial nil :visit))
+        (save-current-buffer
+          (set-buffer (find-file-noselect nospecial))
+          (set-visited-file-modtime)
+          (kill-buffer))
+        (with-temp-buffer
+          (let ((default-directory nospecial-dir))
+            (shell-command (concat (shell-quote-argument
+                                    (concat invocation-directory invocation-name))
+                                   " --version")
+                           (current-buffer))
+            (goto-char (point-min))
+            (should (search-forward emacs-version nil t))))
+        (with-temp-buffer
+          (let ((default-directory nospecial-dir))
+            (let ((proc (start-file-process
+                         "emacs" (current-buffer)
+                         (concat invocation-directory invocation-name)
+                         "--version")))
+              (accept-process-output proc)
+              (goto-char (point-min))
+              (should (search-forward emacs-version nil t))
+              (kill-process proc)
+              (accept-process-output proc ))))
+        (let ((process-environment (cons "FOO=foo" process-environment)))
+          ;; The "/:" prevents substitution.
+          (equal (substitute-in-file-name nospecial) nospecial))
+        (let ((default-directory nospecial-dir))
+          (equal (temporary-file-directory) temporary-file-directory))
+        (equal (unhandled-file-name-directory nospecial-dir)
+               (file-name-as-directory tmpdir))
+        (should (equal (vc-registered nospecial)
+                       (vc-registered tmpfile)))))))
+
 (ert-deftest files-tests--insert-directory-wildcard-in-dir-p ()
   (let ((alist (list (cons "/home/user/*/.txt" (cons "/home/user/" "*/.txt"))
                      (cons "/home/user/.txt" nil)
-- 
2.11.0

bug#30243: 26.0.91; Infinite recursion in `make-auto-save-file-name' for quoted filenames

[Michael Albinus]

Noam Postavsky <npostavs@users.sourceforge.net> writes:

Hi Noam,

> Yeah, we need to test more than just that operation though.  I've added
> a test for all the operations listed in `(elisp) Magic File Names'.  I
> discovered several other similar problems.  With
> `file-name-all-completions' I even got Emacs to crash (I presume due to
> stack corruption during the recursion).  Also note that the above patch
> breaks find-file for /: quoted files, buffer-file-name needs to be bound
> more selectively (as in the new patch, attached below).
>
> `file-notify-rm-watch' and `file-notify-valid-p' are still broken, I
> can't immediately see how to fix them and it's getting past my bedtime.

Maybe I could be of some help? Pls commit your changes you have worked
on, and tell me which missing part I shall try to follow.

Best regards, Michael.

bug#30243: 26.0.91; Infinite recursion in `make-auto-save-file-name' for quoted filenames

[Noam Postavsky]

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

>> `file-notify-rm-watch' and `file-notify-valid-p' are still broken, I
>> can't immediately see how to fix them and it's getting past my bedtime.
>
> Maybe I could be of some help? Pls commit your changes you have worked
> on, and tell me which missing part I shall try to follow.

I pushed my patch to the scratch/nonspecial-handlers branch.  If you run

    make -C test files-tests SELECTOR='\"non-special\"'

you will see the relevant tests,
files-file-name-non-special-notify-handlers being the one which
currently fails.  Maybe it's just a matter of binding
file-name-handler-alist to nil again for those operations, not sure.

PS we should probably update test/Makefile.in to require less escaping
for SELECTOR.

bug#30243: 26.0.91; Infinite recursion in `make-auto-save-file-name' for quoted filenames

[Michael Albinus]

Noam Postavsky <npostavs@users.sourceforge.net> writes:

Hi Noam,

> I pushed my patch to the scratch/nonspecial-handlers branch.  If you run
>
>     make -C test files-tests SELECTOR='\"non-special\"'
>
> you will see the relevant tests,

Thanks.

> files-file-name-non-special-notify-handlers being the one which
> currently fails.  Maybe it's just a matter of binding
> file-name-handler-alist to nil again for those operations, not sure.

I tried to debug this interactively. First, I had to give the debug
property to files-tests--with-temp-file and files-tests--with-temp-dir,
in order to be able to debug interactively.

I've tried to instrument file-notify-valid-p for debugging, but I've got
the error

Invalid read syntax: "Expected lambda expression"

Strange. No idea what's up, could it be due to the when-let* macro?

Anyway, I've instrumented files-file-name-non-special-notify-handlers.
When running the test, it returns file-notify-add-watch as expected, but
when entering file-notify-valid-p, it returns the error

Lisp nesting exceeds 'max-lisp-eval-depth'

Emacs is frozen, and there is the error message in the shell Emacs is
started from:

emacs: malloc.c:2427: sysmalloc: Assertion `(old_top == initial_top (av) && old_size == 0) || ((unsigned long) (old_size) >= MINSIZE && prev_inuse (old_top) && ((unsigned long) old_end & (pagesize - 1)) == 0)' failed.
Fatal error 6: Aborted

No idea what's up. I'd say you've uncovered a bug :-)

I'll try to continue tomorrow, running Emacs under gdb, but for now I
have to stop (there's a life outside Emacs ...)

> PS we should probably update test/Makefile.in to require less escaping
> for SELECTOR.

Go on. But pls remember, that SELECTOR is not a string only, it can be
also a Lisp form.

Best regards, Michael.

bug#30243: 26.0.91; Infinite recursion in `make-auto-save-file-name' for quoted filenames

[Noam Postavsky]

On Thu, Jan 25, 2018 at 11:36 AM, Michael Albinus
<michael.albinus@gmx.de> wrote:

> Emacs is frozen, and there is the error message in the shell Emacs is
> started from:
>
> emacs: malloc.c:2427: sysmalloc: Assertion `(old_top == initial_top (av) && old_size == 0) || ((unsigned long) (old_size) >= MINSIZE && prev_inuse (old_top) && ((unsigned long) old_end & (pagesize - 1)) == 0)' failed.
> Fatal error 6: Aborted

Oh, that's what I got from `file-name-all-completions' before fixing
it. My guess is that it's due to stack overflow. Although it is a bit
strange to get heap corruption from stack usage, maybe a bad
interaction with the stack overflow recovery mechanism?

I'll take a look at the edebug problems later today.

bug#30243: 26.0.91; Infinite recursion in `make-auto-save-file-name' for quoted filenames

[Noam Postavsky]

Noam Postavsky <npostavs@users.sourceforge.net> writes:

> On Thu, Jan 25, 2018 at 11:36 AM, Michael Albinus
> <michael.albinus@gmx.de> wrote:
>
>> Emacs is frozen, and there is the error message in the shell Emacs is
>> started from:
>>
>> emacs: malloc.c:2427: sysmalloc: Assertion `(old_top == initial_top
>> (av) && old_size == 0) || ((unsigned long) (old_size) >= MINSIZE &&
>> prev_inuse (old_top) && ((unsigned long) old_end & (pagesize - 1))
>> == 0)' failed.
>> Fatal error 6: Aborted
>
> Oh, that's what I got from `file-name-all-completions' before fixing
> it. My guess is that it's due to stack overflow. Although it is a bit
> strange to get heap corruption from stack usage, maybe a bad
> interaction with the stack overflow recovery mechanism?

It's not related to stack overflow recovery, it still happens with
attempt-stack-overflow-recovery set to nil.  The problem appears to be
that we hit the limit in grow_specpdl(), and then call signal_error
which does Ffuncall and then record_in_backtrace writes to specdl, this
latter write is invalid since we failed to grow specdl before.  Thus
memory corruption, undefined behaviour, etc.

#0  0x000000000063999d in record_in_backtrace (function=XIL(0xd9ea380), args=0xffef5b188, nargs=2)
    at ../../src/eval.c:2096
#1  0x000000000063b8c9 in Ffuncall (nargs=3, args=0xffef5b180) at ../../src/eval.c:2746
#2  0x000000000063b320 in call2 (fn=XIL(0xd9ea380), arg1=XIL(0x5250), arg2=XIL(0x1161fc03))
    at ../../src/eval.c:2625
#3  0x00000000006381db in signal_or_quit (error_symbol=XIL(0x5250), data=XIL(0x1161fc03), 
    keyboard_quit=false) at ../../src/eval.c:1565
#4  0x000000000063806d in Fsignal (error_symbol=XIL(0x5250), data=XIL(0x1161fc03))
    at ../../src/eval.c:1514
#5  0x000000000057939a in xsignal (error_symbol=XIL(0x5250), data=XIL(0x1161fc03))
    at ../../src/lisp.h:3861
#6  0x0000000000638704 in signal_error (s=0x75e388 "Variable binding depth exceeds max-specpdl-size", 
    arg=XIL(0)) at ../../src/eval.c:1688
#7  0x00000000006398cd in grow_specpdl () at ../../src/eval.c:2080
(More stack frames follow...)

Lisp Backtrace:
"stringp" (0xfef5b398)
"file-name-non-special" (0xfef5bb58)
"file-newer-than-file-p" (0xfef5bf38)
"apply" (0xfef5c130)
"file-name-non-special" (0xfef5c6f8)
"file-newer-than-file-p" (0xfef5c998)
"apply" (0xfef5cb90)
"file-name-non-special" (0xfef5d158)
"file-newer-than-file-p" (0xfef5d3f8)
"apply" (0xfef5d5f0)
"file-name-non-special" (0xfef5dbb8)
"file-newer-than-file-p" (0xfef5de58)
"apply" (0xfef5e050)
[...]

> I'll take a look at the edebug problems later today.

Turns out this is just Bug#10577.  You can work around it by loading
subr-x before instrumenting forms which use when-let*.

bug#30243: 26.0.91; Infinite recursion in `make-auto-save-file-name' for quoted filenames

[Michael Albinus]

Noam Postavsky <npostavs@users.sourceforge.net> writes:

Hi Noam,

> Lisp Backtrace:
> "stringp" (0xfef5b398)
> "file-name-non-special" (0xfef5bb58)
> "file-newer-than-file-p" (0xfef5bf38)
> "apply" (0xfef5c130)
> "file-name-non-special" (0xfef5c6f8)
> "file-newer-than-file-p" (0xfef5c998)
> "apply" (0xfef5cb90)
> "file-name-non-special" (0xfef5d158)
> "file-newer-than-file-p" (0xfef5d3f8)
> "apply" (0xfef5d5f0)
> "file-name-non-special" (0xfef5dbb8)
> "file-newer-than-file-p" (0xfef5de58)
> "apply" (0xfef5e050)
> [...]

Thanks! I've fixed this in filenotify.el (in fact, the underlying watch
descriptor should not use quoted file names). Patch pushed to the branch.

In my interactive test, there was also another error:

--8<---------------cut here---------------start------------->8---
F files-file-name-non-special-handlers
    (ert-test-failed
     ((should
       (equal
        (find-backup-file-name nospecial)
        (mapcar
         (lambda ... ...)
         (find-backup-file-name tmpfile))))
      :form
      (equal
       ("/:/tmp/emacsqk0Dcl~")
       ("/:/tmp/emacsqk0Dcl.~1~"))
      :value nil :explanation
      (list-elt 0
                (arrays-of-different-length 19 22 "/:/tmp/emacsqk0Dcl~" "/:/tmp/emacsqk0Dcl.~1~" first-mismatch-at 18))))
--8<---------------cut here---------------end--------------->8---

I guess, it is because I have set `version-control' to t in my .emacs
(not investigated further).

Best regards, Michael.

bug#30243: 26.0.91; Infinite recursion in `make-auto-save-file-name' for quoted filenames

[Noam Postavsky]

On Fri, Jan 26, 2018 at 6:01 AM, Michael Albinus <michael.albinus@gmx.de> wrote:

> Thanks! I've fixed this in filenotify.el (in fact, the underlying watch
> descriptor should not use quoted file names). Patch pushed to the branch.

Ah, makes sense.

>       (equal
>        ("/:/tmp/emacsqk0Dcl~")
>        ("/:/tmp/emacsqk0Dcl.~1~"))

> I guess, it is because I have set `version-control' to t in my .emacs
> (not investigated further).

Hmm, couldn't reproduce here, but it looks like a bug (although not as
severe as the inf loop stuff).

I've tried running the tests on w32, and discovered I missed testing
file-newer-than-file-p with both args quoted. There's also some
apparently w32-specific trouble with dired-compress-file and
insert-directory (although looking at the code, I'm not entirely sure
why dired-compress-file passed on GNU/Linux). I pushed 2 more patches
to fix file-newer-than-file-p and avoid the test w32 errors.

I'm thinking also the massive test needs to be split up, as it's a bit
unwieldy. Having one test per operation almost seems like overkill,
but I think it will make it easier to check we've covered everything,
so I'll probably go with that.

bug#30243: 26.0.91; Infinite recursion in `make-auto-save-file-name' for quoted filenames

[Michael Albinus]

Noam Postavsky <npostavs@users.sourceforge.net> writes:

Hi Noam,

>>       (equal
>>        ("/:/tmp/emacsqk0Dcl~")
>>        ("/:/tmp/emacsqk0Dcl.~1~"))
>
>> I guess, it is because I have set `version-control' to t in my .emacs
>> (not investigated further).
>
> Hmm, couldn't reproduce here, but it looks like a bug (although not as
> severe as the inf loop stuff).

Well, if I start emacs -Q, the error does not appear. Hmm, maybe I will
track it later.

> I'm thinking also the massive test needs to be split up, as it's a bit
> unwieldy. Having one test per operation almost seems like overkill,
> but I think it will make it easier to check we've covered everything,
> so I'll probably go with that.

Yes, would be better. There shall also be cleanup; I've added a missing
delete-file which was missing.

The branch scratch/nonspecial-handlers is based on Emacs 26.0.91, do you
plan to merge it there? I feel a little bit uncomfortable with this,
because my fix in file-notify-add-watch didn't get heavy testing.

Best regards, Michael.

bug#30243: 26.0.91; Infinite recursion in `make-auto-save-file-name' for quoted filenames

[Noam Postavsky]

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

>> I'm thinking also the massive test needs to be split up, as it's a bit
>> unwieldy. Having one test per operation almost seems like overkill,
>> but I think it will make it easier to check we've covered everything,
>> so I'll probably go with that.
>
> Yes, would be better. There shall also be cleanup; I've added a missing
> delete-file which was missing.

Right, should all be done now.

> The branch scratch/nonspecial-handlers is based on Emacs 26.0.91, do you
> plan to merge it there?

Yeah, I agree with Phillip that breaking "/:" files for the release
would be quite a severe regression.

> I feel a little bit uncomfortable with this, because my fix in
> file-notify-add-watch didn't get heavy testing.

However, if you think the file-notify fix might possibly affect other
things, I think leaving that part out for 26.1 would be acceptable.

Now that the code is ready, perhaps Eli has something to say on where it
should go?

bug#30243: 26.0.91; Infinite recursion in `make-auto-save-file-name' for quoted filenames

[Michael Albinus]

Noam Postavsky <npostavs@users.sourceforge.net> writes:

>> Yes, would be better. There shall also be cleanup; I've added a missing
>> delete-file which was missing.
>
> Right, should all be done now.

I've pushed another fix, which uses file-name-quote{,d-p} instead of
messing with "/:". This is cleaner, and shall also be an example for
people who care about quoted file names.

Futhermore, I've added a small test, which demonstrates how to use these
functions.

>> I feel a little bit uncomfortable with this, because my fix in
>> file-notify-add-watch didn't get heavy testing.
>
> However, if you think the file-notify fix might possibly affect other
> things, I think leaving that part out for 26.1 would be acceptable.

LGTM.

Best regards, Michael.

bug#30243: 26.0.91; Infinite recursion in `make-auto-save-file-name' for quoted filenames

[Eli Zaretskii]

> From: Noam Postavsky <npostavs@users.sourceforge.net>
> Date: Sun, 28 Jan 2018 09:58:21 -0500
> Cc: 30243@debbugs.gnu.org, Philipp Stephani <p.stephani2@gmail.com>
> 
> > The branch scratch/nonspecial-handlers is based on Emacs 26.0.91, do you
> > plan to merge it there?
> 
> Yeah, I agree with Phillip that breaking "/:" files for the release
> would be quite a severe regression.
> 
> > I feel a little bit uncomfortable with this, because my fix in
> > file-notify-add-watch didn't get heavy testing.
> 
> However, if you think the file-notify fix might possibly affect other
> things, I think leaving that part out for 26.1 would be acceptable.
> 
> Now that the code is ready, perhaps Eli has something to say on where it
> should go?

I think to make up my mind I'd need a few words about each part of the
changes (with the exception of the test suite changes): why is each of
them needed, and what does it do to fix which part of the original
problem.

I also wonder how come we've succeeded to break quoted file names so
fundamentally -- what change did that, and why did we make it on the
release branch?

Thanks.

bug#30243: 26.0.91; Infinite recursion in `make-auto-save-file-name' for quoted filenames

[Michael Albinus]

Eli Zaretskii <eliz@gnu.org> writes:

Hi Eli,

> I think to make up my mind I'd need a few words about each part of the
> changes (with the exception of the test suite changes): why is each of
> them needed, and what does it do to fix which part of the original
> problem.

The change in `file-notify-valid-p' is not related to the original
problem; it was uncovered by the extensive tests Noam has written.

I have already proposed, not to push *this* change into the emacs-26
branch. It shall go to the master.

> Thanks.

Best regards, Michael.

bug#30243: 26.0.91; Infinite recursion in `make-auto-save-file-name' for quoted filenames

[Philipp Stephani]

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

Eli Zaretskii <eliz@gnu.org> schrieb am Di., 30. Jan. 2018 um 14:46 Uhr:

> > From: Noam Postavsky <npostavs@users.sourceforge.net>
> > Date: Sun, 28 Jan 2018 09:58:21 -0500
> > Cc: 30243@debbugs.gnu.org, Philipp Stephani <p.stephani2@gmail.com>
> >
> > > The branch scratch/nonspecial-handlers is based on Emacs 26.0.91, do
> you
> > > plan to merge it there?
> >
> > Yeah, I agree with Phillip that breaking "/:" files for the release
> > would be quite a severe regression.
> >
> > > I feel a little bit uncomfortable with this, because my fix in
> > > file-notify-add-watch didn't get heavy testing.
> >
> > However, if you think the file-notify fix might possibly affect other
> > things, I think leaving that part out for 26.1 would be acceptable.
> >
> > Now that the code is ready, perhaps Eli has something to say on where it
> > should go?
>
> I think to make up my mind I'd need a few words about each part of the
> changes (with the exception of the test suite changes): why is each of
> them needed, and what does it do to fix which part of the original
> problem.
>
> I also wonder how come we've succeeded to break quoted file names so
> fundamentally -- what change did that, and why did we make it on the
> release branch?
>
>
If my git bisect is correct, it was
commit a1bbc490155b61a634a6d0b165000ce35b93aa35 to fix Bug#29579. So by
fixing one bug we introduced another one :(

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

bug#30243: 26.0.91; Infinite recursion in `make-auto-save-file-name' for quoted filenames

[Noam Postavsky]

Philipp Stephani <p.stephani2@gmail.com> writes:

> If my git bisect is correct, it was
> commit a1bbc490155b61a634a6d0b165000ce35b93aa35 to fix Bug#29579. So
> by fixing one bug we introduced another one :(

Actually, looking at Bug#29579 again, it doesn't seem *that* bad, and as
far as I can tell, it has existed for a long time (still occurs back in
24.3).  So reverting that fix seems like a reasonable option too.  I can
confirm that doing so fixes this bug.

> Eli Zaretskii <eliz@gnu.org> schrieb am Di., 30. Jan. 2018 um
> 14:46 Uhr:
>    
>     I think to make up my mind I'd need a few words about each part of
>     the changes (with the exception of the test suite changes): why is
>     each of them needed, and what does it do to fix which part of the
>     original problem.

The file-name-non-special function handles all file-handler operations
for "/:" quoted files.  It has an alist inside, to decide which
arguments are filenames and so require the "/:" to be removed before
calling the real operation.  The alist was not complete, so for many
operations the "/:" would only be stripped from the first argument
(that's a fallback for operations not listed in the alist).

Up until the fix for Bug#29579 this didn't matter so much, because
file-name-non-special would bind file-name-handler-alist to nil, thus
preventing any further filenames from being passed to the handler
anyway.  So multi-filename-arg handlers were broken, but unobtrusively
so.  With the fix for Bug#29579, such handlers got stuck in infinite
recursion, as they kept getting called, not stripping the "/:" prefix,
thus getting called again, etc...

The proposed patch just fixes up this alist to finally list all the
arguments for all the handlers correctly:

@@ -7000,7 +7000,7 @@ file-name-non-special
            ;; Bug#25949.
 	   (if (memq operation
                      '(insert-directory process-file start-file-process
-                                        shell-command))
+                                        shell-command temporary-file-directory))
 	       (directory-file-name
 	        (expand-file-name
 		 (unhandled-file-name-directory default-directory)))
@@ -7024,15 +7024,23 @@ file-name-non-special
 			    ;; temporarily to unquoted filename.
 			    (verify-visited-file-modtime unquote-then-quote)
 			    ;; List the arguments which are filenames.
-			    (file-name-completion 1)
-			    (file-name-all-completions 1)
+			    (file-name-completion 0 1)
+			    (file-name-all-completions 0 1)
+                            (file-equal-p 0 1)
+                            (file-newer-than-file-p 0 1)
 			    (write-region 2 5)
 			    (rename-file 0 1)
 			    (copy-file 0 1)
 			    (copy-directory 0 1)
 			    (file-in-directory-p 0 1)
 			    (make-symbolic-link 0 1)
-			    (add-name-to-file 0 1))))
+			    (add-name-to-file 0 1)
+                            (make-auto-save-file-name buffer-file-name)
+                            (set-visited-file-modtime buffer-file-name)
+                            ;; These file-notify-* operations take a
+                            ;; descriptor.
+                            (file-notify-rm-watch . nil)
+                            (file-notify-valid-p . nil))))
 		   ;; For all other operations, treat the first argument only
 		   ;; as the file name.
 		   '(nil 0))))
@@ -7055,6 +7063,12 @@ file-name-non-special
     (pcase method
       (`identity (car arguments))
       (`add (file-name-quote (apply operation arguments)))
+      (`buffer-file-name
+       (let ((buffer-file-name
+              (if (string-match "\\`/:" buffer-file-name)
+                  (substring buffer-file-name (match-end 0))
+                buffer-file-name)))
+         (apply operation arguments)))
       (`insert-file-contents
        (let ((visit (nth 1 arguments)))
          (unwind-protect

>     I also wonder how come we've succeeded to break quoted file names
>     so fundamentally -- what change did that, and why did we make it
>     on the release branch?

IMO, the root cause is pretty clearly lack of adequate tests for this.
There are more than 60 file-handler operations; it's crazy to expect to
be able to make a correct change without an automated test that at least
exercises each one.

bug#30243: 26.0.91; Infinite recursion in `make-auto-save-file-name' for quoted filenames

[Eli Zaretskii]

> From: Noam Postavsky <npostavs@users.sourceforge.net>
> Cc: Eli Zaretskii <eliz@gnu.org>,  30243@debbugs.gnu.org,  michael.albinus@gmx.de
> Date: Tue, 30 Jan 2018 19:01:52 -0500
> 
> Philipp Stephani <p.stephani2@gmail.com> writes:
> 
> > If my git bisect is correct, it was
> > commit a1bbc490155b61a634a6d0b165000ce35b93aa35 to fix Bug#29579. So
> > by fixing one bug we introduced another one :(
> 
> Actually, looking at Bug#29579 again, it doesn't seem *that* bad, and as
> far as I can tell, it has existed for a long time (still occurs back in
> 24.3).  So reverting that fix seems like a reasonable option too.  I can
> confirm that doing so fixes this bug.

I tend to agree.  Michael, WDYT?

> >     I also wonder how come we've succeeded to break quoted file names
> >     so fundamentally -- what change did that, and why did we make it
> >     on the release branch?
> 
> IMO, the root cause is pretty clearly lack of adequate tests for this.
> There are more than 60 file-handler operations; it's crazy to expect to
> be able to make a correct change without an automated test that at least
> exercises each one.

Right, thanks.

So if Michael agrees, I think we should revert the fix for bug#29579
on the release branch, and merge the branch you two worked on onto
master.

bug#30243: 26.0.91; Infinite recursion in `make-auto-save-file-name' for quoted filenames

[Michael Albinus]

Eli Zaretskii <eliz@gnu.org> writes:

Hi Eli,

>> Actually, looking at Bug#29579 again, it doesn't seem *that* bad, and as
>> far as I can tell, it has existed for a long time (still occurs back in
>> 24.3).  So reverting that fix seems like a reasonable option too.  I can
>> confirm that doing so fixes this bug.
>
> I tend to agree.  Michael, WDYT?

Noam is right. The bug exist "since ever", and it wasn't detected in the
wild but when I was working on my tramp-tests. So it might be OK to
revert it in the emacs-26 branch.

> So if Michael agrees, I think we should revert the fix for bug#29579
> on the release branch, and merge the branch you two worked on onto
> master.

Agreed. I expect some broken tests in tramp-tests then (IIRC, copy-file
and rename-file were affected). I will skip those tests for Emacs 26,
immediately after the revert has happened. Pls ping me when it is
reverted.

Best regards, Michael.

bug#30243: 26.0.91; Infinite recursion in `make-auto-save-file-name' for quoted filenames

[Noam Postavsky]

On Wed, Jan 31, 2018 at 1:07 PM, Michael Albinus <michael.albinus@gmx.de> wrote:

> Agreed. I expect some broken tests in tramp-tests then (IIRC, copy-file
> and rename-file were affected). I will skip those tests for Emacs 26,
> immediately after the revert has happened. Pls ping me when it is
> reverted.

Actually, if it's not too much trouble, I think it would be better if
you did the reverting; it looks to me like not all of the commit is
directly related to this, and I think you're probably the best able to
untangle that.

bug#30243: 26.0.91; Infinite recursion in `make-auto-save-file-name' for quoted filenames

[Michael Albinus]

Noam Postavsky <npostavs@users.sourceforge.net> writes:

> Actually, if it's not too much trouble, I think it would be better if
> you did the reverting; it looks to me like not all of the commit is
> directly related to this, and I think you're probably the best able to
> untangle that.

Will do, but rather tomorrow. I'm just attending the monthly Berlin
Emacs Meetup.

Best regards, Michael.

bug#30243: 26.0.91; Infinite recursion in `make-auto-save-file-name' for quoted filenames

[Michael Albinus]

Noam Postavsky <npostavs@users.sourceforge.net> writes:

> Actually, if it's not too much trouble, I think it would be better if
> you did the reverting; it looks to me like not all of the commit is
> directly related to this, and I think you're probably the best able to
> untangle that.

I've pushed this to the emacs-26 branch. Philipp, could you pls check
whether everything is OK now for you?

Best regards, Michael.

bug#30243: 26.0.91; Infinite recursion in `make-auto-save-file-name' for quoted filenames

[Philipp Stephani]

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

Michael Albinus <michael.albinus@gmx.de> schrieb am Do., 1. Feb. 2018 um
15:01 Uhr:

> Noam Postavsky <npostavs@users.sourceforge.net> writes:
>
> > Actually, if it's not too much trouble, I think it would be better if
> > you did the reverting; it looks to me like not all of the commit is
> > directly related to this, and I think you're probably the best able to
> > untangle that.
>
> I've pushed this to the emacs-26 branch. Philipp, could you pls check
> whether everything is OK now for you?
>
>
Seems to work. I've tested both the batch command and visiting a quoted
file, they appear to work as expected. Thanks!

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

bug#30243: 26.0.91; Infinite recursion in `make-auto-save-file-name' for quoted filenames

[Michael Albinus]

Philipp Stephani <p.stephani2@gmail.com> writes:

Hi Philipp,

> Seems to work. I've tested both the batch command and visiting a
> quoted file, they appear to work as expected. Thanks! 

Thanks for checking. So we seem to be OK now with the emacs-26 branch.

I propose to close this bug after Noam has merged the
scratch/nonspecial-handlers branch into master, and the tests pass
successfully.

Best regards, Michael.

bug#30243: 26.0.91; Infinite recursion in `make-auto-save-file-name' for quoted filenames

[Noam Postavsky]

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

> Philipp Stephani <p.stephani2@gmail.com> writes:
>
> Hi Philipp,
>
>> Seems to work. I've tested both the batch command and visiting a
>> quoted file, they appear to work as expected. Thanks! 
>
> Thanks for checking. So we seem to be OK now with the emacs-26 branch.
>
> I propose to close this bug after Noam has merged the
> scratch/nonspecial-handlers branch into master, and the tests pass
> successfully.

I'v pushed to master.

[1: 65da409e41]: 2018-02-01 20:14:57 -0500
  Test and fix "/:" quoted file name handlers (Bug#30243)
  https://git.savannah.gnu.org/cgit/emacs.git/commit/?id=65da409e411a0cdfa1932d21ce8a7f87ceae9e25

[2: 00c65bcf4e]: 2018-02-01 20:15:11 -0500
  Use file-name-quote{,d-p} in files-tests.el
  https://git.savannah.gnu.org/cgit/emacs.git/commit/?id=00c65bcf4ee8ca4ce04ad46907de29c832b8310b

[3: e08de2bae2]: 2018-02-01 20:15:12 -0500
  Handle quoted file names in filenotify.el
  https://git.savannah.gnu.org/cgit/emacs.git/commit/?id=e08de2bae2a8e91c0245259dfcbfdca1d191a119

bug#30243: 26.0.91; Infinite recursion in `make-auto-save-file-name' for quoted filenames

[Michael Albinus]

Noam Postavsky <npostavs@users.sourceforge.net> writes:

Hi Noam,

> I'v pushed to master.
>
> [1: 65da409e41]: 2018-02-01 20:14:57 -0500
>   Test and fix "/:" quoted file name handlers (Bug#30243)
>   https://git.savannah.gnu.org/cgit/emacs.git/commit/?id=65da409e411a0cdfa1932d21ce8a7f87ceae9e25
>
> [2: 00c65bcf4e]: 2018-02-01 20:15:11 -0500
>   Use file-name-quote{,d-p} in files-tests.el
>   https://git.savannah.gnu.org/cgit/emacs.git/commit/?id=00c65bcf4ee8ca4ce04ad46907de29c832b8310b
>
> [3: e08de2bae2]: 2018-02-01 20:15:12 -0500
>   Handle quoted file names in filenotify.el
>   https://git.savannah.gnu.org/cgit/emacs.git/commit/?id=e08de2bae2a8e91c0245259dfcbfdca1d191a119

Thanks. I did rerun filenotify-tests.el interactively, after applying
(setq temporary-file-directory (file-name-quote temporary-file-directory))

And indeed, the tests for a quoted remote temporary-file-directory did
fail. Fixed now, and pushed to master.

I've also extended files-tests-file-name-non-special-quote-unquote with
some more quoting/unquoting scenarii.

Best regards, Michael.

bug#30243: 26.0.91; Infinite recursion in `make-auto-save-file-name' for quoted filenames

[Noam Postavsky]

tags 30243 fixed
close 30243 26.1
quit

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

> Thanks. I did rerun filenotify-tests.el interactively, after applying
> (setq temporary-file-directory (file-name-quote temporary-file-directory))
>
> And indeed, the tests for a quoted remote temporary-file-directory did
> fail. Fixed now, and pushed to master.
>
> I've also extended files-tests-file-name-non-special-quote-unquote with
> some more quoting/unquoting scenarii.

Okay, I think we can consider this bug done.

bug#30243: 26.0.91; Infinite recursion in `make-auto-save-file-name' for quoted filenames

[Eli Zaretskii]

> From: Philipp Stephani <p.stephani2@gmail.com>
> Date: Tue, 30 Jan 2018 19:22:34 +0000
> Cc: Noam Postavsky <npostavs@users.sourceforge.net>, michael.albinus@gmx.de, 
> 	30243@debbugs.gnu.org
> 
> If my git bisect is correct, it was commit a1bbc490155b61a634a6d0b165000ce35b93aa35 to fix Bug#29579.
> So by fixing one bug we introduced another one :(

Thanks.  yes, this happens to us all the time.

bug#30481: 26.0.91; infinite recursion + edebug = memory corruption

[Noam Postavsky]

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

Tags: patch

Picking up on a side issue from Bug#30243:

>>> emacs: malloc.c:2427: sysmalloc: Assertion `(old_top == initial_top
>>> (av) && old_size == 0) || ((unsigned long) (old_size) >= MINSIZE &&
>>> prev_inuse (old_top) && ((unsigned long) old_end & (pagesize - 1))
>>> == 0)' failed.
>>> Fatal error 6: Aborted

> The problem appears to be that we hit the limit in grow_specpdl(),
> and then call signal_error which does Ffuncall and then
> record_in_backtrace writes to specdl, this latter write is invalid
> since we failed to grow specdl before.  Thus memory corruption,
> undefined behaviour, etc.
>
> #0  0x000000000063999d in record_in_backtrace (function=XIL(0xd9ea380), args=0xffef5b188, nargs=2)
>     at ../../src/eval.c:2096
> #1  0x000000000063b8c9 in Ffuncall (nargs=3, args=0xffef5b180) at ../../src/eval.c:2746
> #2  0x000000000063b320 in call2 (fn=XIL(0xd9ea380), arg1=XIL(0x5250), arg2=XIL(0x1161fc03))
>     at ../../src/eval.c:2625
> #3  0x00000000006381db in signal_or_quit (error_symbol=XIL(0x5250), data=XIL(0x1161fc03), 
>     keyboard_quit=false) at ../../src/eval.c:1565
> #4  0x000000000063806d in Fsignal (error_symbol=XIL(0x5250), data=XIL(0x1161fc03))
>     at ../../src/eval.c:1514
> #5  0x000000000057939a in xsignal (error_symbol=XIL(0x5250), data=XIL(0x1161fc03))
>     at ../../src/lisp.h:3861
> #6  0x0000000000638704 in signal_error (s=0x75e388 "Variable binding depth exceeds max-specpdl-size", 
>     arg=XIL(0)) at ../../src/eval.c:1688
> #7  0x00000000006398cd in grow_specpdl () at ../../src/eval.c:2080
> (More stack frames follow...)

A simple reproducer from emacs -Q, C-u C-M-x on the following:

    (defun foo ()
      (let ((x 1))
        (foo)))

then evaluate (foo) and git 'g' to continue until the "Variable binding
depth exceeds max-specpdl-size" error.  At that point the memory
corruption has happened (verified with valgrind), although I found I had
to split window to actually trigger the malloc assertion.

The following patch solves the problem by not calling
signal-hook-function when the specpdl array is exhausted.  I think it
could be safe for emacs-26.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: patch --]
[-- Type: text/x-diff, Size: 1427 bytes --]

From c9a183b31dce87803dad3d5feccf561fe3f63c9b Mon Sep 17 00:00:00 2001
From: Noam Postavsky <npostavs@gmail.com>
Date: Thu, 15 Feb 2018 22:13:51 -0500
Subject: [PATCH v1] Avoid memory corruption with lisp stack overflow + edebug

If grow_specpdl fails due to outgrowing max_specpdl_size, it will
signal an error *before* growing the specpdl array.  Therefore, when
handling the signal, specpdl_ptr points past the end of the specpdl
array and any further use of of specpdl before unwinding (e.g., if
edebug binds signal-hook-function) will cause memory corruption.
* src/eval.c (signal_or_quit): Don't call `signal-hook-function' if
the specpdl_ptr is already past the end of the specpdl array.
---
 src/eval.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/src/eval.c b/src/eval.c
index e05a17f7b4..ca1eb84ff3 100644
--- a/src/eval.c
+++ b/src/eval.c
@@ -1553,7 +1553,10 @@ signal_or_quit (Lisp_Object error_symbol, Lisp_Object data, bool keyboard_quit)
 
   /* This hook is used by edebug.  */
   if (! NILP (Vsignal_hook_function)
-      && ! NILP (error_symbol))
+      && ! NILP (error_symbol)
+      /* Don't try to call a lisp function if we've already overflowed
+         the specpdl stack.  */
+      && specpdl_ptr < specpdl + specpdl_size)
     {
       /* Edebug takes care of restoring these variables when it exits.  */
       if (lisp_eval_depth + 20 > max_lisp_eval_depth)
-- 
2.11.0

bug#30481: 26.0.91; infinite recursion + edebug = memory corruption

[Eli Zaretskii]

> From: Noam Postavsky <npostavs@gmail.com>
> Date: Thu, 15 Feb 2018 22:38:10 -0500
> 
> The following patch solves the problem by not calling
> signal-hook-function when the specpdl array is exhausted.  I think it
> could be safe for emacs-26.

Please push to emacs-26, and thanks.

(Is it practical to have a test for this?)

bug#30481: 26.0.91; infinite recursion + edebug = memory corruption

[Noam Postavsky]

tags 30481 fixed
close 30481 26.1
quit

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Noam Postavsky <npostavs@gmail.com>
>> Date: Thu, 15 Feb 2018 22:38:10 -0500
>> 
>> The following patch solves the problem by not calling
>> signal-hook-function when the specpdl array is exhausted.  I think it
>> could be safe for emacs-26.
>
> Please push to emacs-26, and thanks.

Pushed (with test) [1: c352434ab8].

> (Is it practical to have a test for this?)

Yes, actually.  I initially had some trouble reproducing without
instrumenting a function with edebug, but now I see that's just because
a function which let-binds only a single variable hits
max-lisp-eval-depth before max-specpdl-size (edebug's intrumentation
adds more bindings per call).  Let-binding two variables allows to
trigger the bug with just

    (defun foo ()
      (let ((x 1)
            (y 2))
        (foo)))

    (let ((signal-hook-function #'ignore))
      (foo))

[1: c352434ab8]: 2018-02-16 22:13:34 -0500
  Avoid memory corruption with specpdl overflow + edebug (Bug#30481)
  https://git.savannah.gnu.org/cgit/emacs.git/commit/?id=c352434ab89617b48c7c1f29342a22e5a5685504