bug#74208: 31.0.50; minibuffer read-file-name-default mutates global value of default-directory incorrectly

bug#74208: 31.0.50; minibuffer read-file-name-default mutates global value of default-directory incorrectly

[Madhu]

minibuffer.el: (read-file-name-default) has the following code:
```
                     (minibuffer-with-setup-hook
                         (lambda ()
                           (setq default-directory dir)
		   ...
			  (set-syntax-table minibuffer-local-filename-syntax))
```

This mutates the global binding of default-directory which is incorrect.

To demonstrate the problem, in emacs -Q

(insert "http://example.com")
(setq enable-recursive-minibuffers t)

position point at in the middle of the string  http://example.com
M-: (ffap)

;; while waiting for input
M-: default-directory
;; => default-directory is bound to "http://example.com"
M-: (shell-command "echo foo")
; ;=>
;; Debugger entered--Lisp error: (file-missing "Setting current directory" "No such file or directory" "http://example.com/")

I think this can be addressed by binding default-directory before
modifying it.

```
diff --git a/lisp/minibuffer.el b/lisp/minibuffer.el
--- a/lisp/minibuffer.el
+++ b/lisp/minibuffer.el
@@ -3671,6 +3671,7 @@ read-file-name-default
                               (expand-file-name dir))))
                     (minibuffer-with-setup-hook
                         (lambda ()
+                         (let ((default-directory default-directory))
                           (setq default-directory dir)
                           ;; When the first default in `minibuffer-default'
                           ;; duplicates initial input `insdef',
@@ -3689,7 +3690,7 @@ read-file-name-default
                                  (with-current-buffer
                                      (window-buffer (minibuffer-selected-window))
 				   (read-file-name--defaults dir initial))))
-			  (set-syntax-table minibuffer-local-filename-syntax))
+			  (set-syntax-table minibuffer-local-filename-syntax)))
                       (completing-read prompt 'read-file-name-internal
                                        pred require-match insdef
                                        'file-name-history default-filename)))
```

[I'm sending this before I forget so it can be reviewed, but I can
send this patch as as attachment if needed, and it'll take some more
time until I figure out how to write a test for it and test it.]

bug#74208: 31.0.50; minibuffer read-file-name-default mutates global value of default-directory incorrectly

[Eli Zaretskii]

> Date: Tue, 05 Nov 2024 07:39:24 +0530 (IST)
> From: Madhu <enometh@meer.net>
> 
> minibuffer.el: (read-file-name-default) has the following code:
> ```
>                      (minibuffer-with-setup-hook
>                          (lambda ()
>                            (setq default-directory dir)
> 		   ...
> 			  (set-syntax-table minibuffer-local-filename-syntax))
> ```
> 
> This mutates the global binding of default-directory which is incorrect.
> 
> To demonstrate the problem, in emacs -Q
> 
> (insert "http://example.com")
> (setq enable-recursive-minibuffers t)
> 
> position point at in the middle of the string  http://example.com
> M-: (ffap)
> 
> ;; while waiting for input
> M-: default-directory
> ;; => default-directory is bound to "http://example.com"
> M-: (shell-command "echo foo")
> ; ;=>
> ;; Debugger entered--Lisp error: (file-missing "Setting current directory" "No such file or directory" "http://example.com/")
> 
> I think this can be addressed by binding default-directory before
> modifying it.
> 
> ```
> diff --git a/lisp/minibuffer.el b/lisp/minibuffer.el
> --- a/lisp/minibuffer.el
> +++ b/lisp/minibuffer.el
> @@ -3671,6 +3671,7 @@ read-file-name-default
>                                (expand-file-name dir))))
>                      (minibuffer-with-setup-hook
>                          (lambda ()
> +                         (let ((default-directory default-directory))
>                            (setq default-directory dir)
>                            ;; When the first default in `minibuffer-default'
>                            ;; duplicates initial input `insdef',
> @@ -3689,7 +3690,7 @@ read-file-name-default
>                                   (with-current-buffer
>                                       (window-buffer (minibuffer-selected-window))
>  				   (read-file-name--defaults dir initial))))
> -			  (set-syntax-table minibuffer-local-filename-syntax))
> +			  (set-syntax-table minibuffer-local-filename-syntax)))
>                        (completing-read prompt 'read-file-name-internal
>                                         pred require-match insdef
>                                         'file-name-history default-filename)))
> ```
> 
> [I'm sending this before I forget so it can be reviewed, but I can
> send this patch as as attachment if needed, and it'll take some more
> time until I figure out how to write a test for it and test it.]

Stefan, any comments?

bug#74208: 31.0.50; minibuffer read-file-name-default mutates global value of default-directory incorrectly

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

> To demonstrate the problem, in emacs -Q
>
> (insert "http://example.com")
> (setq enable-recursive-minibuffers t)
>
> position point at in the middle of the string  http://example.com
> M-: (ffap)
>
> ;; while waiting for input
> M-: default-directory
> ;; => default-directory is bound to "http://example.com"
> M-: (shell-command "echo foo")
> ; ;=>
> ;; Debugger entered--Lisp error: (file-missing "Setting current directory" "No such file or directory" "http://example.com/")
>
> I think this can be addressed by binding default-directory before
> modifying it.

IIRC the `setq` is really there to set the value of `default-directory`
during the whole duration of the completion, not just during the setup hook.
This is needed/used if the user modifies the minibuffer's content to
hold only a relative file name, at which point we need that default
directory info in order to perform completions.

> ```
> diff --git a/lisp/minibuffer.el b/lisp/minibuffer.el
> --- a/lisp/minibuffer.el
> +++ b/lisp/minibuffer.el
> @@ -3671,6 +3671,7 @@ read-file-name-default
>                                (expand-file-name dir))))
>                      (minibuffer-with-setup-hook
>                          (lambda ()
> +                         (let ((default-directory default-directory))
>                            (setq default-directory dir)
>                            ;; When the first default in `minibuffer-default'
>                            ;; duplicates initial input `insdef',
> @@ -3689,7 +3690,7 @@ read-file-name-default
>                                   (with-current-buffer
>                                       (window-buffer (minibuffer-selected-window))
>  				   (read-file-name--defaults dir initial))))
> -			  (set-syntax-table minibuffer-local-filename-syntax))
> +			  (set-syntax-table minibuffer-local-filename-syntax)))
>                        (completing-read prompt 'read-file-name-internal
>                                         pred require-match insdef
>                                         'file-name-history default-filename)))
> ```

