[patch,libgfortran] Handle invalid command line in EXECUTE_COMMAND_LINE

[patch,libgfortran] Handle invalid command line in EXECUTE_COMMAND_LINE

[FX]

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

The attached patch fixes PR 62296 (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=62296). The Fortran interpretation there was a bit confused (see links to comp.lang.fortran thread from the PR), but the consensus is that the standard makes a difference between the command-line returning with nonzero status (which sets EXITSTAT) and the command-line not being able to execute at all (which sets CMDSTAT and CMDMSG).

The attached patch recognizes this by checking the return value of system() for magic values 126 and 127, which are used by various combinations of shells and libc to indicate that the shell could not be executed. The attached testcase checks that.

Bootstrapped and regtested on x86_64-apple-darwin14.
OK to commit to trunk?

FX



PS: I’ve also taken the opportunity to rework a bit the runtime error message issued when CMDSTAT is not present, to actually include the actual error condition message (that which would be CMDMSG if it were present) rather than a generic error message.



[-- Attachment #2: execute_command_line.ChangeLog --]
[-- Type: application/octet-stream, Size: 441 bytes --]

2015-08-14  Francois-Xavier Coudert  <fxcoudert@gcc.gnu.org>

	PR libfortran/62296
	* intrinsics/execute_command_line.c (EXEC_INVALIDCOMMAND): New
	error code.
	(cmdmsg_values): New error message.
	(set_cmdstat): Rework runtime error.
	(execute_command_line): Handle invalid command line error status.


2015-08-14  Francois-Xavier Coudert  <fxcoudert@gcc.gnu.org>

	PR libfortran/62296
	* gfortran.dg/execute_command_line_2.f90: New test.


[-- Attachment #3: execute_command_line.diff --]
[-- Type: application/octet-stream, Size: 2362 bytes --]

Index: libgfortran/intrinsics/execute_command_line.c
===================================================================
--- libgfortran/intrinsics/execute_command_line.c	(revision 226823)
+++ libgfortran/intrinsics/execute_command_line.c	(working copy)
@@ -36,11 +36,12 @@ see the files COPYING3 and COPYING.RUNTI
 
 
 enum { EXEC_SYNCHRONOUS = -2, EXEC_NOERROR = 0, EXEC_SYSTEMFAILED,
-       EXEC_CHILDFAILED };
+       EXEC_CHILDFAILED, EXEC_INVALIDCOMMAND };
 static const char *cmdmsg_values[] =
   { "",
     "Termination status of the command-language interpreter cannot be obtained",
-    "Execution of child process impossible" };
+    "Execution of child process impossible",
+    "Invalid command line" };
 
 
 
@@ -50,7 +51,12 @@ set_cmdstat (int *cmdstat, int value)
   if (cmdstat)
     *cmdstat = value;
   else if (value > EXEC_NOERROR)
-    runtime_error ("Could not execute command line");
+    {
+#define MSGLEN 200
+      char msg[MSGLEN] = "EXECUTE_COMMAND_LINE: ";
+      strncat (msg, cmdmsg_values[value], MSGLEN - strlen(msg) - 1);
+      runtime_error (msg);
+    }
 }
 
 
@@ -95,6 +101,15 @@ execute_command_line (const char *comman
       else if (!wait)
 	set_cmdstat (cmdstat, EXEC_SYNCHRONOUS);
 #endif
+      else if (res == 127 || res == 126
+#if defined(WEXITSTATUS) && defined(WIFEXITED)
+	       || (WIFEXITED(res) && WEXITSTATUS(res) == 127)
+	       || (WIFEXITED(res) && WEXITSTATUS(res) == 126)
+#endif
+	       )
+	/* Shell return codes 126 and 127 mean that the command line could
+	   not be executed for various reasons.  */
+	set_cmdstat (cmdstat, EXEC_INVALIDCOMMAND);
       else
 	set_cmdstat (cmdstat, EXEC_NOERROR);
 
Index: gcc/testsuite/gfortran.dg/execute_command_line_2.f90
===================================================================
--- gcc/testsuite/gfortran.dg/execute_command_line_2.f90	(revision 0)
+++ gcc/testsuite/gfortran.dg/execute_command_line_2.f90	(working copy)
@@ -0,0 +1,14 @@
+! { dg-do run }
+!
+! Check that EXECUTE_COMMAND_LINE handles invalid command lines appropriately
+!
+  integer :: s = 0, c = 0
+  character(len=255) :: msg = ""
+
+  ! This should fail, set CMDSTAT to nonzero value, and an error message
+  ! in CMDMSG.
+  call execute_command_line ("/nosuchfile", exitstat=s, cmdstat=c, cmdmsg=msg)
+  if (c == 0) call abort
+  if (len_trim(msg) == 0) call abort
+
+end

Re: [patch,libgfortran] Handle invalid command line in EXECUTE_COMMAND_LINE

[FX]

10-day *ping* for my 3 Fortran patches:

- Handle invalid command line in EXECUTE_COMMAND_LINE (https://gcc.gnu.org/ml/gcc-patches/2015-08/msg00758.html)
- Use libbacktrace in libgfortran (https://gcc.gnu.org/ml/gcc-patches/2015-08/msg00762.html)
- Fix configure test for weakref support (https://gcc.gnu.org/ml/gcc-patches/2015-08/msg00796.html)

Re: [patch,libgfortran] Handle invalid command line in EXECUTE_COMMAND_LINE

[Janne Blomqvist]

On Fri, Aug 14, 2015 at 10:54 AM, FX <fxcoudert@gmail.com> wrote:
> The attached patch fixes PR 62296 (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=62296). The Fortran interpretation there was a bit confused (see links to comp.lang.fortran thread from the PR), but the consensus is that the standard makes a difference between the command-line returning with nonzero status (which sets EXITSTAT) and the command-line not being able to execute at all (which sets CMDSTAT and CMDMSG).
>
> The attached patch recognizes this by checking the return value of system() for magic values 126 and 127, which are used by various combinations of shells and libc to indicate that the shell could not be executed. The attached testcase checks that.
>
> Bootstrapped and regtested on x86_64-apple-darwin14.
> OK to commit to trunk?
>
> FX
>
>
>
> PS: I’ve also taken the opportunity to rework a bit the runtime error message issued when CMDSTAT is not present, to actually include the actual error condition message (that which would be CMDMSG if it were present) rather than a generic error message.
>
>

Otherwise looks good, but strncat is C99, and we support targets which
don't have a C99 libc (been there, done that..). Since in this case
you're dealing with string literals rather than user input, it ought
to be safe to just use plain strcat (or strlen+memcpy, if you prefer).

Ok with that change.


-- 
Janne Blomqvist

Re: [patch,libgfortran] Handle invalid command line in EXECUTE_COMMAND_LINE

[FX]

> Otherwise looks good, but strncat is C99, and we support targets which
> don't have a C99 libc (been there, done that..). Since in this case
> you're dealing with string literals rather than user input, it ought
> to be safe to just use plain strcat (or strlen+memcpy, if you prefer).

Nope, strncat() is C90: http://clc-wiki.net/wiki/strncat
Thus, safe to use, I think. I committed as submitted, as revision 227105.

Thanks for the review.
FX