Re: Emacsclient/server filename quoting error

Re: Emacsclient/server filename quoting error

[Juanma Barranquero]

On 12/10/06, Francis Wright <F.J.Wright@qmul.ac.uk> wrote:

> I execute all the following commands from a cmd command
> prompt (outside of Emacs).
>
> emacsclient -n -a runemacs "TO DO.txt"
>
> works correctly if Emacs IS already running, but if it is not
> already running then Emacs does not see the filename correctly;
> the quotes do not appear to be passed on to runemacs.

The following patch addresses the issue by allocating quoted copies of
any argument containing spaces before calling execvp.

Any objections to install this fix? As it stands, it affects also
non-Windows builds. Is requoting args the right behavior in these
environments?

                    /L/e/k/t/u



Index: lib-src/emacsclient.c
===================================================================
RCS file: /cvsroot/emacs/emacs/lib-src/emacsclient.c,v
retrieving revision 1.98
diff -u -2 -r1.98 emacsclient.c
--- lib-src/emacsclient.c	30 Nov 2006 22:49:38 -0000	1.98
+++ lib-src/emacsclient.c	15 Dec 2006 10:19:44 -0000
@@ -310,8 +310,20 @@
   if (alternate_editor)
     {
-      int i = optind - 1;
+      int j, i = optind - 1;
+
 #ifdef WINDOWSNT
-      argv[i] = (char *)alternate_editor;
+      argv[i] = (char *) alternate_editor;
 #endif
+
+      /* Arguments with spaces have been dequoted, so we
+	 have to requote them before calling execvp.  */
+      for (j = i; argv[j]; j++)
+	if (strchr (argv[j], ' '))
+	  {
+	    char *quoted = alloca (strlen (argv[j]) + 3);
+	    sprintf (quoted, "\"%s\"", argv[j]);
+	    argv[j] = quoted;
+	  }
+
       execvp (alternate_editor, argv + i);
       message (TRUE, "%s: error executing alternate editor \"%s\"\n",

Re: Emacsclient/server filename quoting error

[Eli Zaretskii]

> Date: Fri, 15 Dec 2006 11:31:18 +0100
> From: "Juanma Barranquero" <lekktu@gmail.com>
> Cc: emacs-pretest-bug@gnu.org, Emacs Devel <emacs-devel@gnu.org>
> 
> > I execute all the following commands from a cmd command
> > prompt (outside of Emacs).
> >
> > emacsclient -n -a runemacs "TO DO.txt"
> >
> > works correctly if Emacs IS already running, but if it is not
> > already running then Emacs does not see the filename correctly;
> > the quotes do not appear to be passed on to runemacs.
> 
> The following patch addresses the issue by allocating quoted copies of
> any argument containing spaces before calling execvp.
> 
> Any objections to install this fix? As it stands, it affects also
> non-Windows builds. Is requoting args the right behavior in these
> environments?

Quoting arguments passed to execvp is _definitely_ the wrong thing for
Posix platforms.  The fact that we need to do that for Windows is due
to the broken implementation of exec* routines in the Microsoft
libraries: they concatenate the arguments together without quoting
special characters, and pass the result to CreateProcess, with
predictably bad results.

By contrast, Posix execvp passes the arguments directly into the
argv[] array of the child process.

So please make this change conditioned on WINDOWSNT.

Actually, a cleaner way of fixing this would be to have a
WINDOWSNT-only wrapper for execvp, called, say w32_execvp, that does
TRT with quoting the arguments.  Then you could say

  #ifdef WINDOWSNT
  #define execvp w32_execvp
  #endif

in emacsclient.c, and leave the mainline code intact.

Re: Emacsclient/server filename quoting error

[Juanma Barranquero]

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

On 12/15/06, Eli Zaretskii <eliz@gnu.org> wrote:

> Actually, a cleaner way of fixing this would be to have a
> WINDOWSNT-only wrapper for execvp, called, say w32_execvp, that does
> TRT with quoting the arguments.

You like this one better, then?

                    /L/e/k/t/u


Index: lib-src/emacsclient.c
===================================================================
RCS file: /cvsroot/emacs/emacs/lib-src/emacsclient.c,v
retrieving revision 1.98
diff -u -2 -r1.98 emacsclient.c
--- lib-src/emacsclient.c	30 Nov 2006 22:49:38 -0000	1.98
+++ lib-src/emacsclient.c	15 Dec 2006 11:49:57 -0000
@@ -299,4 +299,35 @@

 \f
+#ifdef WINDOWSNT
+
+/*
+  execvp() wrapper for Windows.
+  Quotes arguments with embedded spaces.
+*/
+int
+w32_execvp (path, argv)
+     char *path;
+     char **argv;
+{
+  int i;
+
+  argv[0] = (char *) alternate_editor;
+
+  for (i = 0; argv[i]; i++)
+    if (strchr (argv[i], ' '))
+      {
+	char *quoted = alloca (strlen (argv[i]) + 3);
+	sprintf (quoted, "\"%s\"", argv[i]);
+	argv[i] = quoted;
+      }
+
+  return execvp (path, argv);
+}
+
+#undef execvp
+#define execvp w32_execvp
+
+#endif /* WINDOWSNT */
+
 /*
   Try to run a different command, or --if no alternate editor is
@@ -311,7 +342,5 @@
     {
       int i = optind - 1;
-#ifdef WINDOWSNT
-      argv[i] = (char *)alternate_editor;
-#endif
+
       execvp (alternate_editor, argv + i);
       message (TRUE, "%s: error executing alternate editor \"%s\"\n",

[-- Attachment #2: Type: text/plain, Size: 161 bytes --]

_______________________________________________
emacs-pretest-bug mailing list
emacs-pretest-bug@gnu.org
http://lists.gnu.org/mailman/listinfo/emacs-pretest-bug

Re: Emacsclient/server filename quoting error

[Eli Zaretskii]

> Date: Fri, 15 Dec 2006 13:07:01 +0100
> From: "Juanma Barranquero" <lekktu@gmail.com>
> Cc: F.J.Wright@qmul.ac.uk, emacs-pretest-bug@gnu.org, emacs-devel@gnu.org
> 
> On 12/15/06, Eli Zaretskii <eliz@gnu.org> wrote:
> 
> > Actually, a cleaner way of fixing this would be to have a
> > WINDOWSNT-only wrapper for execvp, called, say w32_execvp, that does
> > TRT with quoting the arguments.
> 
> You like this one better, then?

Yes, thanks.

However, please mention the Windows bug with execvp in the comment to
w32_execvp, otherwise it is not clear to the uninitiated why it is
needed, and why we quote the arguments we pass to the real execvp.

Re: Emacsclient/server filename quoting error

[Juanma Barranquero]

On 12/15/06, Eli Zaretskii <eliz@gnu.org> wrote:

> However, please mention the Windows bug with execvp in the comment to
> w32_execvp

I've shamelessly stolen your comment from a previous message.

Thanks,

                    /L/e/k/t/u

Re: Emacsclient/server filename quoting error

[David Kastrup]

"Juanma Barranquero" <lekktu@gmail.com> writes:

> On 12/15/06, Eli Zaretskii <eliz@gnu.org> wrote:
>
>> However, please mention the Windows bug with execvp in the comment to
>> w32_execvp
>
> I've shamelessly stolen your comment from a previous message.

Does Eli has a copyright assignment for list mails on file?

-- 
David Kastrup, Kriemhildstr. 15, 44793 Bochum

Re: Emacsclient/server filename quoting error

[Juanma Barranquero]

On 12/15/06, David Kastrup <dak@gnu.org> wrote:

> Does Eli has a copyright assignment for list mails on file?

Do we have it for everyone quoted on etc/DEVEL.HUMOR?

                    /L/e/k/t/u

Re: Emacsclient/server filename quoting error

[Eli Zaretskii]

> Date: Fri, 15 Dec 2006 16:42:57 +0100
> From: "Juanma Barranquero" <lekktu@gmail.com>
> Cc: F.J.Wright@qmul.ac.uk, emacs-devel@gnu.org, emacs-pretest-bug@gnu.org
> 
> I've shamelessly stolen your comment from a previous message.

That's okay, since I have a copyright assignment on file with the FSF.

Re: Emacsclient/server filename quoting error

[Werner LEMBERG]

> > Actually, a cleaner way of fixing this would be to have a
> > WINDOWSNT-only wrapper for execvp, called, say w32_execvp, that
> > does TRT with quoting the arguments.
>
> You like this one better, then?

Since those issues are full of subtleties I suggest that you have a
look at the gnulib CVS (at savannah.gnu.org), inspecting the files
`execute.c' and `pipe.c':

  http://cvs.savannah.gnu.org/viewcvs/gnulib/lib/execute.c?rev=1.7&root=gnulib&view=log
  http://cvs.savannah.gnu.org/viewcvs/gnulib/lib/pipe.c?rev=1.6&root=gnulib&view=log

Similar code is used in groff too.


    Werner

Re: Emacsclient/server filename quoting error

[Juanma Barranquero]

On 12/15/06, Werner LEMBERG <wl@gnu.org> wrote:

> Since those issues are full of subtleties I suggest that you have a
> look at the gnulib CVS (at savannah.gnu.org), inspecting the files
> `execute.c' and `pipe.c':

Thanks, but I think I'll wait for people to test the current
implementation. I don't think Windows users will have very
sophisticate or uncommon alternate editor setups and if they do, they
must be prepared for the possibility of it not working ;)

                    /L/e/k/t/u

Re: Emacsclient/server filename quoting error

[Eli Zaretskii]

> Date: Fri, 15 Dec 2006 16:14:38 +0100 (CET)
> Cc: eliz@gnu.org, F.J.Wright@qmul.ac.uk, emacs-devel@gnu.org,
>  emacs-pretest-bug@gnu.org
> From: Werner LEMBERG <wl@gnu.org>
> 
> Since those issues are full of subtleties I suggest that you have a
> look at the gnulib CVS (at savannah.gnu.org), inspecting the files
> `execute.c' and `pipe.c':
> 
>   http://cvs.savannah.gnu.org/viewcvs/gnulib/lib/execute.c?rev=1.7&root=gnulib&view=log
>   http://cvs.savannah.gnu.org/viewcvs/gnulib/lib/pipe.c?rev=1.6&root=gnulib&view=log

Thanks, but I see nothing in these source files that could shed any
light on the case in point.

> Similar code is used in groff too.

Groff builds a pipe of several programs and then runs that pipe.  By
contrast, emacsclient needs to run a single program, and doesn't need
to read/write its standard streams.  So the complexity of the above
URLs is not needed.

Re: Emacsclient/server filename quoting error

[Francis Wright]

Thanks for the revised patch for emacsclient.  It solves the problem I 
reported.

Francis

----- Original Message ----- 
From: "Juanma Barranquero" <lekktu@gmail.com>
To: "Eli Zaretskii" <eliz@gnu.org>
Cc: <F.J.Wright@qmul.ac.uk>; <emacs-pretest-bug@gnu.org>; 
<emacs-devel@gnu.org>
Sent: Friday, December 15, 2006 12:07 PM
Subject: Re: Emacsclient/server filename quoting error


> On 12/15/06, Eli Zaretskii <eliz@gnu.org> wrote:
>
>> Actually, a cleaner way of fixing this would be to have a
>> WINDOWSNT-only wrapper for execvp, called, say w32_execvp, that does
>> TRT with quoting the arguments.
>
> You like this one better, then?
>
>                    /L/e/k/t/u