The let+setq could be simplified to

                        (let ((default-directory dir))

but none of the code within this `let` uses `default-directory` since
there are only calls to `car/cdr`, `setq`, and `set-syntax-table`, none
of which touch file names, so we may as well remove the
(setq default-directory dir) instead.

I'm leaning towards declaring the `shell-command` behavior above as
a feature rather than a bug.


        Stefan

bug#74208: 31.0.50; minibuffer read-file-name-default mutates global value of default-directory incorrectly

[Madhu]

[-- Attachment #1: Type: Text/Plain, Size: 1664 bytes --]

[snip: I hope the context can be seen on the bug-list]
*  Stefan Monnier  <jwvjzdcz9fp.fsf-monnier+emacs@gnu.org>
> IIRC the `setq` is really there to set the value of `default-directory`
> during the whole duration of the completion, not just during the setup hook.
> This is needed/used if the user modifies the minibuffer's content to
> hold only a relative file name, at which point we need that default
> directory info in order to perform completions.

Ah yes I missed that, (though I haven't worked out yet how it works)

> The let+setq could be simplified to
>
>                         (let ((default-directory dir))
>
> but none of the code within this `let` uses `default-directory` since
> there are only calls to `car/cdr`, `setq`, and `set-syntax-table`, none
> of which touch file names, so we may as well remove the
> (setq default-directory dir) instead.

I still think the default-difectory should be let-bound so the binding
is undone when the dynamic scope exits.  I have not worked out where
the default-directory is being restored to the original value, but i
know it is possible for the global value to be set incorrectly
(e.g. after a M-: (debug), quit - -in the recursive minibuffer which
would do a non-local exit.)


> I'm leaning towards declaring the `shell-command` behavior above as
> a feature rather than a bug.

We could avoid still binding default-directory to a
non-directory. Using the logic of expand-file-name, how about the
following:

[shouldn't binding it before the call to `minibuffer-with-setup-hook'
be equivalent to a top level let in a hypothetical
(with-minibuffer-setup-hook FN &body BODY), or setting it via FN]


[-- Attachment #2: 0001-minibuffer.el-read-file-name-default-avoid-setting-i.patch --]
[-- Type: Text/X-Patch, Size: 1527 bytes --]

From ff4a2e046a66f7b364ad570c7dce11dab4987155 Mon Sep 17 00:00:00 2001
From: Madhu <enometh@net.meer>
Date: Tue, 5 Nov 2024 07:19:55 +0530
Subject: [PATCH] minibuffer.el: (read-file-name-default): avoid setting
 incorrect default-directory

* minibuffer.el: (read-file-name-default): bind default-directory
(in dynamic scope) instead of modifying it. try to bind it to a valid
directory. (e.g. don't set it to a url when calling (ffap) at a url).
---
 lisp/minibuffer.el | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/lisp/minibuffer.el b/lisp/minibuffer.el
index c970752ec2a..672f7b2b318 100644
--- a/lisp/minibuffer.el
+++ b/lisp/minibuffer.el
@@ -3668,10 +3668,11 @@ read-file-name-default
                   ;; changing `default-directory' in the current buffer,
                   ;; we don't let-bind it.
                   (let ((dir (file-name-as-directory
-                              (expand-file-name dir))))
+                              (expand-file-name dir)))
+                        (default-directory (if (file-name-absolute-p dir)
+                                               dir default-directory)))
                     (minibuffer-with-setup-hook
                         (lambda ()
-                          (setq default-directory dir)
                           ;; When the first default in `minibuffer-default'
                           ;; duplicates initial input `insdef',
                           ;; reset `minibuffer-default' to nil.
-- 
2.46.0.27.gfa3b914457

bug#74208: 31.0.50; minibuffer read-file-name-default mutates global value of default-directory incorrectly

[Eli Zaretskii]

> Cc: 74208@debbugs.gnu.org
> Date: Sun, 10 Nov 2024 06:27:00 +0530 (IST)
> From: Madhu <enometh@meer.net>
> 
> --- a/lisp/minibuffer.el
> +++ b/lisp/minibuffer.el
> @@ -3668,10 +3668,11 @@ read-file-name-default
>                    ;; changing `default-directory' in the current buffer,
>                    ;; we don't let-bind it.
>                    (let ((dir (file-name-as-directory
> -                              (expand-file-name dir))))
> +                              (expand-file-name dir)))
> +                        (default-directory (if (file-name-absolute-p dir)
> +                                               dir default-directory)))

You probably meant to use let*, right?

But anyway, I don't understand the logic: expand-file-name always
returns an absolute file name, so the test will always succeed.  What
did I miss?

bug#74208: 31.0.50; minibuffer read-file-name-default mutates global value of default-directory incorrectly

[Madhu]

*  Eli Zaretskii <eliz@gnu.org> <86msi7ljb0.fsf@gnu.org>
Wrote on Sun, 10 Nov 2024 08:00:35 +0200
>> --- a/lisp/minibuffer.el
>> +++ b/lisp/minibuffer.el
>> @@ -3668,10 +3668,11 @@ read-file-name-default
>>                    ;; changing `default-directory' in the current buffer,
>>                    ;; we don't let-bind it.
>>                    (let ((dir (file-name-as-directory
>> -                              (expand-file-name dir))))
>> +                              (expand-file-name dir)))
>> +                        (default-directory (if (file-name-absolute-p dir)
>> +                                               dir default-directory)))
> You probably meant to use let*, right?

Oops. yes, of course.

> But anyway, I don't understand the logic: expand-file-name always
> returns an absolute file name, so the test will always succeed.  What
> did I miss?

for the example in the thread,
ffap.el:(ffap-read-file-or-url) calls read-file-name-default, via
              (funcall #'read-file-name-default prompt guess guess) =>

  read-file-name-default("Find file or URL: " "http://example.com" "http://example.com")

(read-file-name-default PROMPT &optional DIR DEFAULT-FILENAME
 MUSTMATCH INITIAL PREDICATE)

The problem is that the parameter DIR == "http://example.com" was
getting bound to default-directory with undesirable results. DIR a url
not a file name but a url. and since expand-file-name behaves like
this:

(expand-file-name "http://example.com" "~") ->  "http://example.com"

it can probably be used to determine that DIR is not a legit filename
and default directory should not be bound to it. I don't think it has
any consequences for the completion-system completion via
read-file-name, which would not work anyway (and completing urls in
this context say matching against a history list of urls would not
make sense either)

bug#74208: 31.0.50; minibuffer read-file-name-default mutates global value of default-directory incorrectly

[Eli Zaretskii]

> Date: Sun, 10 Nov 2024 12:30:07 +0530 (IST)
> Cc: monnier@iro.umontreal.ca, 74208@debbugs.gnu.org
> From: Madhu <enometh@meer.net>
> 
> >> -                              (expand-file-name dir))))
> >> +                              (expand-file-name dir)))
> >> +                        (default-directory (if (file-name-absolute-p dir)
> >> +                                               dir default-directory)))
> > You probably meant to use let*, right?
> 
> Oops. yes, of course.
> 
> > But anyway, I don't understand the logic: expand-file-name always
> > returns an absolute file name, so the test will always succeed.  What
> > did I miss?
> 
> for the example in the thread,
> ffap.el:(ffap-read-file-or-url) calls read-file-name-default, via
>               (funcall #'read-file-name-default prompt guess guess) =>
> 
>   read-file-name-default("Find file or URL: " "http://example.com" "http://example.com")
> 
> (read-file-name-default PROMPT &optional DIR DEFAULT-FILENAME
>  MUSTMATCH INITIAL PREDICATE)
> 
> The problem is that the parameter DIR == "http://example.com" was
> getting bound to default-directory with undesirable results. DIR a url
> not a file name but a url. and since expand-file-name behaves like
> this:
> 
> (expand-file-name "http://example.com" "~") ->  "http://example.com"

That's not what I see.  I see this:

  (expand-file-name "http://example.com" "~")
    => /my/home/directory/http:/example.com

In what version of Emacs do you see your result?
Is that in "emacs -Q"?

In any case, the code you propose calls

  (expand-file-name dir)

without the 2nd argument.

> it can probably be used to determine that DIR is not a legit filename
> and default directory should not be bound to it.

I don't see how expand-file-name could help here, see above.

bug#74208: 31.0.50; minibuffer read-file-name-default mutates global value of default-directory incorrectly

[Madhu]

*  Eli Zaretskii <eliz@gnu.org> <86h68fl86n.fsf@gnu.org>
Wrote on Sun, 10 Nov 2024 12:00:48 +0200
>   (expand-file-name "http://example.com" "~")
>     => /my/home/directory/http:/example.com
I get the same result.

> In what version of Emacs do you see your result?

I was evaluating it under edebug after calling edebug-defun on
read-file-name-default, and ivoking (ffap) on the url.

It returns the argument with or without the second parameter to
expand-file-name, and I was hoping I could count on this behaviour to
separate the urls from the files.

The behaviour of expand-file-name is apparenlty modified when it comes
to read-file-name-default, but I can't spot what's going on. ?????

bug#74208: 31.0.50; minibuffer read-file-name-default mutates global value of default-directory incorrectly

[Eli Zaretskii]

> Date: Sun, 10 Nov 2024 16:11:41 +0530 (IST)
> Cc: monnier@iro.umontreal.ca, 74208@debbugs.gnu.org
> From: Madhu <enometh@meer.net>
> 
> *  Eli Zaretskii <eliz@gnu.org> <86h68fl86n.fsf@gnu.org>
> Wrote on Sun, 10 Nov 2024 12:00:48 +0200
> >   (expand-file-name "http://example.com" "~")
> >     => /my/home/directory/http:/example.com
> I get the same result.
> 
> > In what version of Emacs do you see your result?
> 
> I was evaluating it under edebug after calling edebug-defun on
> read-file-name-default, and ivoking (ffap) on the url.
> 
> It returns the argument with or without the second parameter to
> expand-file-name, and I was hoping I could count on this behaviour to
> separate the urls from the files.
> 
> The behaviour of expand-file-name is apparenlty modified when it comes
> to read-file-name-default, but I can't spot what's going on. ?????

Perhaps because TRAMP was loaded?

bug#74208: 31.0.50; minibuffer read-file-name-default mutates global value of default-directory incorrectly

[Madhu]

*  Eli Zaretskii <eliz@gnu.org> <86ed3jl63j.fsf@gnu.org>
Wrote on Sun, 10 Nov 2024 12:45:52 +0200
>> I was evaluating it under edebug after calling edebug-defun on
>> read-file-name-default, and ivoking (ffap) on the url.
>>
>> It returns the argument with or without the second parameter to
>> expand-file-name, and I was hoping I could count on this behaviour to
>> separate the urls from the files.
>>
>> The behaviour of expand-file-name is apparenlty modified when it comes
>> to read-file-name-default, but I can't spot what's going on. ?????
>
> Perhaps because TRAMP was loaded?
It's because of the call to
 (ffap-read-file-or-url "foo" "https://example.com/")

which roughly does the equivalent of

(let ((file-name-handler-alist
       (cl-adjoin (cons ffap-url-regexp #'ffap--url-file-handler)
		  file-name-handler-alist)))
  (expand-file-name "https://example.com" "~"))

Maybe the idea will still work?

bug#74208: 31.0.50; minibuffer read-file-name-default mutates global value of default-directory incorrectly

[Eli Zaretskii]

> Date: Sun, 10 Nov 2024 16:47:17 +0530 (IST)
> Cc: monnier@iro.umontreal.ca, 74208@debbugs.gnu.org
> From: Madhu <enometh@meer.net>
> 
> *  Eli Zaretskii <eliz@gnu.org> <86ed3jl63j.fsf@gnu.org>
> Wrote on Sun, 10 Nov 2024 12:45:52 +0200
> >> I was evaluating it under edebug after calling edebug-defun on
> >> read-file-name-default, and ivoking (ffap) on the url.
> >>
> >> It returns the argument with or without the second parameter to
> >> expand-file-name, and I was hoping I could count on this behaviour to
> >> separate the urls from the files.
> >>
> >> The behaviour of expand-file-name is apparenlty modified when it comes
> >> to read-file-name-default, but I can't spot what's going on. ?????
> >
> > Perhaps because TRAMP was loaded?
> It's because of the call to
>  (ffap-read-file-or-url "foo" "https://example.com/")
> 
> which roughly does the equivalent of
> 
> (let ((file-name-handler-alist
>        (cl-adjoin (cons ffap-url-regexp #'ffap--url-file-handler)
> 		  file-name-handler-alist)))
>   (expand-file-name "https://example.com" "~"))
> 
> Maybe the idea will still work?

The idea being not to bind default-directory to the URL?  Doesn't ffap
need that?  If not, why does it override the file handlers?

bug#74208: 31.0.50; minibuffer read-file-name-default mutates global value of default-directory incorrectly

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

> for the example in the thread,
> ffap.el:(ffap-read-file-or-url) calls read-file-name-default, via
>               (funcall #'read-file-name-default prompt guess guess) =>
>
>   read-file-name-default("Find file or URL: " "http://example.com" "http://example.com")
>
> (read-file-name-default PROMPT &optional DIR DEFAULT-FILENAME
>  MUSTMATCH INITIAL PREDICATE)
>
> The problem is that the parameter DIR == "http://example.com" was
> getting bound to default-directory with undesirable results.

It can also have desirable results sometimes (depends on the URL at point
and what kind of support for URLs you have in your Emacs, admittedly).
Do you happen to have a more realistic scenario than your "echo foo"
that lets us better judge the severity of the problem, and maybe other
ways to solve the actual problem?


        Stefan

bug#74208: 31.0.50; minibuffer read-file-name-default mutates global value of default-directory incorrectly

[Madhu]

>> The problem is that the parameter DIR == "http://example.com" was
>> getting bound to default-directory with undesirable results.

> It can also have desirable results sometimes (depends on the URL at point
> and what kind of support for URLs you have in your Emacs, admittedly).
> Do you happen to have a more realistic scenario than your "echo foo"
> that lets us better judge the severity of the problem, and maybe other
> ways to solve the actual problem?

Are you saying it is ok for emacs to make all calls to `call-prcoess'
to unconditionally fail --- when emacs is waiting for input at the
minibuffer?  the calls to call-process can come from anywhere, from
timers, or from outside via emacsclient, etc.

I already posted that any hypothetical completion facilites are
singularly useless for the task of ffap (url) -- to get the url at
point into the minibuffer and maybe manually edit it.  If these
facilities exist and you are using them, I'd like to see an example.

bug#74208: 31.0.50; minibuffer read-file-name-default mutates global value of default-directory incorrectly

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

>>> The problem is that the parameter DIR == "http://example.com" was
>>> getting bound to default-directory with undesirable results.
>
>> It can also have desirable results sometimes (depends on the URL at point
>> and what kind of support for URLs you have in your Emacs, admittedly).
>> Do you happen to have a more realistic scenario than your "echo foo"
>> that lets us better judge the severity of the problem, and maybe other
>> ways to solve the actual problem?
>
> Are you saying it is ok for emacs to make all calls to `call-prcoess'
> to unconditionally fail --- when emacs is waiting for input at the
> minibuffer?  the calls to call-process can come from anywhere, from
> timers, or from outside via emacsclient, etc.

`default-directory` can point to random places, including non-existing
directories, HTTP places (e.g. if you enable `url-handler-mode`), so
code like timers need to take this possibility into account, yes.
The ffap minibuffer discussed here is just one of many other ways to get
into this situation.

> I already posted that any hypothetical completion facilites are
> singularly useless for the task of ffap (url) -- to get the url at
> point into the minibuffer and maybe manually edit it.  If these
> facilities exist and you are using them, I'd like to see an example.

diff --git a/lisp/url/url-handlers.el b/lisp/url/url-handlers.el
index 9edc7865a74..17013345c25 100644
--- a/lisp/url/url-handlers.el
+++ b/lisp/url/url-handlers.el
@@ -375,18 +375,48 @@ url-insert-file-contents-literally
     (kill-buffer buffer)
     nil))
 
-(defun url-file-name-completion (url _directory &optional _predicate)
-  ;; Even if it's not implemented, it's not an error to ask for completion,
-  ;; in case it's available (bug#14806).
-  ;; (error "Unimplemented")
-  url)
+(defun url-file-name-completion (url directory &optional predicate)
+  (let ((all (url-file-name-all-completions url directory)))
+    (if (null all)
+        ;; If `url' is the empty string, don't return nil, so as to prevent
+        ;; partial-completion from recursing into the parent directory.
+        (if (equal url "") url)
+      (try-completion url all predicate))))
 (put 'file-name-completion 'url-file-handlers #'url-file-name-completion)
 
-(defun url-file-name-all-completions (_file _directory)
-  ;; Even if it's not implemented, it's not an error to ask for completion,
-  ;; in case it's available (bug#14806).
-  ;; (error "Unimplemented")
-  nil)
+(defvar url-handler-temp-buf)
+
+(defun url-file-name-all-completions (file directory)
+  ;; FIXME: Cache the "directory" buffers between completion requests.
+  (let ((buf (get-file-buffer directory)))
+    (unless buf
+      (setq buf (ignore-errors (find-file-noselect directory)))
+      (when buf
+        (with-current-buffer buf
+          (set (make-local-variable 'url-handler-temp-buf) t))))
+    (when buf
+      (unwind-protect
+          (with-current-buffer buf
+            (save-excursion
+              (let ((all ())
+                    (case-fold-search t)
+                    ;; FIXME: Handle URL-quoting.
+                    (regexp (format "<a href=\"\\(%s[^\"]+\\)\"" file)))
+                (goto-char (point-min))
+                (while (re-search-forward regexp nil t)
+                  (let ((url (match-string 1)))
+                    (unless (string-match
+                             "\\`\\(?:\\.\\.\\|[#?/]\\|[-a-z]+:/\\)\\|" url)
+                      ;; It's a relative URL.
+                      (when (string-match "[#?]\\|/\\(.\\)" url)
+                        (setq url (substring url (or (match-beginning 1)
+                                                     (match-beginning 0)))))
+                      ;; FIXME: Handle URL-unquoting.
+                      (push url all))))
+                all)))
+        (and (buffer-live-p buf)
+             (buffer-local-value 'url-handler-temp-buf buf)
+             (kill-buffer buf))))))
 (put 'file-name-all-completions
      'url-file-handlers #'url-file-name-all-completions)
 

