[Bug ada/20075] New: Bug in GNAT.Expect.Non_Blocking_Spawn

[Bug ada/20075] New: Bug in GNAT.Expect.Non_Blocking_Spawn

[sbellon at sbellon dot de]

GNAT.Expect.Non_Blocking_Spawn puts Command_With_Path into argv[0] when calling
__gnat_expect_portable_execvp (via Set_Up_Child_Communications). This leads to
problems on Windows when the path to command contains spaces as the spawned
process only returns the path up to the first space when questioning argv[0]. A
fix is to only put just Command (without the complete path) into argv[0]. The
spawn versions in GNAT.OS_Lib do it that way, so GNAT.Expect.Non_Blocking_Spawn
is inconsistent anyway.

The fix is simple and needs only to change two lines (patch for GCC 3.4.4 but is
similar for 3.15p as well):

--- 3.4.4/g-expect.adb  2005-02-19 12:28:29.350605929 +0100
+++ patched/g-expect.adb        2005-02-19 12:29:16.332632637 +0100
@@ -873,8 +873,8 @@
       if Descriptor.Pid = Null_Pid then
          --  Prepare an array of arguments to pass to C
 
-         Arg := new String (1 .. Command_With_Path'Length + 1);
-         Arg (1 .. Command_With_Path'Length) := Command_With_Path.all;
+         Arg := new String (1 .. Command'Length + 1);
+         Arg (1 .. Command'Length) := Command;
          Arg (Arg'Last)        := ASCII.NUL;
          Arg_List (1)          := Arg;
 

A minimal test case that shows the problem needs two main programs, one
(launcher.adb) which launches the other (show_params.adb):

$ cat show_params.adb
with Ada.Command_Line;
with Ada.Text_IO;
procedure Show_Params is
begin
   Ada.Text_IO.Put_Line (Ada.Command_Line.Command_Name);
end Show_Params;

$ cat show launcher.adb
with Ada.Text_IO;
with GNAT.OS_Lib;
with GNAT.Expect;
procedure Launcher is
   Args : GNAT.OS_Lib.Argument_List (1 .. 0);
   PD   : GNAT.Expect.Process_Descriptor;
   Succ : Boolean;
   Res  : GNAT.Expect.Expect_Match;
   use type GNAT.Expect.Expect_Match;
begin
   Ada.Text_IO.Put_Line ("via GNAT.OS_Lib.Spawn:");
   GNAT.OS_Lib.Spawn ("show_params", Args, Succ);
   Ada.Text_IO.Put_Line ("via GNAT.Expect.Non_Blocking_Spawn:");
   GNAT.Expect.Non_Blocking_Spawn (PD, "show_params", Args);
   begin
      loop
         GNAT.Expect.Expect (PD, Res, ".");
         if Res >= 0 then
            Ada.Text_IO.Put (GNAT.Expect.Expect_Out (PD));
         end if;
       end loop;
   exception
      when GNAT.Expect.Process_Died =>
         GNAT.Expect.Close (PD);
   end;
end Launcher;

$ gnatmake show_params.adb 
gcc-3.4 -c show_params.adb
gnatbind -x show_params.ali
gnatlink show_params.ali
$ gnatmake launcher.adb 
gcc-3.4 -c launcher.adb
gnatbind -x launcher.ali
gnatlink launcher.ali

Put show_params somewhere in your $PATH and start launcher. You'll see that via
GNAT.OS_Lib, only the basename of the command is put into argv[0] and via
GNAT.Expect, the whole path is put there. Now, if you put show_params into a
path with some spaces, you'll see the problem:

$ PATH=`pwd` launcher
via GNAT.OS_Lib.Spawn:
show_params
via GNAT.Expect.Non_Blocking_Spawn:
/home/sbellon/tmp/show params/show_params

It works under GNU/Linux, but it fails on Windows. If you put show_params.exe
into a path of "C:\Program Files\show_params.exe", put that into your %PATH% and
start launcher.exe, then you'll see that only "C:\Program" is returned as
Ada.Command_Line.Command_Name.

This is true for all versions of GNAT we have looked at (GNAT 3.15p, GCC 3.3.x
and GCC 3.4.x). As the fix is very simple, I hope it can be applied to the
current version before the release of GCC 4.0 and perhaps even backported to
older versions.

According to your instructions of how to submit a bug report, I'll include all
the further details but I don't see that they're relevant here as it's a simple
bug in the GNAT lib and is present across all versions of GNAT and all
target/host configurations (applies to MinGW as well).

$ gcc-3.4 -v
Reading specs from /usr/lib/gcc/i486-linux/3.4.4/specs
Configured with: ../src/configure -v
--enable-languages=c,c++,java,f77,pascal,objc,ada,treelang --prefix=/usr
--libexecdir=/usr/lib --with-gxx-include-dir=/usr/include/c++/3.4
--enable-shared --with-system-zlib --enable-nls --without-included-gettext
--program-suffix=-3.4 --enable-__cxa_atexit --enable-libstdcxx-allocator=mt
--enable-clocale=gnu --enable-libstdcxx-debug --enable-java-gc=boehm
--enable-java-awt=gtk --disable-werror i486-linux
Thread model: posix
gcc version 3.4.4 20050203 (prerelease) (Debian 3.4.3-9)

-- 
           Summary: Bug in GNAT.Expect.Non_Blocking_Spawn
           Product: gcc
           Version: 3.4.4
            Status: UNCONFIRMED
          Severity: normal
          Priority: P2
         Component: ada
        AssignedTo: unassigned at gcc dot gnu dot org
        ReportedBy: sbellon at sbellon dot de
                CC: gcc-bugs at gcc dot gnu dot org


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=20075

[Bug ada/20075] Bug in GNAT.Expect.Non_Blocking_Spawn

[ludovic dot brenta at insalien dot org]

------- Additional Comments From ludovic dot brenta at insalien dot org  2005-02-20 17:14 -------
Hi Stefan,

I've given some more thought to the problem, and I think that perhaps
the bug is in Ada.Command_Line.Command_Name, not in GNAT.OS_Lib or
GNAT.Expect.  Indeed, it is Ada.Command_Line.Command_Name that
truncates argv[0] at the first space, isn't it?  And this is wrong,
because even the basename of the program may have spaces in it.

To demonstrate this, I renamed "show_params" to "show params" and
amended launcher.adb accordingly:

$ gnatmake -o "show params" show_params.adb
$ gnatmake launcher.adb
$ PATH=$(pwd) ./launcher
via GNAT.OS_Lib.Spawn:
show params
via GNAT.Expect.Non_Blocking_Spawn:
/home/lbrenta/src/ada/tmp/ada 20075/show params

Could you try that on Windows in a path that does not have a space in
it (e.g. "C:\Temp\show params.exe")?

-- 
Ludovic Brenta.

-- 
           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |ludovic dot brenta at
                   |                            |insalien dot org


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=20075

[Bug ada/20075] Bug in GNAT.Expect.Non_Blocking_Spawn

[sbellon at sbellon dot de]

------- Additional Comments From sbellon at sbellon dot de  2005-02-20 18:33 -------
Subject: Re:  Bug in GNAT.Expect.Non_Blocking_Spawn

On 20 Feb, you wrote:

> I've given some more thought to the problem, and I think that perhaps
> the bug is in Ada.Command_Line.Command_Name, not in GNAT.OS_Lib or
> GNAT.Expect.  Indeed, it is Ada.Command_Line.Command_Name that
> truncates argv[0] at the first space, isn't it?  And this is wrong,
> because even the basename of the program may have spaces in it.

Yes, I completely agree.

[snip]

> Could you try that on Windows in a path that does not have a space in
> it (e.g. "C:\Temp\show params.exe")?

I already tried that last week (and you are right), but decided to go
for the easy solution as the main inconsistency is indeed in
GNAT.Expect.Non_Blocking_Spawn. Fixing it there brings it in line with
the other Spawn functions in GNAT.OS_Lib and fixes the most frequent
case of spaces in pathnames ("C:\Program Files\"). It's in any case not
wrong to apply my suggested patch there.

If in addition, Ada.Command_Line was changed that even spaces in the
basename are taken care of, even better. But changing Ada.Command_Line
isn't an alternative but an addition to the suggested patch.



-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=20075

[Bug ada/20075] Bug in GNAT.Expect.Non_Blocking_Spawn

[sbellon at sbellon dot de]

------- Additional Comments From sbellon at sbellon dot de  2005-02-27 18:47 -------
I have investigated further. There are further places in the C part of the GNAT
library that need changing because they are inconsistent.

In __gnat_portable_spawn in ada/adaint.c the spawn/exec calls are always the
'path' variants except for Unix. The patch for GCC 3.4.3 should be as follows:

--- adaint.c    2004-01-13 12:51:31.000000000 +0100
+++ patched/adaint.c    2005-02-25 13:25:40.000000000 +0100
@@ -1520,9 +1520,9 @@
   if (pid == 0)
     {
       /* The child. */
-      if (execv (args[0], args) != 0)
+      if (execvp (args[0], args) != 0)
 #if defined (VMS)
-       return -1; /* execv is in parent context on VMS.  */
+       return -1; /* execvp is in parent context on VMS.  */
 #else
        _exit (1);
 #endif

The only excuse could be that not all operating systems support execvp. In that
case, a simple define could fix that situation.

Furthermore, in ada/expect.c there are several versions of
__gnat_expect_portable_execvp that in fact don't call the 'path' variant of the
spawn/exec call although the function is called __gnat_expect_portable_execvp.
The following patch fixes that:

--- expect.c    2003-10-31 02:08:42.000000000 +0100
+++ patched/expect.c    2005-02-25 13:31:57.000000000 +0100
@@ -80,7 +80,7 @@
 void
 __gnat_expect_portable_execvp (int *pid, char *cmd, char *argv[])
 {
-  *pid = (int) spawnve (_P_NOWAIT, cmd, argv, NULL);
+  *pid = _spawnvp (_P_NOWAIT, cmd, argv);
 }
 
 int
@@ -168,8 +168,7 @@
 __gnat_expect_portable_execvp (int *pid, char *cmd, char *argv[])
 {
   *pid = (int) getpid ();
-  /* Since cmd is fully qualified, it is incorrect to call execvp */
-  execv (cmd, argv);
+  execvp (cmd, argv);
   _exit (1);
 }
 
@@ -308,8 +307,7 @@
 __gnat_expect_portable_execvp (int *pid, char *cmd, char *argv[])
 {
   *pid = (int) getpid ();
-  /* Since cmd is fully qualified, it is incorrect to call execvp */
-  execv (cmd, argv);
+  execvp (cmd, argv);
   _exit (1);
 }
 

The comment that it is incorrect to call execvp is in fact incorrect itself. The
specification of execvp allows to call it with a fully qualified pathname and in
fact does exactly what is needed here.

With all my suggested patches applied, the handling of spawning child processes
and the way the command itself is passed to the child process are made consistent.

-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=20075

[Bug ada/20075] Bug in GNAT.Expect.Non_Blocking_Spawn

[charlet at gcc dot gnu dot org]

------- Additional Comments From charlet at gcc dot gnu dot org  2005-09-01 11:45 -------
This particular bug is already fixed in HEAD (future GCC 4.1.0).

As for the inconsistency between OS_Lib and Expect, it is intentional
and the sugget patch would simply create an incompatibility which is
not acceptable.

Arno

-- 
           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |RESOLVED
         Resolution|                            |FIXED
   Target Milestone|---                         |4.1.0


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=20075

[Bug ada/20075] Bug in GNAT.Expect.Non_Blocking_Spawn

[charlet at gcc dot gnu dot org]

------- Additional Comments From charlet at gcc dot gnu dot org  2005-09-01 11:47 -------
One last comment: the patch proposed would have actually hidden the real
problem (bad handling of path with spaces under Windows) rather than fixing it.

-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=20075