bug#71935: split-string-and-unquote mishandles dired-listing-switches with '

bug#71935: split-string-and-unquote mishandles dired-listing-switches with '

[Juri Linkov]

0. emacs -Q
1. (setopt dired-listing-switches "-al --block-size='1")
2. C-x d /tmp/*

  /tmp:
  wildcard *
  /bin/bash: -c: line 0: unexpected EOF while looking for matching `''
  /bin/bash: -c: line 1: syntax error: unexpected end of file

3. (setopt dired-listing-switches "-al --block-size=\\'1")
4. C-x d /tmp/

Debugger entered--Lisp error: (error "Listing directory failed but ‘access-file’ worked")
  error("Listing directory failed but `access-file' worked")
  insert-directory("/tmp/" "--dired -N -al --block-size=\\'1" nil t)
  dired-insert-directory("/tmp/" "-al --block-size=\\'1" nil nil t)
  dired-readin-insert()
  dired-readin()
  dired-internal-noselect("/tmp/" nil)
  dired-noselect("/tmp/" nil)
  dired("/tmp/" nil)
  funcall-interactively(dired "/tmp/" nil)
  command-execute(dired)

because 'split-string-and-unquote' in 'insert-directory'
doesn't turn "--block-size=\\'1" into "--block-size='1".

5. (setopt dired-listing-switches "-al --block-size=\"'1\"")
6. C-x d /tmp/

Same error for another reason, because 'split-string-and-unquote'
splits "--block-size=\"'1\"" to '("--block-size=" "'1").

bug#71935: split-string-and-unquote mishandles dired-listing-switches with '

[Eli Zaretskii]

> From: Juri Linkov <juri@linkov.net>
> Date: Thu, 04 Jul 2024 09:51:10 +0300
> 
> 0. emacs -Q
> 1. (setopt dired-listing-switches "-al --block-size='1")
> 2. C-x d /tmp/*
> 
>   /tmp:
>   wildcard *
>   /bin/bash: -c: line 0: unexpected EOF while looking for matching `''
>   /bin/bash: -c: line 1: syntax error: unexpected end of file
> 
> 3. (setopt dired-listing-switches "-al --block-size=\\'1")
> 4. C-x d /tmp/
> 
> Debugger entered--Lisp error: (error "Listing directory failed but ‘access-file’ worked")
>   error("Listing directory failed but `access-file' worked")
>   insert-directory("/tmp/" "--dired -N -al --block-size=\\'1" nil t)
>   dired-insert-directory("/tmp/" "-al --block-size=\\'1" nil nil t)
>   dired-readin-insert()
>   dired-readin()
>   dired-internal-noselect("/tmp/" nil)
>   dired-noselect("/tmp/" nil)
>   dired("/tmp/" nil)
>   funcall-interactively(dired "/tmp/" nil)
>   command-execute(dired)
> 
> because 'split-string-and-unquote' in 'insert-directory'
> doesn't turn "--block-size=\\'1" into "--block-size='1".

I think the part of insert-directory that deals with wildcard lacks
the call to shell-quote-argument here:

		      ;; NB since switches is passed to the shell, be
		      ;; careful of malicious values, eg "-l;reboot".
		      ;; See eg dired-safe-switches-p.
		      (call-process
		       shell-file-name nil t nil
		       shell-command-switch
		       (concat (if (memq system-type '(ms-dos windows-nt))
				   ""
				 "\\") ; Disregard Unix shell aliases!
			       insert-directory-program
			       " -d "
			       (if (stringp switches)
				   switches
				 (mapconcat #'identity switches " "))

Can you try running each switch through shell-quote-argument?

bug#71935: split-string-and-unquote mishandles dired-listing-switches with '

[Juri Linkov]

>> 1. (setopt dired-listing-switches "-al --block-size='1")
>> 2. C-x d /tmp/*
>>
>>   /tmp:
>>   wildcard *
>>   /bin/bash: -c: line 0: unexpected EOF while looking for matching `''
>>   /bin/bash: -c: line 1: syntax error: unexpected end of file
>>
>> 3. (setopt dired-listing-switches "-al --block-size=\\'1")
>> 4. C-x d /tmp/
>>
>> Debugger entered--Lisp error: (error "Listing directory failed but ‘access-file’ worked")
>>   error("Listing directory failed but `access-file' worked")
>>   insert-directory("/tmp/" "--dired -N -al --block-size=\\'1" nil t)
>
> I think the part of insert-directory that deals with wildcard lacks
> the call to shell-quote-argument here:
>
> 		      ;; NB since switches is passed to the shell, be
> 		      ;; careful of malicious values, eg "-l;reboot".
> 		      ;; See eg dired-safe-switches-p.
> 		      (call-process
> 		       shell-file-name nil t nil
> 		       shell-command-switch
> 		       (concat (if (memq system-type '(ms-dos windows-nt))
> 				   ""
> 				 "\\") ; Disregard Unix shell aliases!
> 			       insert-directory-program
> 			       " -d "
> 			       (if (stringp switches)
> 				   switches
> 				 (mapconcat #'identity switches " "))
>
> Can you try running each switch through shell-quote-argument?

This part of insert-directory is used only in case of 1-2 above,
i.e. for wildcard '/tmp/*'.  In this case the value of 'switches'
is "--dired -N -al --block-size='1", and 'shell-quote-argument'
returns "--dired\\ -N\\ -al\\ --block-size\\=\\'1" that fails.

For the non-wildcard case of 3-4 above, this doesn't help either.
Using (mapcar 'shell-quote-argument (split-string-and-unquote switches))
on ("--dired" "-N" "-al" "--block-size=\\'1") returns
("--dired" "-N" "-al" "--block-size\\=\\\\\\'1") that fails with

  /bin/ls: unrecognized option '--block-size\=\\\'1'

bug#71935: split-string-and-unquote mishandles dired-listing-switches with '

[Eli Zaretskii]

> From: Juri Linkov <juri@linkov.net>
> Cc: 71935@debbugs.gnu.org
> Date: Thu, 04 Jul 2024 19:10:17 +0300
> 
> >> 1. (setopt dired-listing-switches "-al --block-size='1")
> >> 2. C-x d /tmp/*
> >>
> >>   /tmp:
> >>   wildcard *
> >>   /bin/bash: -c: line 0: unexpected EOF while looking for matching `''
> >>   /bin/bash: -c: line 1: syntax error: unexpected end of file
> >>
> >> 3. (setopt dired-listing-switches "-al --block-size=\\'1")
> >> 4. C-x d /tmp/
> >>
> >> Debugger entered--Lisp error: (error "Listing directory failed but ‘access-file’ worked")
> >>   error("Listing directory failed but `access-file' worked")
> >>   insert-directory("/tmp/" "--dired -N -al --block-size=\\'1" nil t)
> >
> > I think the part of insert-directory that deals with wildcard lacks
> > the call to shell-quote-argument here:
> >
> > 		      ;; NB since switches is passed to the shell, be
> > 		      ;; careful of malicious values, eg "-l;reboot".
> > 		      ;; See eg dired-safe-switches-p.
> > 		      (call-process
> > 		       shell-file-name nil t nil
> > 		       shell-command-switch
> > 		       (concat (if (memq system-type '(ms-dos windows-nt))
> > 				   ""
> > 				 "\\") ; Disregard Unix shell aliases!
> > 			       insert-directory-program
> > 			       " -d "
> > 			       (if (stringp switches)
> > 				   switches
> > 				 (mapconcat #'identity switches " "))
> >
> > Can you try running each switch through shell-quote-argument?
> 
> This part of insert-directory is used only in case of 1-2 above,
> i.e. for wildcard '/tmp/*'.  In this case the value of 'switches'
> is "--dired -N -al --block-size='1", and 'shell-quote-argument'
> returns "--dired\\ -N\\ -al\\ --block-size\\=\\'1" that fails.

I meant to call shell-quote-argument on each option, before they are
concatenated.

> For the non-wildcard case of 3-4 above, this doesn't help either.
> Using (mapcar 'shell-quote-argument (split-string-and-unquote switches))
> on ("--dired" "-N" "-al" "--block-size=\\'1") returns
> ("--dired" "-N" "-al" "--block-size\\=\\\\\\'1") that fails with
> 
>   /bin/ls: unrecognized option '--block-size\=\\\'1'

Why did you use "--block-size=\\'1"?  My idea is that the quoting
should not come from the user.

bug#71935: split-string-and-unquote mishandles dired-listing-switches with '

[Juri Linkov]

>> >> 1. (setopt dired-listing-switches "-al --block-size='1")
>> >> 2. C-x d /tmp/*
>> 
>> This part of insert-directory is used only in case of 1-2 above,
>> i.e. for wildcard '/tmp/*'.  In this case the value of 'switches'
>> is "--dired -N -al --block-size='1", and 'shell-quote-argument'
>> returns "--dired\\ -N\\ -al\\ --block-size\\=\\'1" that fails.
>
> I meant to call shell-quote-argument on each option, before they are
> concatenated.

But switches are never unconcatenated in a list,
they come from the string:

  (setopt dired-listing-switches "-al --block-size='1")

>> For the non-wildcard case of 3-4 above, this doesn't help either.
>> Using (mapcar 'shell-quote-argument (split-string-and-unquote switches))
>> on ("--dired" "-N" "-al" "--block-size=\\'1") returns
>> ("--dired" "-N" "-al" "--block-size\\=\\\\\\'1") that fails with
>> 
>>   /bin/ls: unrecognized option '--block-size\=\\\'1'
>
> Why did you use "--block-size=\\'1"?  My idea is that the quoting
> should not come from the user.

If the wildcard case above could be fixed, then there is no need
to use "--block-size=\\'1".

bug#71935: split-string-and-unquote mishandles dired-listing-switches with '

[Eli Zaretskii]

> From: Juri Linkov <juri@linkov.net>
> Cc: 71935@debbugs.gnu.org
> Date: Thu, 04 Jul 2024 21:12:01 +0300
> 
> >> >> 1. (setopt dired-listing-switches "-al --block-size='1")
> >> >> 2. C-x d /tmp/*
> >> 
> >> This part of insert-directory is used only in case of 1-2 above,
> >> i.e. for wildcard '/tmp/*'.  In this case the value of 'switches'
> >> is "--dired -N -al --block-size='1", and 'shell-quote-argument'
> >> returns "--dired\\ -N\\ -al\\ --block-size\\=\\'1" that fails.
> >
> > I meant to call shell-quote-argument on each option, before they are
> > concatenated.
> 
> But switches are never unconcatenated in a list,
> they come from the string:
> 
>   (setopt dired-listing-switches "-al --block-size='1")

If they are a single string, split it with split-string-and-unquote,
then concatenate after running each one through shell-quote-argument.
If they are a list of strings, quote each one before concatenating
with mapconcat.

bug#71935: split-string-and-unquote mishandles dired-listing-switches with '

[Juri Linkov]

>>> 1. (setopt dired-listing-switches "-al --block-size='1")
>>> 2. C-x d /tmp/*
>
> If they are a single string, split it with split-string-and-unquote,
> then concatenate after running each one through shell-quote-argument.
> If they are a list of strings, quote each one before concatenating
> with mapconcat.

Ok, this seems to work:

diff --git a/lisp/files.el b/lisp/files.el
index 042b8e2d515..6ed07f02890 100644
--- a/lisp/files.el
+++ b/lisp/files.el
@@ -8195,9 +8193,11 @@ insert-directory
 				 "\\") ; Disregard Unix shell aliases!
 			       insert-directory-program
 			       " -d "
-			       (if (stringp switches)
-				   switches
-				 (mapconcat #'identity switches " "))
+			       (mapconcat #'shell-quote-argument
+                                          (if (stringp switches)
+				              (split-string-and-unquote switches)
+                                            switches)
+                                          " ")
 			       " -- "
 			       ;; Quote some characters that have
 			       ;; special meanings in shells; but

bug#71935: split-string-and-unquote mishandles dired-listing-switches with '

[Eli Zaretskii]

> From: Juri Linkov <juri@linkov.net>
> Cc: 71935@debbugs.gnu.org
> Date: Thu, 04 Jul 2024 21:54:02 +0300
> 
> >>> 1. (setopt dired-listing-switches "-al --block-size='1")
> >>> 2. C-x d /tmp/*
> >
> > If they are a single string, split it with split-string-and-unquote,
> > then concatenate after running each one through shell-quote-argument.
> > If they are a list of strings, quote each one before concatenating
> > with mapconcat.
> 
> Ok, this seems to work:
> 
> diff --git a/lisp/files.el b/lisp/files.el
> index 042b8e2d515..6ed07f02890 100644
> --- a/lisp/files.el
> +++ b/lisp/files.el
> @@ -8195,9 +8193,11 @@ insert-directory
>  				 "\\") ; Disregard Unix shell aliases!
>  			       insert-directory-program
>  			       " -d "
> -			       (if (stringp switches)
> -				   switches
> -				 (mapconcat #'identity switches " "))
> +			       (mapconcat #'shell-quote-argument
> +                                          (if (stringp switches)
> +				              (split-string-and-unquote switches)
> +                                            switches)
> +                                          " ")
>  			       " -- "
>  			       ;; Quote some characters that have
>  			       ;; special meanings in shells; but
> 

Thanks, that's what I had in mind.  Please install on the emacs-30
branch.

bug#71935: split-string-and-unquote mishandles dired-listing-switches with '

[Eli Zaretskii]

> Cc: 71935@debbugs.gnu.org
> Date: Thu, 04 Jul 2024 22:03:00 +0300
> From: Eli Zaretskii <eliz@gnu.org>
> 
> > diff --git a/lisp/files.el b/lisp/files.el
> > index 042b8e2d515..6ed07f02890 100644
> > --- a/lisp/files.el
> > +++ b/lisp/files.el
> > @@ -8195,9 +8193,11 @@ insert-directory
> >  				 "\\") ; Disregard Unix shell aliases!
> >  			       insert-directory-program
> >  			       " -d "
> > -			       (if (stringp switches)
> > -				   switches
> > -				 (mapconcat #'identity switches " "))
> > +			       (mapconcat #'shell-quote-argument
> > +                                          (if (stringp switches)
> > +				              (split-string-and-unquote switches)
> > +                                            switches)
> > +                                          " ")
> >  			       " -- "
> >  			       ;; Quote some characters that have
> >  			       ;; special meanings in shells; but
> > 
> 
> Thanks, that's what I had in mind.  Please install on the emacs-30
> branch.

On second thought: could there be options that include shell
wildcards, which therefore should not be quoted?  If so, perhaps
instead of shell-quote-argument we should use
shell-quote-wildcard-pattern?

bug#71935: split-string-and-unquote mishandles dired-listing-switches with '

[Juri Linkov]

>> > @@ -8195,9 +8193,11 @@ insert-directory
>> > -			       (if (stringp switches)
>> > -				   switches
>> > -				 (mapconcat #'identity switches " "))
>> > +			       (mapconcat #'shell-quote-argument
>> > +                                          (if (stringp switches)
>> > +				              (split-string-and-unquote switches)
>> > +                                            switches)
>> > +                                          " ")
>>
>> Thanks, that's what I had in mind.  Please install on the emacs-30
>> branch.
>
> On second thought: could there be options that include shell
> wildcards, which therefore should not be quoted?  If so, perhaps
> instead of shell-quote-argument we should use
> shell-quote-wildcard-pattern?

Indeed, there are ls switches that use wildcards, e.g.
‘--hide=PATTERN’ and ‘--ignore=PATTERN’.  But it seems
they are ignored anyway while using wildcards with 'ls -d',
so I can't test them.  What I can confirm only is that with

1. (setopt dired-listing-switches "-al --block-size='1 --ignore=system*")
2. C-x d /tmp/s*

the switches are correctly quoted by 'shell-quote-wildcard-pattern':

"ls -d --dired -N -al --block-size=\\'1 --ignore=system* -- s*"

diff --git a/lisp/files.el b/lisp/files.el
index 042b8e2d515..e69dfb22a5f 100644
--- a/lisp/files.el
+++ b/lisp/files.el
@@ -8195,9 +8193,15 @@ insert-directory
                                 "\\") ; Disregard Unix shell aliases!
                               insert-directory-program
                               " -d "
-                              (if (stringp switches)
-                                  switches
-                                (mapconcat #'identity switches " "))
+                              ;; Quote switches that require quoting
+                              ;; such as "--block-size='1".  But don't
+                              ;; quote switches that use patterns
+                              ;; such as "--ignore=PATTERN" (bug#71935).
+                              (mapconcat #'shell-quote-wildcard-pattern
+                                         (if (stringp switches)
+                                             (split-string-and-unquote switches)
+                                           switches)
+                                         " ")
                               " -- "
                               ;; Quote some characters that have
                               ;; special meanings in shells; but

bug#71935: split-string-and-unquote mishandles dired-listing-switches with '

[Eli Zaretskii]

> From: Juri Linkov <juri@linkov.net>
> Cc: 71935@debbugs.gnu.org
> Date: Fri, 05 Jul 2024 09:48:39 +0300
> 
> >> Thanks, that's what I had in mind.  Please install on the emacs-30
> >> branch.
> >
> > On second thought: could there be options that include shell
> > wildcards, which therefore should not be quoted?  If so, perhaps
> > instead of shell-quote-argument we should use
> > shell-quote-wildcard-pattern?
> 
> Indeed, there are ls switches that use wildcards, e.g.
> ‘--hide=PATTERN’ and ‘--ignore=PATTERN’.  But it seems
> they are ignored anyway while using wildcards with 'ls -d',
> so I can't test them.  What I can confirm only is that with
> 
> 1. (setopt dired-listing-switches "-al --block-size='1 --ignore=system*")
> 2. C-x d /tmp/s*
> 
> the switches are correctly quoted by 'shell-quote-wildcard-pattern':
> 
> "ls -d --dired -N -al --block-size=\\'1 --ignore=system* -- s*"

Thanks.  So I think this is a better solution for this tricky problem.

bug#71935: split-string-and-unquote mishandles dired-listing-switches with '

[Juri Linkov]

close 71935 30.0.60
thanks

> Please install on the emacs-30 branch.

Installed.