bug#74208: 31.0.50; minibuffer read-file-name-default mutates global value of default-directory incorrectly

[Eli Zaretskii]

Ping! How should we make progress with this issue?

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: eliz@gnu.org,  74208@debbugs.gnu.org
> Date: Sun, 10 Nov 2024 22:40:27 -0500
> 
> >>> The problem is that the parameter DIR == "http://example.com" was
> >>> getting bound to default-directory with undesirable results.
> >
> >> It can also have desirable results sometimes (depends on the URL at point
> >> and what kind of support for URLs you have in your Emacs, admittedly).
> >> Do you happen to have a more realistic scenario than your "echo foo"
> >> that lets us better judge the severity of the problem, and maybe other
> >> ways to solve the actual problem?
> >
> > Are you saying it is ok for emacs to make all calls to `call-prcoess'
> > to unconditionally fail --- when emacs is waiting for input at the
> > minibuffer?  the calls to call-process can come from anywhere, from
> > timers, or from outside via emacsclient, etc.
> 
> `default-directory` can point to random places, including non-existing
> directories, HTTP places (e.g. if you enable `url-handler-mode`), so
> code like timers need to take this possibility into account, yes.
> The ffap minibuffer discussed here is just one of many other ways to get
> into this situation.
> 
> > I already posted that any hypothetical completion facilites are
> > singularly useless for the task of ffap (url) -- to get the url at
> > point into the minibuffer and maybe manually edit it.  If these
> > facilities exist and you are using them, I'd like to see an example.
> 
> diff --git a/lisp/url/url-handlers.el b/lisp/url/url-handlers.el
> index 9edc7865a74..17013345c25 100644
> --- a/lisp/url/url-handlers.el
> +++ b/lisp/url/url-handlers.el
> @@ -375,18 +375,48 @@ url-insert-file-contents-literally
>      (kill-buffer buffer)
>      nil))
>  
> -(defun url-file-name-completion (url _directory &optional _predicate)
> -  ;; Even if it's not implemented, it's not an error to ask for completion,
> -  ;; in case it's available (bug#14806).
> -  ;; (error "Unimplemented")
> -  url)
> +(defun url-file-name-completion (url directory &optional predicate)
> +  (let ((all (url-file-name-all-completions url directory)))
> +    (if (null all)
> +        ;; If `url' is the empty string, don't return nil, so as to prevent
> +        ;; partial-completion from recursing into the parent directory.
> +        (if (equal url "") url)
> +      (try-completion url all predicate))))
>  (put 'file-name-completion 'url-file-handlers #'url-file-name-completion)
>  
> -(defun url-file-name-all-completions (_file _directory)
> -  ;; Even if it's not implemented, it's not an error to ask for completion,
> -  ;; in case it's available (bug#14806).
> -  ;; (error "Unimplemented")
> -  nil)
> +(defvar url-handler-temp-buf)
> +
> +(defun url-file-name-all-completions (file directory)
> +  ;; FIXME: Cache the "directory" buffers between completion requests.
> +  (let ((buf (get-file-buffer directory)))
> +    (unless buf
> +      (setq buf (ignore-errors (find-file-noselect directory)))
> +      (when buf
> +        (with-current-buffer buf
> +          (set (make-local-variable 'url-handler-temp-buf) t))))
> +    (when buf
> +      (unwind-protect
> +          (with-current-buffer buf
> +            (save-excursion
> +              (let ((all ())
> +                    (case-fold-search t)
> +                    ;; FIXME: Handle URL-quoting.
> +                    (regexp (format "<a href=\"\\(%s[^\"]+\\)\"" file)))
> +                (goto-char (point-min))
> +                (while (re-search-forward regexp nil t)
> +                  (let ((url (match-string 1)))
> +                    (unless (string-match
> +                             "\\`\\(?:\\.\\.\\|[#?/]\\|[-a-z]+:/\\)\\|" url)
> +                      ;; It's a relative URL.
> +                      (when (string-match "[#?]\\|/\\(.\\)" url)
> +                        (setq url (substring url (or (match-beginning 1)
> +                                                     (match-beginning 0)))))
> +                      ;; FIXME: Handle URL-unquoting.
> +                      (push url all))))
> +                all)))
> +        (and (buffer-live-p buf)
> +             (buffer-local-value 'url-handler-temp-buf buf)
> +             (kill-buffer buf))))))
>  (put 'file-name-all-completions
>       'url-file-handlers #'url-file-name-all-completions)
>  
> 
> 

bug#74208: 31.0.50; minibuffer read-file-name-default mutates global value of default-directory incorrectly

[Madhu]

*  Eli Zaretskii <eliz@gnu.org> <864j3ym915.fsf@gnu.org>
Wrote on Sat, 23 Nov 2024 14:25:42 +0200

> Ping! How should we make progress with this issue?

I haven't had a chance to use Stefan's url history code yet to see how
it works, but hopefully i'll get around to it soon.  But I don't think
it is acceptable for call process to fail just because
default-directory is bound to a url. (There are other problems like
bogus invalid entries in file-name-history -- resulting from a merging
a relative pathname to a url) which also argue against setting the
default-url in general (it may make sense in a specific use case)

bug#74208: 31.0.50; minibuffer read-file-name-default mutates global value of default-directory incorrectly

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

> Ping! How should we make progress with this issue?

I think the core of this bug report is that `shell-command` can fail
when `default-directory` is set to a value that doesn't correspond to
a local directory.

At least for things like `(shell-command "date")` this is undesirable.

Maybe `call-process` should try to "look for" a valid directory if
`default-directory` isn't good enough?


        Stefan

bug#74208: 31.0.50; minibuffer read-file-name-default mutates global value of default-directory incorrectly

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: enometh@meer.net,  74208@debbugs.gnu.org
> Date: Sat, 23 Nov 2024 22:00:16 -0500
> 
> > Ping! How should we make progress with this issue?
> 
> I think the core of this bug report is that `shell-command` can fail
> when `default-directory` is set to a value that doesn't correspond to
> a local directory.
> 
> At least for things like `(shell-command "date")` this is undesirable.
> 
> Maybe `call-process` should try to "look for" a valid directory if
> `default-directory` isn't good enough?

It already does:

  Lisp_Object
  get_current_directory (bool encode)
  {
    Lisp_Object curdir = BVAR (current_buffer, directory);
    Lisp_Object dir = Funhandled_file_name_directory (curdir);

    /* If the file name handler says that dir is unreachable, use
       a sensible default. */
    if (NILP (dir))
      dir = build_string ("~");

I guess this case somehow evades the test?

bug#74208: 31.0.50; minibuffer read-file-name-default mutates global value of default-directory incorrectly

[Eli Zaretskii]

Ping! Do we want to do anything about this issue?

> Cc: enometh@meer.net, 74208@debbugs.gnu.org
> Date: Sun, 24 Nov 2024 09:19:47 +0200
> From: Eli Zaretskii <eliz@gnu.org>
> 
> > From: Stefan Monnier <monnier@iro.umontreal.ca>
> > Cc: enometh@meer.net,  74208@debbugs.gnu.org
> > Date: Sat, 23 Nov 2024 22:00:16 -0500
> > 
> > > Ping! How should we make progress with this issue?
> > 
> > I think the core of this bug report is that `shell-command` can fail
> > when `default-directory` is set to a value that doesn't correspond to
> > a local directory.
> > 
> > At least for things like `(shell-command "date")` this is undesirable.
> > 
> > Maybe `call-process` should try to "look for" a valid directory if
> > `default-directory` isn't good enough?
> 
> It already does:
> 
>   Lisp_Object
>   get_current_directory (bool encode)
>   {
>     Lisp_Object curdir = BVAR (current_buffer, directory);
>     Lisp_Object dir = Funhandled_file_name_directory (curdir);
> 
>     /* If the file name handler says that dir is unreachable, use
>        a sensible default. */
>     if (NILP (dir))
>       dir = build_string ("~");
> 
> I guess this case somehow evades the test?
> 
> 
> 
> 

bug#74208: 31.0.50; minibuffer read-file-name-default mutates global value of default-directory incorrectly

[Madhu]

*  Eli Zaretskii <eliz@gnu.org> <8634izk7ky.fsf@gnu.org>
Wrote on Sat, 07 Dec 2024 14:20:45 +0200
> Ping! Do we want to do anything about this issue?

is "Keep it open" an option? :)

call-process calls get_current_directory (which you exhibited) which
in turn calls unhandled-file-name-directory, for which the
documentation is

```
"unhandled-file-name-directory"

Return a directly usable directory name somehow associated with \
FILENAME.
A `directly usable' directory name is one that may be used without the
intervention of any file name handler.
If FILENAME is a directly usable file itself, return
\(file-name-as-directory FILENAME).
If FILENAME refers to a file which is not accessible from a local process,
then this should return nil.
The `call-process' and `start-process' functions use this function to
get a current directory to run processes in.
```

(let ((default-directory "https://www.gnu.org/"))
  (unhandled-file-name-directory default-directory))
=> "https://www.gnu.org/"

call-process is bound to fail.

I still haven't gotten around to trying url completions (I use the
implementation in emacs-w3m w3m-urls-completion) but assuming it is
going to useful, perhaps the completion mechanism should work off a
new url-default-directory rather than default-directory if needed.

bug#74208: 31.0.50; minibuffer read-file-name-default mutates global value of default-directory incorrectly

[Eli Zaretskii]

> Date: Sat, 07 Dec 2024 22:53:57 +0530 (IST)
> From: Madhu <enometh@meer.net>
> 
> *  Eli Zaretskii <eliz@gnu.org> <8634izk7ky.fsf@gnu.org>
> Wrote on Sat, 07 Dec 2024 14:20:45 +0200
> > Ping! Do we want to do anything about this issue?
> 
> is "Keep it open" an option? :)

That is not a valid interpretation of "do something", not in my book.

> (let ((default-directory "https://www.gnu.org/"))
>   (unhandled-file-name-directory default-directory))
> => "https://www.gnu.org/"

Which is why I asked what, if anything, do we want to do about it.

bug#74208: 31.0.50; minibuffer read-file-name-default mutates global value of default-directory incorrectly

[Michael Albinus via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Madhu <enometh@meer.net> writes:

Hi,

> ```
> "unhandled-file-name-directory"
>
> Return a directly usable directory name somehow associated with \
> FILENAME.
> A `directly usable' directory name is one that may be used without the
> intervention of any file name handler.
> If FILENAME is a directly usable file itself, return
> \(file-name-as-directory FILENAME).
> If FILENAME refers to a file which is not accessible from a local process,
> then this should return nil.
> The `call-process' and `start-process' functions use this function to
> get a current directory to run processes in.
> ```
>
> (let ((default-directory "https://www.gnu.org/"))
>   (unhandled-file-name-directory default-directory))
> => "https://www.gnu.org/"

unhandled-file-name-directory expects, that special constructs in
FILENAME are handled via a file name handler. Per default,
"https://www.gnu.org/" is not covered by a file name handler, so I
believe it is a non-valid value for default-directory.

See

--8<---------------cut here---------------start------------->8---
(progn
  (url-handler-mode)
  (let ((default-directory "https://www.gnu.org/"))
    (unhandled-file-name-directory default-directory)))
=> nil
--8<---------------cut here---------------end--------------->8---

Whether nil is a proper value to be returned is questionable. However,
call-process and friends are able to handle a nil default-directory.

> I still haven't gotten around to trying url completions (I use the
> implementation in emacs-w3m w3m-urls-completion) but assuming it is
> going to useful, perhaps the completion mechanism should work off a
> new url-default-directory rather than default-directory if needed.

We don't need a new url-default-directory. We must ensure that
url-handler-mode is activated (temporarily) for such use cases.

Best regards, Michael.

bug#74208: 31.0.50; minibuffer read-file-name-default mutates global value of default-directory incorrectly

[Madhu]

[-- Attachment #1: Type: Text/Plain, Size: 1044 bytes --]

*  Michael Albinus <michael.albinus@gmx.de> <87y10qv9y1.fsf@gmx.de>
Wrote on Sun, 08 Dec 2024 09:46:30 +0100
>
> unhandled-file-name-directory expects, that special constructs in
> FILENAME are handled via a file name handler. Per default,
> "https://www.gnu.org/" is not covered by a file name handler, so I
> believe it is a non-valid value for default-directory.
>
> See
>
> --8<---------------cut here---------------start------------->8---
> (progn
>   (url-handler-mode)
>   (let ((default-directory "https://www.gnu.org/"))
>     (unhandled-file-name-directory default-directory)))
> => nil
> --8<---------------cut here---------------end--------------->8---
>
> Whether nil is a proper value to be returned is questionable. However,
> call-process and friends are able to handle a nil default-directory.

Thanks, something like the attached? the implementation of "turn on
url-handler-mode temporarily" looks a bit gross but i guess it can't
be avoided.  should call to turn it on be outside the
minibuffer-setup-hook? --Regards, Madhu


[-- Attachment #2: 0001-minibuffer.el-read-file-name-default-handle-default-.patch --]
[-- Type: Text/X-Patch, Size: 3045 bytes --]

From 91c22af6c317855370eb405f8daf6f56c68c4f9f Mon Sep 17 00:00:00 2001
From: Madhu <enometh@net.meer>
Date: Tue, 5 Nov 2024 07:19:55 +0530
Subject: [PATCH] minibuffer.el: (read-file-name-default): handle
 default-directory urls

* minibuffer.el: (read-file-name-default): bind default-directory
(in dynamic scope) instead of modifying it, protect setting against
non-local exits. Turn on url-handler-mode temporarily, so any calls to
call-process and friends do not fail because the default-directory is a
URL. (bug#24708)
---
 lisp/minibuffer.el | 22 ++++++++++++++++------
 1 file changed, 16 insertions(+), 6 deletions(-)

diff --git a/lisp/minibuffer.el b/lisp/minibuffer.el
index c970752ec2a..607a118dd53 100644
--- a/lisp/minibuffer.el
+++ b/lisp/minibuffer.el
@@ -3667,11 +3667,18 @@ read-file-name-default
                   ;; just use `default-directory', but in order to avoid
                   ;; changing `default-directory' in the current buffer,
                   ;; we don't let-bind it.
-                  (let ((dir (file-name-as-directory
-                              (expand-file-name dir))))
+                  (let* ((dir (file-name-as-directory
+                               (expand-file-name dir)))
+                         (default-directory dir)
+                         (orig-uhm url-handler-mode))
                     (minibuffer-with-setup-hook
                         (lambda ()
-                          (setq default-directory dir)
+                          ;; turn on url-handler-mode temporarily so
+                          ;; call-process and friends don't fail on on a
+                          ;; encountering a url default-directory
+                          ;; (bug#74208)
+                          (unless orig-uhm
+                            (url-handler-mode 1))
                           ;; When the first default in `minibuffer-default'
                           ;; duplicates initial input `insdef',
                           ;; reset `minibuffer-default' to nil.
@@ -3690,9 +3697,12 @@ read-file-name-default
                                      (window-buffer (minibuffer-selected-window))
 				   (read-file-name--defaults dir initial))))
 			  (set-syntax-table minibuffer-local-filename-syntax))
-                      (completing-read prompt 'read-file-name-internal
-                                       pred require-match insdef
-                                       'file-name-history default-filename)))
+                      (unwind-protect
+                          (completing-read prompt 'read-file-name-internal
+                                           pred require-match insdef
+                                           'file-name-history default-filename)
+                        (unless orig-uhm
+                          (url-handler-mode -1)))))
                 ;; If DEFAULT-FILENAME not supplied and DIR contains
                 ;; a file name, split it.
                 (let ((file (file-name-nondirectory dir))
-- 
2.46.0.27.gfa3b914457

bug#74208: 31.0.50; minibuffer read-file-name-default mutates global value of default-directory incorrectly

[Michael Albinus via Bug reports for GNU Emacs, the Swiss army knife of text editors]

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

Madhu <enometh@meer.net> writes:

Hi Madhu,

> Thanks, something like the attached? the implementation of "turn on
> url-handler-mode temporarily" looks a bit gross but i guess it can't
> be avoided.  should call to turn it on be outside the
> minibuffer-setup-hook?

Perhaps, I haven't tested. However, the leading comment doesn't match
anymore the implementation (default-directory is let-bound now); better
you'll adjust it.

I've checked the problem in ffap.el. It looks like we could use a much
simpler solution: implement unhandled-file-name-directory in
ffap--url-file-handler. Something like


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

diff --git a/lisp/ffap.el b/lisp/ffap.el
index 6a4915fb5a3..b03e625c123 100644
--- a/lisp/ffap.el
+++ b/lisp/ffap.el
@@ -1512,6 +1512,7 @@ ffap--url-file-handler
       ;; We mainly just want to disable these bits:
       (substitute-in-file-name (car args))
       (expand-file-name (car args))
+      (unhandled-file-name-directory nil)
       (otherwise
        (apply operation args)))))


[-- Attachment #3: Type: text/plain, Size: 75 bytes --]


Does this solve your problem?

> --Regards, Madhu

Best regards, Michael.

bug#74208: 31.0.50; minibuffer read-file-name-default mutates global value of default-directory incorrectly

[Madhu]

*  Michael Albinus <michael.albinus@gmx.de> <87seqyv286.fsf@gmx.de>
Wrote on Sun, 08 Dec 2024 12:33:13 +0100
> Perhaps, I haven't tested. However, the leading comment doesn't match
> anymore the implementation (default-directory is let-bound now); better
> you'll adjust it.

No, that is an independent change in the patch, (which is still in the
Subject: line, but not admittedly not relevant to the ffap fix.)


> I've checked the problem in ffap.el. It looks like we could use a much
> simpler solution: implement unhandled-file-name-directory in
> ffap--url-file-handler. Something like

In a quick test with the "M-! echo foo" (in a recursive minibuffer at
the ffap prompt), call-shell-region still fails "Setting current
directory" error, -- Regards, Madhu

bug#74208: 31.0.50; minibuffer read-file-name-default mutates global value of default-directory incorrectly

[Michael Albinus via Bug reports for GNU Emacs, the Swiss army knife of text editors]

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

Madhu <enometh@meer.net> writes:

Hi Madhu,

>> I've checked the problem in ffap.el. It looks like we could use a much
>> simpler solution: implement unhandled-file-name-directory in
>> ffap--url-file-handler. Something like
>
> In a quick test with the "M-! echo foo" (in a recursive minibuffer at
> the ffap prompt), call-shell-region still fails "Setting current
> directory" error,

Indeed. The following patch seems to work better, could you pls test?


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

diff --git a/lisp/ffap.el b/lisp/ffap.el
index 6a4915fb5a3..180fe408104 100644
--- a/lisp/ffap.el
+++ b/lisp/ffap.el
@@ -1512,6 +1512,7 @@ ffap--url-file-handler
       ;; We mainly just want to disable these bits:
       (substitute-in-file-name (car args))
       (expand-file-name (car args))
+      (unhandled-file-name-directory temporary-file-directory)
       (otherwise
        (apply operation args)))))


[-- Attachment #3: Type: text/plain, Size: 45 bytes --]


> -- Regards, Madhu

Best regards, Michael.

bug#74208: 31.0.50; minibuffer read-file-name-default mutates global value of default-directory incorrectly

[Madhu]

*  Michael Albinus <michael.albinus@gmx.de> <87jzcaus0j.fsf@gmx.de>
Wrote on Sun, 08 Dec 2024 16:13:48 +0100
> Madhu <enometh@meer.net> writes:
>>> I've checked the problem in ffap.el. It looks like we could use a much
>>> simpler solution: implement unhandled-file-name-directory in
>>> ffap--url-file-handler. Something like
>>
>> In a quick test with the "M-! echo foo" (in a recursive minibuffer at
>> the ffap prompt), call-shell-region still fails "Setting current
>> directory" error,
>
> Indeed. The following patch seems to work better, could you pls test?

Yes this works, M-! pwd prints /tmp while in the recursive minibuffer
at the ffap prompt.


This looks like the right solution to the problem but perhaps use "~"
instead of /tmp in line with what the other code does. Thanks

bug#74208: 31.0.50; minibuffer read-file-name-default mutates global value of default-directory incorrectly

[Michael Albinus via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Madhu <enometh@meer.net> writes:

Hi Madhu,

> Yes this works, M-! pwd prints /tmp while in the recursive minibuffer
> at the ffap prompt.

Thanks for the feedback. I've pushed the change to Emacs master.

> This looks like the right solution to the problem but perhaps use "~"
> instead of /tmp in line with what the other code does. Thanks

No, it isn't guaranteed that the directory "~/" does exist, it depends
on the external environment Emacs is called from. And likely, we would
need to use (expand-file-name "~/"), which might be problematic when the
default directory is a URL.

Btw, I don't see any other use of "~/" in ffap.el. What do you mean with
"what the other code does"?

I didn't follow closely the bug discussion. Is there something left to
be done, before closing?

Best regards, Michael.

bug#74208: 31.0.50; minibuffer read-file-name-default mutates global value of default-directory incorrectly

[Madhu]

*  Michael Albinus <michael.albinus@gmx.de> <87ed2iulrd.fsf@gmx.de>
Wrote on Sun, 08 Dec 2024 18:28:54 +0100
>> This looks like the right solution to the problem but perhaps use "~"
>> instead of /tmp in line with what the other code does. Thanks
>
> No, it isn't guaranteed that the directory "~/" does exist, it depends
> on the external environment Emacs is called from. And likely, we would
> need to use (expand-file-name "~/"), which might be problematic when the
> default directory is a URL.

> Btw, I don't see any other use of "~/" in ffap.el. What do you mean with
> "what the other code does"?

I meant the defaulting use in the call to get_current_directory in
callproc.c

> I didn't follow closely the bug discussion. Is there something left to
> be done, before closing?

Speaking for myself: no. I'm a bit dissapointed that the setq doesn't
get fixed, but I think the main problem has been addressed.

bug#74208: 31.0.50; minibuffer read-file-name-default mutates global value of default-directory incorrectly

[Michael Albinus via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Version: 31.1

Madhu <enometh@meer.net> writes:

Hi Madhu,

> I meant the defaulting use in the call to get_current_directory in
> callproc.c

I see. But as said, it isn't a robust setting:

--8<---------------cut here---------------start------------->8---
# env HOME=/foo emacs -Q --batch --eval '(let ((default-directory (expand-file-name "~"))) (shell-command "ls"))'

Error: file-missing ("Setting current directory" "No such file or directory" "/foo")
  call-shell-region(1 1 "ls" nil #<buffer *Shell Command Output*>)
  shell-command-on-region(1 1 "ls" nil nil nil)
  shell-command("ls")
  (let ((default-directory (expand-file-name "~"))) (shell-command "ls"))
  eval((let ((default-directory (expand-file-name "~"))) (shell-command "ls")) t)
  command-line-1(("--eval" "(let ((default-directory (expand-file-name \"~\"))) (shell-command \"ls\"))"))
  command-line()
  normal-top-level()

(Shell command failed with error)
Setting current directory: No such file or directory, /foo
--8<---------------cut here---------------end--------------->8---

> Speaking for myself: no. I'm a bit dissapointed that the setq doesn't
> get fixed, but I think the main problem has been addressed.

Thanks, so I'm closing the bug.

Best regards, Michael.