* open-process and related functions for MinGW Guile
@ 2014-06-29 15:43 Eli Zaretskii
2014-06-29 20:21 ` Ludovic Courtès
0 siblings, 1 reply; 10+ messages in thread
From: Eli Zaretskii @ 2014-06-29 15:43 UTC (permalink / raw)
To: Mark H Weaver; +Cc: ludo, guile-devel
This is a sequel to the thread that started here:
http://lists.gnu.org/archive/html/guile-devel/2014-02/msg00047.html
As agreed with Mark at the end of that thread, please find below
patches that enable open-process and friends in the MinGW build of
Guile. The main changes since the patches I presented in February
are:
. Guile's standard handles are not redirected before running the
child process.
. The code which runs the child process supports both a Unixy shell
(if available), which is useful for the test suite, the stock
Windows shell cmd.exe, and other programs. This support includes
correct handling of quoted command-line arguments.
. Translation of signals to exit status and back is based on a
mapping that produces signal values identical to the ones in
signal.h, as opposed to some convention private to Guile.
. waitpid emulation supports WNOHANG (required to pass the
rnrs-libraries test).
I put most of the Windows-specific code on a separate file, I hope
that is OK.
Building the patched Guile and running the relevant tests in
ports.test and popen.test revealed some additional issues, some
related, some not entirely related. I will address them in a separate
message.
Thanks.
--- libguile/posix.c~ 2014-06-22 19:08:35.862625000 +0300
+++ libguile/posix.c 2014-06-29 18:36:02.000000000 +0300
@@ -84,6 +84,32 @@
#if HAVE_SYS_WAIT_H
# include <sys/wait.h>
#endif
+#ifdef __MINGW32__
+
+#include <c-strcase.h>
+
+# define WEXITSTATUS(stat_val) ((stat_val) & 255)
+# define WIFEXITED(stat_val) (((stat_val) & 0xC0000000) == 0)
+# define WIFSIGNALED(stat_val) (((stat_val) & 0xC0000000) == 0xC0000000)
+# define WTERMSIG(stat_val) win32_status_to_termsig (stat_val)
+/* The funny conditional avoids a compiler warning in status:stop_sig. */
+# define WIFSTOPPED(stat_val) ((stat_val) == (stat_val) ? 0 : 0)
+# define WSTOPSIG(stat_var) (0)
+# include <process.h>
+# define HAVE_WAITPID 1
+ static int win32_status_to_termsig (DWORD);
+ static int win32_signal_to_status (int);
+# define getuid() (500) /* Local Administrator */
+# define getgid() (513) /* None */
+# define setuid(u) (0)
+# define setgid(g) (0)
+# define WIN32_LEAN_AND_MEAN
+# include <windows.h>
+# define WNOHANG 1
+ int waitpid (intptr_t, int *, int);
+# include "win32-proc.c"
+#endif /* __MINGW32__ */
+
#ifndef WEXITSTATUS
# define WEXITSTATUS(stat_val) ((unsigned)(stat_val) >> 8)
#endif
@@ -659,7 +685,7 @@ SCM_DEFINE (scm_kill, "kill", 2, 0, 0,
#else
/* Mingw has raise(), but not kill(). (Other raw DOS environments might
be similar.) Use raise() when the requested pid is our own process,
- otherwise bomb. */
+ otherwise TerminateProcess. */
if (scm_to_int (pid) == getpid ())
{
if (raise (scm_to_int (sig)) != 0)
@@ -673,6 +699,25 @@ SCM_DEFINE (scm_kill, "kill", 2, 0, 0,
goto err;
}
}
+#ifdef __MINGW32__
+ else
+ {
+ HANDLE ph = OpenProcess (PROCESS_TERMINATE, 0, scm_to_int (pid));
+ int s = scm_to_int (sig);
+
+ if (!ph)
+ {
+ errno = EPERM;
+ goto err;
+ }
+ if (!TerminateProcess (ph, win32_signal_to_status (s)))
+ {
+ errno = EINVAL;
+ goto err;
+ }
+ CloseHandle (ph);
+ }
+#endif /* __MINGW32__ */
#endif
return SCM_UNSPECIFIED;
}
@@ -735,7 +780,6 @@ SCM_DEFINE (scm_waitpid, "waitpid", 1, 1
#undef FUNC_NAME
#endif /* HAVE_WAITPID */
-#ifndef __MINGW32__
SCM_DEFINE (scm_status_exit_val, "status:exit-val", 1, 0, 0,
(SCM status),
"Return the exit status value, as would be set if a process\n"
@@ -786,7 +830,6 @@ SCM_DEFINE (scm_status_stop_sig, "status
return SCM_BOOL_F;
}
#undef FUNC_NAME
-#endif /* __MINGW32__ */
#ifdef HAVE_GETPPID
SCM_DEFINE (scm_getppid, "getppid", 0, 0, 0,
@@ -801,7 +844,6 @@ SCM_DEFINE (scm_getppid, "getppid", 0, 0
#endif /* HAVE_GETPPID */
-#ifndef __MINGW32__
SCM_DEFINE (scm_getuid, "getuid", 0, 0, 0,
(),
"Return an integer representing the current real user ID.")
@@ -905,10 +947,8 @@ SCM_DEFINE (scm_seteuid, "seteuid", 1, 0
return SCM_UNSPECIFIED;
}
#undef FUNC_NAME
-#endif /* __MINGW32__ */
-#ifdef HAVE_SETEGID
SCM_DEFINE (scm_setegid, "setegid", 1, 0, 0,
(SCM id),
"Sets the effective group ID to the integer @var{id}, provided the process\n"
@@ -931,7 +971,6 @@ SCM_DEFINE (scm_setegid, "setegid", 1, 0
}
#undef FUNC_NAME
-#endif
#ifdef HAVE_GETPGRP
@@ -1141,7 +1180,7 @@ SCM_DEFINE (scm_execl, "execl", 1, 0, 1,
exec_argv = scm_i_allocate_string_pointers (args);
- execv (exec_file, exec_argv);
+ execv (exec_file, (char const * const *)exec_argv);
SCM_SYSERROR;
/* not reached. */
@@ -1170,7 +1209,7 @@ SCM_DEFINE (scm_execlp, "execlp", 1, 0,
exec_argv = scm_i_allocate_string_pointers (args);
- execvp (exec_file, exec_argv);
+ execvp (exec_file, (char const * const *)exec_argv);
SCM_SYSERROR;
/* not reached. */
@@ -1204,7 +1243,8 @@ SCM_DEFINE (scm_execle, "execle", 2, 0,
exec_argv = scm_i_allocate_string_pointers (args);
exec_env = scm_i_allocate_string_pointers (env);
- execve (exec_file, exec_argv, exec_env);
+ execve (exec_file, (char const * const *)exec_argv,
+ (char const * const *)exec_env);
SCM_SYSERROR;
/* not reached. */
@@ -1246,6 +1286,7 @@ SCM_DEFINE (scm_fork, "primitive-fork",
return scm_from_int (pid);
}
#undef FUNC_NAME
+#endif /* HAVE_FORK */
/* Since Guile uses threads, we have to be very careful to avoid calling
functions that are not async-signal-safe in the child. That's why
@@ -1262,7 +1303,9 @@ scm_open_process (SCM mode, SCM prog, SC
int pid;
char *exec_file;
char **exec_argv;
+#ifdef HAVE_FORK
int max_fd = 1024;
+#endif
exec_file = scm_to_locale_string (prog);
exec_argv = scm_i_allocate_string_pointers (scm_cons (prog, args));
@@ -1319,12 +1362,28 @@ scm_open_process (SCM mode, SCM prog, SC
}
#endif
+#ifdef HAVE_FORK
pid = fork ();
+#elif defined(__MINGW32__)
+ if (!reading)
+ c2p[1] = out;
+ if (!writing)
+ p2c[0] = in;
+ pid = start_child (exec_file, exec_argv, reading, c2p, writing, p2c, err);
+#else
+ close (c2p[0]);
+ close (c2p[1]);
+ close (p2c[0]);
+ close (p2c[1]);
+ free (exec_file);
+ errno = ENOSYS;
+ SCM_SYSERROR;
+#endif /* HAVE_FORK */
if (pid == -1)
{
int errno_save = errno;
- free (exec_file);
+
if (reading)
{
close (c2p[0]);
@@ -1336,6 +1395,8 @@ scm_open_process (SCM mode, SCM prog, SC
close (p2c[1]);
}
errno = errno_save;
+
+ free (exec_file);
SCM_SYSERROR;
}
@@ -1345,21 +1406,24 @@ scm_open_process (SCM mode, SCM prog, SC
SCM read_port = SCM_BOOL_F, write_port = SCM_BOOL_F;
/* There is no sense in catching errors on close(). */
- if (reading)
+ if (reading)
{
close (c2p[1]);
- read_port = scm_fdes_to_port (c2p[0], "r0", sym_read_pipe);
+ read_port = scm_fdes_to_port (c2p[0], "r", sym_read_pipe);
+ scm_setvbuf (read_port, scm_from_int (_IONBF), SCM_UNDEFINED);
}
if (writing)
{
close (p2c[0]);
- write_port = scm_fdes_to_port (p2c[1], "w0", sym_write_pipe);
+ write_port = scm_fdes_to_port (p2c[1], "w", sym_write_pipe);
+ scm_setvbuf (write_port, scm_from_int (_IONBF), SCM_UNDEFINED);
}
-
+
return scm_values
(scm_list_3 (read_port, write_port, scm_from_int (pid)));
}
+#ifdef HAVE_FORK
/* The child. */
if (reading)
close (c2p[0]);
@@ -1408,16 +1472,16 @@ scm_open_process (SCM mode, SCM prog, SC
if (err > 0)
{
char *msg = strerror (errno);
- fprintf (fdopen (err, "a"), "In execlp of %s: %s\n",
+ fprintf (fdopen (err, "a"), "In execvp of %s: %s\n",
exec_file, msg);
}
_exit (EXIT_FAILURE);
+#endif /* HAVE_FORK */
/* Not reached. */
return SCM_BOOL_F;
}
#undef FUNC_NAME
-#endif /* HAVE_FORK */
#ifdef __MINGW32__
# include "win32-uname.h"
@@ -2235,13 +2299,11 @@ SCM_DEFINE (scm_gethostname, "gethostnam
#endif /* HAVE_GETHOSTNAME */
\f
-#ifdef HAVE_FORK
static void
scm_init_popen (void)
{
scm_c_define_gsubr ("open-process", 2, 0, 1, scm_open_process);
}
-#endif
void
scm_init_posix ()
@@ -2334,11 +2396,11 @@ scm_init_posix ()
#ifdef HAVE_FORK
scm_add_feature ("fork");
+#endif /* HAVE_FORK */
scm_c_register_extension ("libguile-" SCM_EFFECTIVE_VERSION,
"scm_init_popen",
(scm_t_extension_init_func) scm_init_popen,
NULL);
-#endif /* HAVE_FORK */
}
/*
--- /dev/null 1970-01-01 02:00:00 +0200
+++ libguile/win32-proc.c 2014-06-29 11:26:08 +0300
@@ -0,0 +1,563 @@
+/* Run a child process with redirected standard handles, without
+ redirecting standard handles of the parent. This is required in
+ multithreaded programs, where redirecting a standard handle affects
+ all threads. */
+
+#include <stdlib.h>
+#include <string.h>
+
+/* Prepare a possibly redirected file handle to be passed to a child
+ process. The handle is for the file/device open on file descriptor
+ FD; if FD is invalid, use the null device instead.
+
+ USE_STD non-zero means we have been passed the descriptor used by
+ the parent.
+
+ ACCESS is the Windows access mode for opening the null device.
+
+ Returns the Win32 handle to be passed to CreateProcess. */
+static HANDLE
+prepare_child_handle (int fd, int use_std, DWORD access)
+{
+ HANDLE htem, hret;
+ DWORD err = 0;
+
+ /* Start with the descriptor, if specified by the caller and valid,
+ otherwise open the null device. */
+ if (fd < 0)
+ htem = INVALID_HANDLE_VALUE;
+ else
+ htem = (HANDLE)_get_osfhandle (fd);
+
+ /* Duplicate the handle and make it inheritable. */
+ if (DuplicateHandle (GetCurrentProcess (),
+ htem,
+ GetCurrentProcess (),
+ &hret,
+ 0,
+ TRUE,
+ DUPLICATE_SAME_ACCESS) == FALSE)
+ {
+ /* If the original standard handle was invalid (happens, e.g.,
+ in GUI programs), open the null device instead. */
+ if ((err = GetLastError ()) == ERROR_INVALID_HANDLE
+ && use_std)
+ {
+ htem = CreateFile ("NUL", access,
+ FILE_SHARE_READ | FILE_SHARE_WRITE, NULL,
+ OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, NULL);
+ if (htem != INVALID_HANDLE_VALUE
+ && DuplicateHandle (GetCurrentProcess (),
+ htem,
+ GetCurrentProcess (),
+ &hret,
+ 0,
+ TRUE,
+ DUPLICATE_SAME_ACCESS) == FALSE)
+ {
+ err = GetLastError ();
+ CloseHandle (htem);
+ hret = INVALID_HANDLE_VALUE;
+ }
+ }
+ }
+
+ if (hret == INVALID_HANDLE_VALUE)
+ {
+ switch (err)
+ {
+ case ERROR_NO_MORE_FILES:
+ errno = EMFILE;
+ break;
+ case ERROR_INVALID_HANDLE:
+ default:
+ errno = EBADF;
+ break;
+ }
+ }
+
+ return hret;
+}
+
+/* A comparison function for sorting the environment. */
+static int
+compenv (const void *a1, const void *a2)
+{
+ return stricmp (*((char**)a1), *((char**)a2));
+}
+
+/* Convert the program's 'environ' array to a block of environment
+ variables suitable to be passed to CreateProcess. This is needed
+ to ensure the child process inherits the up-to-date environment of
+ the parent, including any variables inserted by the parent. */
+static void
+prepare_envblk (char **envp, char **envblk)
+{
+ char **tmp;
+ int size_needed;
+ int envcnt;
+ char *ptr;
+
+ for (envcnt = 0; envp[envcnt]; envcnt++)
+ ;
+
+ tmp = scm_calloc ((envcnt + 1) * sizeof (*tmp));
+
+ for (envcnt = size_needed = 0; envp[envcnt]; envcnt++)
+ {
+ tmp[envcnt] = envp[envcnt];
+ size_needed += strlen (envp[envcnt]) + 1;
+ }
+ size_needed++;
+
+ /* Windows likes its environment variables sorted. */
+ qsort ((void *) tmp, (size_t) envcnt, sizeof (char *), compenv);
+
+ /* CreateProcess needs the environment block as a linear array,
+ where each variable is terminated by a null character, and the
+ last one is terminated by 2 null characters. */
+ ptr = *envblk = scm_calloc (size_needed);
+
+ for (envcnt = 0; tmp[envcnt]; envcnt++)
+ {
+ strcpy (ptr, tmp[envcnt]);
+ ptr += strlen (tmp[envcnt]) + 1;
+ }
+
+ free (tmp);
+}
+
+/* Find an executable PROGRAM on PATH, return result in malloc'ed
+ storage. If PROGRAM is /bin/sh, and no sh.exe was found on PATH,
+ fall back on the Windows shell and set BIN_SH_REPLACED to non-zero. */
+static char *
+lookup_cmd (const char *program, int *bin_sh_replaced)
+{
+ static const char *extensions[] = {
+ ".exe", ".cmd", ".bat", "", ".com", NULL
+ };
+ int bin_sh_requested = 0;
+ const char *path;
+ char abs_name[MAX_PATH];
+ DWORD abs_namelen;
+ int i;
+
+ /* If they ask for the Unix system shell, try to find it on PATH. */
+ if (c_strcasecmp (program, "/bin/sh") == 0)
+ {
+ bin_sh_requested = 1;
+ program = "sh.exe";
+ }
+
+ /* If PROGRAM includes leading directories, the caller already did
+ our job. */
+ if (strchr (program, '/') != NULL
+ || strchr (program, '\\') != NULL)
+ return scm_strdup (program);
+
+ /* Note: It is OK for getenv below to return NULL -- in that case,
+ SearchPath will search in the directories whose list is specified
+ by the system Registry. */
+ path = getenv ("PATH");
+ for (i = 0; extensions[i]; i++)
+ {
+ abs_namelen = SearchPath (path, program, extensions[i],
+ MAX_PATH, abs_name, NULL);
+ if (0 < abs_namelen && abs_namelen <= MAX_PATH) /* found! */
+ break;
+ }
+
+ /* If they asked for /bin/sh and we didn't find it, fall back on the
+ default Windows shell. */
+ if (abs_namelen <= 0 && bin_sh_requested)
+ {
+ const char *shell = getenv ("ComSpec");
+
+ if (!shell)
+ shell = "C:\\Windows\\system32\\cmd.exe";
+
+ *bin_sh_replaced = 1;
+ strcpy (abs_name, shell);
+ abs_namelen = strlen (abs_name);
+ }
+
+ /* If not found, return the original PROGRAM name. */
+ if (abs_namelen <= 0 || abs_namelen > MAX_PATH)
+ return scm_strdup (program);
+
+ return scm_strndup (abs_name, abs_namelen);
+}
+
+/* Concatenate command-line arguments in argv[] into a single
+ command-line string, while quoting arguments as needed. The result
+ is malloc'ed. */
+static char *
+prepare_cmdline (const char *cmd, const char * const *argv, int bin_sh_replaced)
+{
+ /* These characters should include anything that is special to _any_
+ program, including both Windows and Unixy shells, and the
+ widlcard expansion in startup code of a typical Windows app. */
+ const char need_quotes[] = " \t#;\"\'*?[]&|<>(){}$`^";
+ size_t cmdlen = 1; /* for terminating null */
+ char *cmdline = scm_malloc (cmdlen);
+ char *dst = cmdline;
+ int cmd_exe_quoting = 0;
+ int i;
+ const char *p;
+
+ /* Are we constructing a command line for cmd.exe? */
+ if (bin_sh_replaced)
+ cmd_exe_quoting = 1;
+ else
+ {
+ for (p = cmd + strlen (cmd);
+ p > cmd && p[-1] != '/' && p[-1] != '\\' && p[-1] != ':';
+ p--)
+ ;
+ if (c_strcasecmp (p, "cmd.exe") == 0
+ || c_strcasecmp (p, "cmd") == 0)
+ cmd_exe_quoting = 1;
+ }
+
+ /* Initialize the command line to empty. */
+ *dst = '\0';
+
+ /* Append arguments, if any, from argv[]. */
+ for (i = 0; argv[i]; i++)
+ {
+ const char *src = argv[i];
+ size_t len;
+ int quote_this = 0, n_backslashes = 0;
+ int j;
+
+ /* Append the blank separator. We don't do that for argv[0]
+ because that is the command name (will end up in child's
+ argv[0]), and is only recognized as such if there're no
+ blanks before it. */
+ if (i > 0)
+ *dst++ = ' ';
+ len = dst - cmdline;
+
+ /* How much space is required for this argument? */
+ cmdlen += strlen (argv[i]) + 1; /* 1 for a blank separator */
+ /* cmd.exe needs a different style of quoting: all the arguments
+ beyond the /c switch are enclosed in an extra pair of quotes,
+ and not otherwise quoted/escaped. */
+ if (cmd_exe_quoting)
+ {
+ if (i == 2)
+ cmdlen += 2;
+ }
+ else if (strpbrk (argv[i], need_quotes))
+ {
+ quote_this = 1;
+ cmdlen += 2;
+ for ( ; *src; src++)
+ {
+ /* An embedded quote needs to be escaped by a backslash.
+ Any backslashes immediately preceding that quote need
+ each one to be escaped by another backslash. */
+ if (*src == '\"')
+ cmdlen += n_backslashes + 1;
+ if (*src == '\\')
+ n_backslashes++;
+ else
+ n_backslashes = 0;
+ }
+ /* If the closing quote we will add is preceded by
+ backslashes, those backslashes need to be escaped. */
+ cmdlen += n_backslashes;
+ }
+
+ /* Enlarge the command-line string as needed. */
+ cmdline = scm_realloc (cmdline, cmdlen);
+ dst = cmdline + len;
+
+ if (i == 0
+ && c_strcasecmp (argv[0], "/bin/sh") == 0
+ && bin_sh_replaced)
+ {
+ strcpy (dst, "cmd.exe");
+ dst += sizeof ("cmd.exe") - 1;
+ continue;
+ }
+ if (i == 1 && bin_sh_replaced && strcmp (argv[1], "-c") == 0)
+ {
+ *dst++ = '/';
+ *dst++ = 'c';
+ *dst = '\0';
+ continue;
+ }
+
+ /* Add this argument, possibly quoted, to the command line. */
+ if (quote_this || (i == 2 && cmd_exe_quoting))
+ *dst++ = '\"';
+ for (src = argv[i]; *src; src++)
+ {
+ if (quote_this)
+ {
+ if (*src == '\"')
+ for (j = n_backslashes + 1; j > 0; j--)
+ *dst++ = '\\';
+ if (*src == '\\')
+ n_backslashes++;
+ else
+ n_backslashes = 0;
+ }
+ *dst++ = *src;
+ }
+ if (quote_this)
+ {
+ for (j = n_backslashes; j > 0; j--)
+ *dst++ = '\\';
+ *dst++ = '\"';
+ }
+ *dst = '\0';
+ }
+
+ if (cmd_exe_quoting && i > 2)
+ {
+ /* One extra slot was already reserved when we enlarged cmdlen
+ by 2 in the "if (cmd_exe_quoting)" clause above. So we can
+ safely append a closing quote. */
+ *dst++ = '\"';
+ *dst = '\0';
+ }
+
+ return cmdline;
+}
+
+/* Start a child process running the program in EXEC_FILE with its
+ standard input and output optionally redirected to a pipe. ARGV is
+ the array of command-line arguments to pass to the child. P2C and
+ C2P are 2 pipes for communicating with the child, and ERRFD is the
+ standard error file descriptor to be inherited by the child.
+ READING and WRITING, if non-zero, mean that the corresponding pipe
+ will be used.
+
+ Return the PID of the child process, or -1 if couldn't start a
+ process. */
+static intptr_t
+start_child (const char *exec_file, char **argv,
+ int reading, int c2p[2], int writing, int p2c[2], int errfd)
+{
+ HANDLE hin = INVALID_HANDLE_VALUE, hout = INVALID_HANDLE_VALUE;
+ HANDLE herr = INVALID_HANDLE_VALUE;
+ STARTUPINFO si;
+ char *env_block = NULL;
+ char *cmdline = NULL;
+ PROCESS_INFORMATION pi;
+ char *progfile, *p;
+ int errno_save;
+ intptr_t pid;
+ int bin_sh_replaced = 0;
+
+ /* Prepare standard handles to be passed to the child process. */
+ hin = prepare_child_handle (p2c[0], !writing, GENERIC_READ);
+ if (hin == INVALID_HANDLE_VALUE)
+ return -1;
+ hout = prepare_child_handle (c2p[1], !reading, GENERIC_WRITE);
+ if (hout == INVALID_HANDLE_VALUE)
+ return -1;
+ herr = prepare_child_handle (errfd, 1, GENERIC_WRITE);
+ if (herr == INVALID_HANDLE_VALUE)
+ return -1;
+
+ /* Make sure the parent side of both pipes is not inherited. This
+ is required because gnulib's 'pipe' creates pipes whose both ends
+ are inheritable, which is traditional on Posix (where pipe
+ descriptors are implicitly duplicated by 'fork'), but wrong on
+ Windows (where pipe handles need to be explicitly
+ duplicated). */
+ if (writing)
+ SetHandleInformation ((HANDLE)_get_osfhandle (p2c[1]),
+ HANDLE_FLAG_INHERIT, 0);
+ if (reading)
+ {
+ SetHandleInformation ((HANDLE)_get_osfhandle (c2p[0]),
+ HANDLE_FLAG_INHERIT, 0);
+ /* Gnulib's 'pipe' opens the pipe in binary mode, but we don't
+ want to read text-mode input of subprocesses in binary more,
+ because then we will get the ^M (a.k.a. "CR") characters we
+ don't expect. */
+ _setmode (c2p[0], _O_TEXT);
+ }
+
+ /* Set up the startup info for the child, using the parent's as the
+ starting point, and specify in it the redirected handles. */
+ GetStartupInfo (&si);
+ si.dwFlags = STARTF_USESTDHANDLES;
+ si.lpReserved = 0;
+ si.cbReserved2 = 0;
+ si.lpReserved2 = 0;
+ si.hStdInput = hin;
+ si.hStdOutput = hout;
+ si.hStdError = herr;
+
+ /* Create the environment block for the child. This is needed
+ because the environment we have in 'environ' is not in the format
+ expected by CreateProcess. */
+ prepare_envblk (environ, &env_block);
+
+ /* CreateProcess doesn't search PATH, so we must do that for it. */
+ progfile = lookup_cmd (exec_file, &bin_sh_replaced);
+
+ /* CreateProcess doesn't like forward slashes in the application
+ file name. */
+ for (p = progfile; *p; p++)
+ if (*p == '/')
+ *p = '\\';
+
+ /* Construct the command line. */
+ cmdline = prepare_cmdline (exec_file, (const char * const *)argv,
+ bin_sh_replaced);
+
+ /* All set and ready to fly. Launch the child process. */
+ if (!CreateProcess (progfile, cmdline, NULL, NULL, TRUE, 0, env_block, NULL,
+ &si, &pi))
+ {
+ pid = -1;
+
+ /* Since we use Win32 APIs directly, we need to translate their
+ errors to errno values by hand. */
+ switch (GetLastError ())
+ {
+ case ERROR_FILE_NOT_FOUND:
+ case ERROR_PATH_NOT_FOUND:
+ case ERROR_INVALID_DRIVE:
+ case ERROR_BAD_PATHNAME:
+ errno = ENOENT;
+ break;
+ case ERROR_ACCESS_DENIED:
+ errno = EACCES;
+ break;
+ case ERROR_BAD_ENVIRONMENT:
+ errno = E2BIG;
+ break;
+ case ERROR_BROKEN_PIPE:
+ errno = EPIPE;
+ break;
+ case ERROR_INVALID_HANDLE:
+ errno = EBADF;
+ break;
+ case ERROR_MAX_THRDS_REACHED:
+ errno = EAGAIN;
+ break;
+ case ERROR_BAD_EXE_FORMAT:
+ case ERROR_BAD_FORMAT:
+ default:
+ errno = ENOEXEC;
+ break;
+ }
+ }
+ else
+ pid = (intptr_t)pi.hProcess;
+
+ errno_save = errno;
+
+ /* Free resources. */
+ free (progfile);
+ free (cmdline);
+ free (env_block);
+ CloseHandle (hin);
+ CloseHandle (hout);
+ CloseHandle (herr);
+ CloseHandle (pi.hThread);
+
+ /* Posix requires to call the shell if execvp fails to invoke EXEC_FILE. */
+ if (errno_save == ENOEXEC || errno_save == ENOENT)
+ {
+ const char *shell = getenv ("ComSpec");
+
+ if (!shell)
+ shell = "cmd.exe";
+
+ if (c_strcasecmp (exec_file, shell) != 0)
+ {
+ argv[0] = (char *)exec_file;
+ return start_child (shell, argv, reading, c2p, writing, p2c, errfd);
+ }
+ }
+
+ errno = errno_save;
+ return pid;
+}
+
+\f
+/* Emulation of waitpid which only supports WNOHANG, since _cwait doesn't. */
+int
+waitpid (intptr_t pid, int *status, int options)
+{
+ if ((options & WNOHANG) != 0)
+ {
+ DWORD st;
+
+ if (!GetExitCodeProcess ((HANDLE)pid, &st))
+ {
+ errno = ECHILD;
+ return -1;
+ }
+ if (st == STILL_ACTIVE)
+ return 0;
+ if (status)
+ *status = st;
+ return (int)pid;
+ }
+
+ return (int)_cwait (status, pid, WAIT_CHILD);
+}
+
+\f
+/* Translate abnormal exit status of Windows programs into the signal
+ that terminated the program. This is required to support scm_kill
+ and WTERMSIG. */
+
+struct signal_and_status {
+ int sig;
+ DWORD status;
+};
+
+static const struct signal_and_status sigtbl[] = {
+ {SIGSEGV, 0xC0000005}, /* access to invalid address */
+ {SIGSEGV, 0xC0000008}, /* invalid handle */
+ {SIGILL, 0xC000001D}, /* illegal instruction */
+ {SIGILL, 0xC0000025}, /* non-continuable instruction */
+ {SIGSEGV, 0xC000008C}, /* array bounds exceeded */
+ {SIGFPE, 0xC000008D}, /* float denormal */
+ {SIGFPE, 0xC000008E}, /* float divide by zero */
+ {SIGFPE, 0xC000008F}, /* float inexact */
+ {SIGFPE, 0xC0000090}, /* float invalid operation */
+ {SIGFPE, 0xC0000091}, /* float overflow */
+ {SIGFPE, 0xC0000092}, /* float stack check */
+ {SIGFPE, 0xC0000093}, /* float underflow */
+ {SIGFPE, 0xC0000094}, /* integer divide by zero */
+ {SIGFPE, 0xC0000095}, /* integer overflow */
+ {SIGILL, 0xC0000096}, /* privileged instruction */
+ {SIGSEGV, 0xC00000FD}, /* stack overflow */
+ {SIGTERM, 0xC000013A}, /* Ctrl-C exit */
+ {SIGINT, 0xC000013A}
+};
+
+static int
+win32_signal_to_status (int sig)
+{
+ int i;
+
+ for (i = 0; i < sizeof (sigtbl) / sizeof (sigtbl[0]); i++)
+ if (sig == sigtbl[i].sig)
+ return sigtbl[i].status;
+
+ return (int)0xC000013A;
+}
+
+static int
+win32_status_to_termsig (DWORD status)
+{
+ int i;
+
+ for (i = 0; i < sizeof (sigtbl) / sizeof (sigtbl[0]); i++)
+ if (status == sigtbl[i].status)
+ return sigtbl[i].sig;
+
+ return SIGTERM;
+}
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: open-process and related functions for MinGW Guile
2014-06-29 15:43 open-process and related functions for MinGW Guile Eli Zaretskii
@ 2014-06-29 20:21 ` Ludovic Courtès
2014-06-30 2:49 ` Eli Zaretskii
0 siblings, 1 reply; 10+ messages in thread
From: Ludovic Courtès @ 2014-06-29 20:21 UTC (permalink / raw)
To: Eli Zaretskii; +Cc: Mark H Weaver, guile-devel
Hello, Eli,
Eli Zaretskii <eliz@gnu.org> skribis:
> This is a sequel to the thread that started here:
>
> http://lists.gnu.org/archive/html/guile-devel/2014-02/msg00047.html
>
> As agreed with Mark at the end of that thread, please find below
> patches that enable open-process and friends in the MinGW build of
> Guile. The main changes since the patches I presented in February
> are:
>
> . Guile's standard handles are not redirected before running the
> child process.
>
> . The code which runs the child process supports both a Unixy shell
> (if available), which is useful for the test suite, the stock
> Windows shell cmd.exe, and other programs. This support includes
> correct handling of quoted command-line arguments.
>
> . Translation of signals to exit status and back is based on a
> mapping that produces signal values identical to the ones in
> signal.h, as opposed to some convention private to Guile.
>
> . waitpid emulation supports WNOHANG (required to pass the
> rnrs-libraries test).
Nice!
Preliminary comments below.
> --- libguile/posix.c~ 2014-06-22 19:08:35.862625000 +0300
> +++ libguile/posix.c 2014-06-29 18:36:02.000000000 +0300
> @@ -84,6 +84,32 @@
> #if HAVE_SYS_WAIT_H
> # include <sys/wait.h>
> #endif
> +#ifdef __MINGW32__
> +
> +#include <c-strcase.h>
> +
> +# define WEXITSTATUS(stat_val) ((stat_val) & 255)
> +# define WIFEXITED(stat_val) (((stat_val) & 0xC0000000) == 0)
> +# define WIFSIGNALED(stat_val) (((stat_val) & 0xC0000000) == 0xC0000000)
> +# define WTERMSIG(stat_val) win32_status_to_termsig (stat_val)
> +/* The funny conditional avoids a compiler warning in status:stop_sig. */
> +# define WIFSTOPPED(stat_val) ((stat_val) == (stat_val) ? 0 : 0)
> +# define WSTOPSIG(stat_var) (0)
I think this was raised in the previous discussion: it looks a bit like
black magic, so there should be a comment explaining why this is needed,
how the constants were chosen, etc.
In addition...
> +# include <process.h>
> +# define HAVE_WAITPID 1
> + static int win32_status_to_termsig (DWORD);
> + static int win32_signal_to_status (int);
> +# define getuid() (500) /* Local Administrator */
> +# define getgid() (513) /* None */
> +# define setuid(u) (0)
> +# define setgid(g) (0)
> +# define WIN32_LEAN_AND_MEAN
> +# include <windows.h>
> +# define WNOHANG 1
> + int waitpid (intptr_t, int *, int);
> +# include "win32-proc.c"
... what would you think of putting all this in a Gnulib module? It
would benefit all GNU packages and probably get more testing.
> -#ifdef HAVE_SETEGID
> SCM_DEFINE (scm_setegid, "setegid", 1, 0, 0,
> (SCM id),
> "Sets the effective group ID to the integer @var{id}, provided the process\n"
This should be a separate change, and it’s dubious since there could be
platforms without setegid.
> exec_argv = scm_i_allocate_string_pointers (args);
>
> - execv (exec_file, exec_argv);
> + execv (exec_file, (char const * const *)exec_argv);
This should be a separate change (if at all needed.)
> - if (reading)
> + if (reading)
> {
> close (c2p[1]);
> - read_port = scm_fdes_to_port (c2p[0], "r0", sym_read_pipe);
> + read_port = scm_fdes_to_port (c2p[0], "r", sym_read_pipe);
> + scm_setvbuf (read_port, scm_from_int (_IONBF), SCM_UNDEFINED);
> }
> if (writing)
> {
> close (p2c[0]);
> - write_port = scm_fdes_to_port (p2c[1], "w0", sym_write_pipe);
> + write_port = scm_fdes_to_port (p2c[1], "w", sym_write_pipe);
> + scm_setvbuf (write_port, scm_from_int (_IONBF), SCM_UNDEFINED);
This reverts a43fa1b. Could you explain why it’s needed, and make it a
separate patch?
> --- /dev/null 1970-01-01 02:00:00 +0200
> +++ libguile/win32-proc.c 2014-06-29 11:26:08 +0300
Please call it “w32-proc.c” or “woe32-proc.c”, and add the LGPLv3+
license header.
> +/* Translate abnormal exit status of Windows programs into the signal
> + that terminated the program. This is required to support scm_kill
> + and WTERMSIG. */
> +
> +struct signal_and_status {
> + int sig;
> + DWORD status;
> +};
> +
> +static const struct signal_and_status sigtbl[] = {
> + {SIGSEGV, 0xC0000005}, /* access to invalid address */
Opening braces on a line of their own; spaces around braces.
Thank you,
Ludo’.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: open-process and related functions for MinGW Guile
2014-06-29 20:21 ` Ludovic Courtès
@ 2014-06-30 2:49 ` Eli Zaretskii
2014-08-09 14:13 ` Eli Zaretskii
0 siblings, 1 reply; 10+ messages in thread
From: Eli Zaretskii @ 2014-06-30 2:49 UTC (permalink / raw)
To: Ludovic Courtès; +Cc: mhw, guile-devel
> From: ludo@gnu.org (Ludovic Courtès)
> Cc: Mark H Weaver <mhw@netris.org>, guile-devel@gnu.org
> Date: Sun, 29 Jun 2014 22:21:28 +0200
>
> > +#ifdef __MINGW32__
> > +
> > +#include <c-strcase.h>
> > +
> > +# define WEXITSTATUS(stat_val) ((stat_val) & 255)
> > +# define WIFEXITED(stat_val) (((stat_val) & 0xC0000000) == 0)
> > +# define WIFSIGNALED(stat_val) (((stat_val) & 0xC0000000) == 0xC0000000)
> > +# define WTERMSIG(stat_val) win32_status_to_termsig (stat_val)
> > +/* The funny conditional avoids a compiler warning in status:stop_sig. */
> > +# define WIFSTOPPED(stat_val) ((stat_val) == (stat_val) ? 0 : 0)
> > +# define WSTOPSIG(stat_var) (0)
>
> I think this was raised in the previous discussion: it looks a bit like
> black magic, so there should be a comment explaining why this is needed,
> how the constants were chosen, etc.
Most of the magic is gone in this version. I will add a comment about
0xC0000000.
> > +# include <process.h>
> > +# define HAVE_WAITPID 1
> > + static int win32_status_to_termsig (DWORD);
> > + static int win32_signal_to_status (int);
> > +# define getuid() (500) /* Local Administrator */
> > +# define getgid() (513) /* None */
> > +# define setuid(u) (0)
> > +# define setgid(g) (0)
> > +# define WIN32_LEAN_AND_MEAN
> > +# include <windows.h>
> > +# define WNOHANG 1
> > + int waitpid (intptr_t, int *, int);
> > +# include "win32-proc.c"
>
> ... what would you think of putting all this in a Gnulib module? It
> would benefit all GNU packages and probably get more testing.
Gnulib already has such a module, but its design and implementation is
based on wrong premises. We've been through that with Mark back in
February.
And my experience with Gnulib responsiveness hasn't changed much since
then: 2 tiny patches I submitted were accepted, but a larger patch to
nl_langinfo, which is very important for Guile, was left without a
comment for the past 3 weeks.
> > -#ifdef HAVE_SETEGID
> > SCM_DEFINE (scm_setegid, "setegid", 1, 0, 0,
> > (SCM id),
> > "Sets the effective group ID to the integer @var{id}, provided the process\n"
>
> This should be a separate change, and it’s dubious since there could be
> platforms without setegid.
Which ones?
> > exec_argv = scm_i_allocate_string_pointers (args);
> >
> > - execv (exec_file, exec_argv);
> > + execv (exec_file, (char const * const *)exec_argv);
>
> This should be a separate change (if at all needed.)
It fixes a compiler warning.
> > - if (reading)
> > + if (reading)
> > {
> > close (c2p[1]);
> > - read_port = scm_fdes_to_port (c2p[0], "r0", sym_read_pipe);
> > + read_port = scm_fdes_to_port (c2p[0], "r", sym_read_pipe);
> > + scm_setvbuf (read_port, scm_from_int (_IONBF), SCM_UNDEFINED);
> > }
> > if (writing)
> > {
> > close (p2c[0]);
> > - write_port = scm_fdes_to_port (p2c[1], "w0", sym_write_pipe);
> > + write_port = scm_fdes_to_port (p2c[1], "w", sym_write_pipe);
> > + scm_setvbuf (write_port, scm_from_int (_IONBF), SCM_UNDEFINED);
>
> This reverts a43fa1b. Could you explain why it’s needed, and make it a
> separate patch?
Ignore this, I wasn't aware a change was made there.
> > --- /dev/null 1970-01-01 02:00:00 +0200
> > +++ libguile/win32-proc.c 2014-06-29 11:26:08 +0300
>
> Please call it “w32-proc.c” or “woe32-proc.c”
I was just following the example of win32-uname.c.
Thanks for the other feedback.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: open-process and related functions for MinGW Guile
2014-06-30 2:49 ` Eli Zaretskii
@ 2014-08-09 14:13 ` Eli Zaretskii
2014-08-12 18:08 ` Mark H Weaver
0 siblings, 1 reply; 10+ messages in thread
From: Eli Zaretskii @ 2014-08-09 14:13 UTC (permalink / raw)
To: ludo, mhw; +Cc: guile-devel
> Date: Mon, 30 Jun 2014 05:49:52 +0300
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: mhw@netris.org, guile-devel@gnu.org
Ping! I've been waiting for comments from Mark, but those never came.
Meanwhile I implemented the changes requested by Ludo, see the updated
patch below.
I hope this will be finally admitted into the repo.
TIA
>
> > From: ludo@gnu.org (Ludovic Courtès)
> > Cc: Mark H Weaver <mhw@netris.org>, guile-devel@gnu.org
> > Date: Sun, 29 Jun 2014 22:21:28 +0200
> >
> > > +#ifdef __MINGW32__
> > > +
> > > +#include <c-strcase.h>
> > > +
> > > +# define WEXITSTATUS(stat_val) ((stat_val) & 255)
> > > +# define WIFEXITED(stat_val) (((stat_val) & 0xC0000000) == 0)
> > > +# define WIFSIGNALED(stat_val) (((stat_val) & 0xC0000000) == 0xC0000000)
> > > +# define WTERMSIG(stat_val) win32_status_to_termsig (stat_val)
> > > +/* The funny conditional avoids a compiler warning in status:stop_sig. */
> > > +# define WIFSTOPPED(stat_val) ((stat_val) == (stat_val) ? 0 : 0)
> > > +# define WSTOPSIG(stat_var) (0)
> >
> > I think this was raised in the previous discussion: it looks a bit like
> > black magic, so there should be a comment explaining why this is needed,
> > how the constants were chosen, etc.
>
> Most of the magic is gone in this version. I will add a comment about
> 0xC0000000.
>
> > > +# include <process.h>
> > > +# define HAVE_WAITPID 1
> > > + static int win32_status_to_termsig (DWORD);
> > > + static int win32_signal_to_status (int);
> > > +# define getuid() (500) /* Local Administrator */
> > > +# define getgid() (513) /* None */
> > > +# define setuid(u) (0)
> > > +# define setgid(g) (0)
> > > +# define WIN32_LEAN_AND_MEAN
> > > +# include <windows.h>
> > > +# define WNOHANG 1
> > > + int waitpid (intptr_t, int *, int);
> > > +# include "win32-proc.c"
> >
> > ... what would you think of putting all this in a Gnulib module? It
> > would benefit all GNU packages and probably get more testing.
>
> Gnulib already has such a module, but its design and implementation is
> based on wrong premises. We've been through that with Mark back in
> February.
>
> And my experience with Gnulib responsiveness hasn't changed much since
> then: 2 tiny patches I submitted were accepted, but a larger patch to
> nl_langinfo, which is very important for Guile, was left without a
> comment for the past 3 weeks.
>
> > > -#ifdef HAVE_SETEGID
> > > SCM_DEFINE (scm_setegid, "setegid", 1, 0, 0,
> > > (SCM id),
> > > "Sets the effective group ID to the integer @var{id}, provided the process\n"
> >
> > This should be a separate change, and it’s dubious since there could be
> > platforms without setegid.
>
> Which ones?
>
> > > exec_argv = scm_i_allocate_string_pointers (args);
> > >
> > > - execv (exec_file, exec_argv);
> > > + execv (exec_file, (char const * const *)exec_argv);
> >
> > This should be a separate change (if at all needed.)
>
> It fixes a compiler warning.
>
> > > - if (reading)
> > > + if (reading)
> > > {
> > > close (c2p[1]);
> > > - read_port = scm_fdes_to_port (c2p[0], "r0", sym_read_pipe);
> > > + read_port = scm_fdes_to_port (c2p[0], "r", sym_read_pipe);
> > > + scm_setvbuf (read_port, scm_from_int (_IONBF), SCM_UNDEFINED);
> > > }
> > > if (writing)
> > > {
> > > close (p2c[0]);
> > > - write_port = scm_fdes_to_port (p2c[1], "w0", sym_write_pipe);
> > > + write_port = scm_fdes_to_port (p2c[1], "w", sym_write_pipe);
> > > + scm_setvbuf (write_port, scm_from_int (_IONBF), SCM_UNDEFINED);
> >
> > This reverts a43fa1b. Could you explain why it’s needed, and make it a
> > separate patch?
>
> Ignore this, I wasn't aware a change was made there.
>
> > > --- /dev/null 1970-01-01 02:00:00 +0200
> > > +++ libguile/win32-proc.c 2014-06-29 11:26:08 +0300
> >
> > Please call it “w32-proc.c” or “woe32-proc.c”
>
> I was just following the example of win32-uname.c.
>
> Thanks for the other feedback.
Here's the updated patch:
--- libguile/posix.c.~1~ 2014-02-28 22:01:27.000000000 +0200
+++ libguile/posix.c 2014-08-08 17:27:50.339267200 +0300
@@ -84,6 +84,42 @@
#if HAVE_SYS_WAIT_H
# include <sys/wait.h>
#endif
+#ifdef __MINGW32__
+
+#include <c-strcase.h>
+
+# define WEXITSTATUS(stat_val) ((stat_val) & 255)
+/* MS-Windows programs that crash due to a fatal exception exit with
+ an exit code whose 2 MSB bits are set. */
+# define WIFEXITED(stat_val) (((stat_val) & 0xC0000000) == 0)
+# define WIFSIGNALED(stat_val) (((stat_val) & 0xC0000000) == 0xC0000000)
+# define WTERMSIG(stat_val) w32_status_to_termsig (stat_val)
+/* The funny conditional avoids a compiler warning in status:stop_sig. */
+# define WIFSTOPPED(stat_val) ((stat_val) == (stat_val) ? 0 : 0)
+# define WSTOPSIG(stat_var) (0)
+# include <process.h>
+# define HAVE_WAITPID 1
+ static int w32_status_to_termsig (DWORD);
+ static int w32_signal_to_status (int);
+# define getuid() (500) /* Local Administrator */
+# define getgid() (513) /* None */
+# define setuid(u) (0)
+# define setgid(g) (0)
+# define WIN32_LEAN_AND_MEAN
+# include <windows.h>
+# define WNOHANG 1
+ int waitpid (intptr_t, int *, int);
+
+ typedef DWORD_PTR cpu_set_t;
+
+#define CPU_ZERO(s) memset(s,0,sizeof(*s))
+#define CPU_ISSET(b,s) ((*s) & (1U << (b))) != 0
+#define CPU_SET(b,s) (*s) |= (1U << (b))
+#define CPU_SETSIZE (8*sizeof(DWORD_PTR))
+
+# include "w32-proc.c"
+#endif /* __MINGW32__ */
+
#ifndef WEXITSTATUS
# define WEXITSTATUS(stat_val) ((unsigned)(stat_val) >> 8)
#endif
@@ -659,7 +695,7 @@ SCM_DEFINE (scm_kill, "kill", 2, 0, 0,
#else
/* Mingw has raise(), but not kill(). (Other raw DOS environments might
be similar.) Use raise() when the requested pid is our own process,
- otherwise bomb. */
+ otherwise TerminateProcess. */
if (scm_to_int (pid) == getpid ())
{
if (raise (scm_to_int (sig)) != 0)
@@ -673,6 +709,25 @@ SCM_DEFINE (scm_kill, "kill", 2, 0, 0,
goto err;
}
}
+#ifdef __MINGW32__
+ else
+ {
+ HANDLE ph = OpenProcess (PROCESS_TERMINATE, 0, scm_to_int (pid));
+ int s = scm_to_int (sig);
+
+ if (!ph)
+ {
+ errno = EPERM;
+ goto err;
+ }
+ if (!TerminateProcess (ph, w32_signal_to_status (s)))
+ {
+ errno = EINVAL;
+ goto err;
+ }
+ CloseHandle (ph);
+ }
+#endif /* __MINGW32__ */
#endif
return SCM_UNSPECIFIED;
}
@@ -735,7 +790,6 @@ SCM_DEFINE (scm_waitpid, "waitpid", 1, 1
#undef FUNC_NAME
#endif /* HAVE_WAITPID */
-#ifndef __MINGW32__
SCM_DEFINE (scm_status_exit_val, "status:exit-val", 1, 0, 0,
(SCM status),
"Return the exit status value, as would be set if a process\n"
@@ -786,7 +840,6 @@ SCM_DEFINE (scm_status_stop_sig, "status
return SCM_BOOL_F;
}
#undef FUNC_NAME
-#endif /* __MINGW32__ */
#ifdef HAVE_GETPPID
SCM_DEFINE (scm_getppid, "getppid", 0, 0, 0,
@@ -801,7 +854,6 @@ SCM_DEFINE (scm_getppid, "getppid", 0, 0
#endif /* HAVE_GETPPID */
-#ifndef __MINGW32__
SCM_DEFINE (scm_getuid, "getuid", 0, 0, 0,
(),
"Return an integer representing the current real user ID.")
@@ -905,10 +957,8 @@ SCM_DEFINE (scm_seteuid, "seteuid", 1, 0
return SCM_UNSPECIFIED;
}
#undef FUNC_NAME
-#endif /* __MINGW32__ */
-#ifdef HAVE_SETEGID
SCM_DEFINE (scm_setegid, "setegid", 1, 0, 0,
(SCM id),
"Sets the effective group ID to the integer @var{id}, provided the process\n"
@@ -931,7 +981,6 @@ SCM_DEFINE (scm_setegid, "setegid", 1, 0
}
#undef FUNC_NAME
-#endif
#ifdef HAVE_GETPGRP
@@ -1141,7 +1190,7 @@ SCM_DEFINE (scm_execl, "execl", 1, 0, 1,
exec_argv = scm_i_allocate_string_pointers (args);
- execv (exec_file, exec_argv);
+ execv (exec_file, (char const * const *)exec_argv);
SCM_SYSERROR;
/* not reached. */
@@ -1170,7 +1219,7 @@ SCM_DEFINE (scm_execlp, "execlp", 1, 0,
exec_argv = scm_i_allocate_string_pointers (args);
- execvp (exec_file, exec_argv);
+ execvp (exec_file, (char const * const *)exec_argv);
SCM_SYSERROR;
/* not reached. */
@@ -1204,7 +1253,8 @@ SCM_DEFINE (scm_execle, "execle", 2, 0,
exec_argv = scm_i_allocate_string_pointers (args);
exec_env = scm_i_allocate_string_pointers (env);
- execve (exec_file, exec_argv, exec_env);
+ execve (exec_file, (char const * const *)exec_argv,
+ (char const * const *)exec_env);
SCM_SYSERROR;
/* not reached. */
@@ -1246,6 +1296,7 @@ SCM_DEFINE (scm_fork, "primitive-fork",
return scm_from_int (pid);
}
#undef FUNC_NAME
+#endif /* HAVE_FORK */
/* Since Guile uses threads, we have to be very careful to avoid calling
functions that are not async-signal-safe in the child. That's why
@@ -1262,7 +1313,9 @@ scm_open_process (SCM mode, SCM prog, SC
int pid;
char *exec_file;
char **exec_argv;
+#ifdef HAVE_FORK
int max_fd = 1024;
+#endif
exec_file = scm_to_locale_string (prog);
exec_argv = scm_i_allocate_string_pointers (scm_cons (prog, args));
@@ -1319,12 +1372,28 @@ scm_open_process (SCM mode, SCM prog, SC
}
#endif
+#ifdef HAVE_FORK
pid = fork ();
+#elif defined(__MINGW32__)
+ if (!reading)
+ c2p[1] = out;
+ if (!writing)
+ p2c[0] = in;
+ pid = start_child (exec_file, exec_argv, reading, c2p, writing, p2c, err);
+#else
+ close (c2p[0]);
+ close (c2p[1]);
+ close (p2c[0]);
+ close (p2c[1]);
+ free (exec_file);
+ errno = ENOSYS;
+ SCM_SYSERROR;
+#endif /* HAVE_FORK */
if (pid == -1)
{
int errno_save = errno;
- free (exec_file);
+
if (reading)
{
close (c2p[0]);
@@ -1336,6 +1405,8 @@ scm_open_process (SCM mode, SCM prog, SC
close (p2c[1]);
}
errno = errno_save;
+
+ free (exec_file);
SCM_SYSERROR;
}
@@ -1361,7 +1432,8 @@ scm_open_process (SCM mode, SCM prog, SC
return scm_values
(scm_list_3 (read_port, write_port, scm_from_int (pid)));
}
-
+
+#ifdef HAVE_FORK
/* The child. */
if (reading)
close (c2p[0]);
@@ -1410,16 +1482,16 @@ scm_open_process (SCM mode, SCM prog, SC
if (err > 0)
{
char *msg = strerror (errno);
- fprintf (fdopen (err, "a"), "In execlp of %s: %s\n",
+ fprintf (fdopen (err, "a"), "In execvp of %s: %s\n",
exec_file, msg);
}
_exit (EXIT_FAILURE);
+#endif /* HAVE_FORK */
/* Not reached. */
return SCM_BOOL_F;
}
#undef FUNC_NAME
-#endif /* HAVE_FORK */
#ifdef __MINGW32__
# include "win32-uname.h"
--- /dev/null 1970-01-01 02:00:00 +0200
+++ libguile/w32-proc.c 2014-06-29 11:26:08 +0300
@@ -0,0 +1,563 @@
+/* Run a child process with redirected standard handles, without
+ redirecting standard handles of the parent. This is required in
+ multithreaded programs, where redirecting a standard handle affects
+ all threads. */
+
+#include <stdlib.h>
+#include <string.h>
+
+/* Prepare a possibly redirected file handle to be passed to a child
+ process. The handle is for the file/device open on file descriptor
+ FD; if FD is invalid, use the null device instead.
+
+ USE_STD non-zero means we have been passed the descriptor used by
+ the parent.
+
+ ACCESS is the Windows access mode for opening the null device.
+
+ Returns the Win32 handle to be passed to CreateProcess. */
+static HANDLE
+prepare_child_handle (int fd, int use_std, DWORD access)
+{
+ HANDLE htem, hret;
+ DWORD err = 0;
+
+ /* Start with the descriptor, if specified by the caller and valid,
+ otherwise open the null device. */
+ if (fd < 0)
+ htem = INVALID_HANDLE_VALUE;
+ else
+ htem = (HANDLE)_get_osfhandle (fd);
+
+ /* Duplicate the handle and make it inheritable. */
+ if (DuplicateHandle (GetCurrentProcess (),
+ htem,
+ GetCurrentProcess (),
+ &hret,
+ 0,
+ TRUE,
+ DUPLICATE_SAME_ACCESS) == FALSE)
+ {
+ /* If the original standard handle was invalid (happens, e.g.,
+ in GUI programs), open the null device instead. */
+ if ((err = GetLastError ()) == ERROR_INVALID_HANDLE
+ && use_std)
+ {
+ htem = CreateFile ("NUL", access,
+ FILE_SHARE_READ | FILE_SHARE_WRITE, NULL,
+ OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, NULL);
+ if (htem != INVALID_HANDLE_VALUE
+ && DuplicateHandle (GetCurrentProcess (),
+ htem,
+ GetCurrentProcess (),
+ &hret,
+ 0,
+ TRUE,
+ DUPLICATE_SAME_ACCESS) == FALSE)
+ {
+ err = GetLastError ();
+ CloseHandle (htem);
+ hret = INVALID_HANDLE_VALUE;
+ }
+ }
+ }
+
+ if (hret == INVALID_HANDLE_VALUE)
+ {
+ switch (err)
+ {
+ case ERROR_NO_MORE_FILES:
+ errno = EMFILE;
+ break;
+ case ERROR_INVALID_HANDLE:
+ default:
+ errno = EBADF;
+ break;
+ }
+ }
+
+ return hret;
+}
+
+/* A comparison function for sorting the environment. */
+static int
+compenv (const void *a1, const void *a2)
+{
+ return stricmp (*((char**)a1), *((char**)a2));
+}
+
+/* Convert the program's 'environ' array to a block of environment
+ variables suitable to be passed to CreateProcess. This is needed
+ to ensure the child process inherits the up-to-date environment of
+ the parent, including any variables inserted by the parent. */
+static void
+prepare_envblk (char **envp, char **envblk)
+{
+ char **tmp;
+ int size_needed;
+ int envcnt;
+ char *ptr;
+
+ for (envcnt = 0; envp[envcnt]; envcnt++)
+ ;
+
+ tmp = scm_calloc ((envcnt + 1) * sizeof (*tmp));
+
+ for (envcnt = size_needed = 0; envp[envcnt]; envcnt++)
+ {
+ tmp[envcnt] = envp[envcnt];
+ size_needed += strlen (envp[envcnt]) + 1;
+ }
+ size_needed++;
+
+ /* Windows likes its environment variables sorted. */
+ qsort ((void *) tmp, (size_t) envcnt, sizeof (char *), compenv);
+
+ /* CreateProcess needs the environment block as a linear array,
+ where each variable is terminated by a null character, and the
+ last one is terminated by 2 null characters. */
+ ptr = *envblk = scm_calloc (size_needed);
+
+ for (envcnt = 0; tmp[envcnt]; envcnt++)
+ {
+ strcpy (ptr, tmp[envcnt]);
+ ptr += strlen (tmp[envcnt]) + 1;
+ }
+
+ free (tmp);
+}
+
+/* Find an executable PROGRAM on PATH, return result in malloc'ed
+ storage. If PROGRAM is /bin/sh, and no sh.exe was found on PATH,
+ fall back on the Windows shell and set BIN_SH_REPLACED to non-zero. */
+static char *
+lookup_cmd (const char *program, int *bin_sh_replaced)
+{
+ static const char *extensions[] = {
+ ".exe", ".cmd", ".bat", "", ".com", NULL
+ };
+ int bin_sh_requested = 0;
+ const char *path;
+ char abs_name[MAX_PATH];
+ DWORD abs_namelen;
+ int i;
+
+ /* If they ask for the Unix system shell, try to find it on PATH. */
+ if (c_strcasecmp (program, "/bin/sh") == 0)
+ {
+ bin_sh_requested = 1;
+ program = "sh.exe";
+ }
+
+ /* If PROGRAM includes leading directories, the caller already did
+ our job. */
+ if (strchr (program, '/') != NULL
+ || strchr (program, '\\') != NULL)
+ return scm_strdup (program);
+
+ /* Note: It is OK for getenv below to return NULL -- in that case,
+ SearchPath will search in the directories whose list is specified
+ by the system Registry. */
+ path = getenv ("PATH");
+ for (i = 0; extensions[i]; i++)
+ {
+ abs_namelen = SearchPath (path, program, extensions[i],
+ MAX_PATH, abs_name, NULL);
+ if (0 < abs_namelen && abs_namelen <= MAX_PATH) /* found! */
+ break;
+ }
+
+ /* If they asked for /bin/sh and we didn't find it, fall back on the
+ default Windows shell. */
+ if (abs_namelen <= 0 && bin_sh_requested)
+ {
+ const char *shell = getenv ("ComSpec");
+
+ if (!shell)
+ shell = "C:\\Windows\\system32\\cmd.exe";
+
+ *bin_sh_replaced = 1;
+ strcpy (abs_name, shell);
+ abs_namelen = strlen (abs_name);
+ }
+
+ /* If not found, return the original PROGRAM name. */
+ if (abs_namelen <= 0 || abs_namelen > MAX_PATH)
+ return scm_strdup (program);
+
+ return scm_strndup (abs_name, abs_namelen);
+}
+
+/* Concatenate command-line arguments in argv[] into a single
+ command-line string, while quoting arguments as needed. The result
+ is malloc'ed. */
+static char *
+prepare_cmdline (const char *cmd, const char * const *argv, int bin_sh_replaced)
+{
+ /* These characters should include anything that is special to _any_
+ program, including both Windows and Unixy shells, and the
+ widlcard expansion in startup code of a typical Windows app. */
+ const char need_quotes[] = " \t#;\"\'*?[]&|<>(){}$`^";
+ size_t cmdlen = 1; /* for terminating null */
+ char *cmdline = scm_malloc (cmdlen);
+ char *dst = cmdline;
+ int cmd_exe_quoting = 0;
+ int i;
+ const char *p;
+
+ /* Are we constructing a command line for cmd.exe? */
+ if (bin_sh_replaced)
+ cmd_exe_quoting = 1;
+ else
+ {
+ for (p = cmd + strlen (cmd);
+ p > cmd && p[-1] != '/' && p[-1] != '\\' && p[-1] != ':';
+ p--)
+ ;
+ if (c_strcasecmp (p, "cmd.exe") == 0
+ || c_strcasecmp (p, "cmd") == 0)
+ cmd_exe_quoting = 1;
+ }
+
+ /* Initialize the command line to empty. */
+ *dst = '\0';
+
+ /* Append arguments, if any, from argv[]. */
+ for (i = 0; argv[i]; i++)
+ {
+ const char *src = argv[i];
+ size_t len;
+ int quote_this = 0, n_backslashes = 0;
+ int j;
+
+ /* Append the blank separator. We don't do that for argv[0]
+ because that is the command name (will end up in child's
+ argv[0]), and is only recognized as such if there're no
+ blanks before it. */
+ if (i > 0)
+ *dst++ = ' ';
+ len = dst - cmdline;
+
+ /* How much space is required for this argument? */
+ cmdlen += strlen (argv[i]) + 1; /* 1 for a blank separator */
+ /* cmd.exe needs a different style of quoting: all the arguments
+ beyond the /c switch are enclosed in an extra pair of quotes,
+ and not otherwise quoted/escaped. */
+ if (cmd_exe_quoting)
+ {
+ if (i == 2)
+ cmdlen += 2;
+ }
+ else if (strpbrk (argv[i], need_quotes))
+ {
+ quote_this = 1;
+ cmdlen += 2;
+ for ( ; *src; src++)
+ {
+ /* An embedded quote needs to be escaped by a backslash.
+ Any backslashes immediately preceding that quote need
+ each one to be escaped by another backslash. */
+ if (*src == '\"')
+ cmdlen += n_backslashes + 1;
+ if (*src == '\\')
+ n_backslashes++;
+ else
+ n_backslashes = 0;
+ }
+ /* If the closing quote we will add is preceded by
+ backslashes, those backslashes need to be escaped. */
+ cmdlen += n_backslashes;
+ }
+
+ /* Enlarge the command-line string as needed. */
+ cmdline = scm_realloc (cmdline, cmdlen);
+ dst = cmdline + len;
+
+ if (i == 0
+ && c_strcasecmp (argv[0], "/bin/sh") == 0
+ && bin_sh_replaced)
+ {
+ strcpy (dst, "cmd.exe");
+ dst += sizeof ("cmd.exe") - 1;
+ continue;
+ }
+ if (i == 1 && bin_sh_replaced && strcmp (argv[1], "-c") == 0)
+ {
+ *dst++ = '/';
+ *dst++ = 'c';
+ *dst = '\0';
+ continue;
+ }
+
+ /* Add this argument, possibly quoted, to the command line. */
+ if (quote_this || (i == 2 && cmd_exe_quoting))
+ *dst++ = '\"';
+ for (src = argv[i]; *src; src++)
+ {
+ if (quote_this)
+ {
+ if (*src == '\"')
+ for (j = n_backslashes + 1; j > 0; j--)
+ *dst++ = '\\';
+ if (*src == '\\')
+ n_backslashes++;
+ else
+ n_backslashes = 0;
+ }
+ *dst++ = *src;
+ }
+ if (quote_this)
+ {
+ for (j = n_backslashes; j > 0; j--)
+ *dst++ = '\\';
+ *dst++ = '\"';
+ }
+ *dst = '\0';
+ }
+
+ if (cmd_exe_quoting && i > 2)
+ {
+ /* One extra slot was already reserved when we enlarged cmdlen
+ by 2 in the "if (cmd_exe_quoting)" clause above. So we can
+ safely append a closing quote. */
+ *dst++ = '\"';
+ *dst = '\0';
+ }
+
+ return cmdline;
+}
+
+/* Start a child process running the program in EXEC_FILE with its
+ standard input and output optionally redirected to a pipe. ARGV is
+ the array of command-line arguments to pass to the child. P2C and
+ C2P are 2 pipes for communicating with the child, and ERRFD is the
+ standard error file descriptor to be inherited by the child.
+ READING and WRITING, if non-zero, mean that the corresponding pipe
+ will be used.
+
+ Return the PID of the child process, or -1 if couldn't start a
+ process. */
+static intptr_t
+start_child (const char *exec_file, char **argv,
+ int reading, int c2p[2], int writing, int p2c[2], int errfd)
+{
+ HANDLE hin = INVALID_HANDLE_VALUE, hout = INVALID_HANDLE_VALUE;
+ HANDLE herr = INVALID_HANDLE_VALUE;
+ STARTUPINFO si;
+ char *env_block = NULL;
+ char *cmdline = NULL;
+ PROCESS_INFORMATION pi;
+ char *progfile, *p;
+ int errno_save;
+ intptr_t pid;
+ int bin_sh_replaced = 0;
+
+ /* Prepare standard handles to be passed to the child process. */
+ hin = prepare_child_handle (p2c[0], !writing, GENERIC_READ);
+ if (hin == INVALID_HANDLE_VALUE)
+ return -1;
+ hout = prepare_child_handle (c2p[1], !reading, GENERIC_WRITE);
+ if (hout == INVALID_HANDLE_VALUE)
+ return -1;
+ herr = prepare_child_handle (errfd, 1, GENERIC_WRITE);
+ if (herr == INVALID_HANDLE_VALUE)
+ return -1;
+
+ /* Make sure the parent side of both pipes is not inherited. This
+ is required because gnulib's 'pipe' creates pipes whose both ends
+ are inheritable, which is traditional on Posix (where pipe
+ descriptors are implicitly duplicated by 'fork'), but wrong on
+ Windows (where pipe handles need to be explicitly
+ duplicated). */
+ if (writing)
+ SetHandleInformation ((HANDLE)_get_osfhandle (p2c[1]),
+ HANDLE_FLAG_INHERIT, 0);
+ if (reading)
+ {
+ SetHandleInformation ((HANDLE)_get_osfhandle (c2p[0]),
+ HANDLE_FLAG_INHERIT, 0);
+ /* Gnulib's 'pipe' opens the pipe in binary mode, but we don't
+ want to read text-mode input of subprocesses in binary more,
+ because then we will get the ^M (a.k.a. "CR") characters we
+ don't expect. */
+ _setmode (c2p[0], _O_TEXT);
+ }
+
+ /* Set up the startup info for the child, using the parent's as the
+ starting point, and specify in it the redirected handles. */
+ GetStartupInfo (&si);
+ si.dwFlags = STARTF_USESTDHANDLES;
+ si.lpReserved = 0;
+ si.cbReserved2 = 0;
+ si.lpReserved2 = 0;
+ si.hStdInput = hin;
+ si.hStdOutput = hout;
+ si.hStdError = herr;
+
+ /* Create the environment block for the child. This is needed
+ because the environment we have in 'environ' is not in the format
+ expected by CreateProcess. */
+ prepare_envblk (environ, &env_block);
+
+ /* CreateProcess doesn't search PATH, so we must do that for it. */
+ progfile = lookup_cmd (exec_file, &bin_sh_replaced);
+
+ /* CreateProcess doesn't like forward slashes in the application
+ file name. */
+ for (p = progfile; *p; p++)
+ if (*p == '/')
+ *p = '\\';
+
+ /* Construct the command line. */
+ cmdline = prepare_cmdline (exec_file, (const char * const *)argv,
+ bin_sh_replaced);
+
+ /* All set and ready to fly. Launch the child process. */
+ if (!CreateProcess (progfile, cmdline, NULL, NULL, TRUE, 0, env_block, NULL,
+ &si, &pi))
+ {
+ pid = -1;
+
+ /* Since we use Win32 APIs directly, we need to translate their
+ errors to errno values by hand. */
+ switch (GetLastError ())
+ {
+ case ERROR_FILE_NOT_FOUND:
+ case ERROR_PATH_NOT_FOUND:
+ case ERROR_INVALID_DRIVE:
+ case ERROR_BAD_PATHNAME:
+ errno = ENOENT;
+ break;
+ case ERROR_ACCESS_DENIED:
+ errno = EACCES;
+ break;
+ case ERROR_BAD_ENVIRONMENT:
+ errno = E2BIG;
+ break;
+ case ERROR_BROKEN_PIPE:
+ errno = EPIPE;
+ break;
+ case ERROR_INVALID_HANDLE:
+ errno = EBADF;
+ break;
+ case ERROR_MAX_THRDS_REACHED:
+ errno = EAGAIN;
+ break;
+ case ERROR_BAD_EXE_FORMAT:
+ case ERROR_BAD_FORMAT:
+ default:
+ errno = ENOEXEC;
+ break;
+ }
+ }
+ else
+ pid = (intptr_t)pi.hProcess;
+
+ errno_save = errno;
+
+ /* Free resources. */
+ free (progfile);
+ free (cmdline);
+ free (env_block);
+ CloseHandle (hin);
+ CloseHandle (hout);
+ CloseHandle (herr);
+ CloseHandle (pi.hThread);
+
+ /* Posix requires to call the shell if execvp fails to invoke EXEC_FILE. */
+ if (errno_save == ENOEXEC || errno_save == ENOENT)
+ {
+ const char *shell = getenv ("ComSpec");
+
+ if (!shell)
+ shell = "cmd.exe";
+
+ if (c_strcasecmp (exec_file, shell) != 0)
+ {
+ argv[0] = (char *)exec_file;
+ return start_child (shell, argv, reading, c2p, writing, p2c, errfd);
+ }
+ }
+
+ errno = errno_save;
+ return pid;
+}
+
+\f
+/* Emulation of waitpid which only supports WNOHANG, since _cwait doesn't. */
+int
+waitpid (intptr_t pid, int *status, int options)
+{
+ if ((options & WNOHANG) != 0)
+ {
+ DWORD st;
+
+ if (!GetExitCodeProcess ((HANDLE)pid, &st))
+ {
+ errno = ECHILD;
+ return -1;
+ }
+ if (st == STILL_ACTIVE)
+ return 0;
+ if (status)
+ *status = st;
+ return (int)pid;
+ }
+
+ return (int)_cwait (status, pid, WAIT_CHILD);
+}
+
+\f
+/* Translate abnormal exit status of Windows programs into the signal
+ that terminated the program. This is required to support scm_kill
+ and WTERMSIG. */
+
+struct signal_and_status {
+ int sig;
+ DWORD status;
+};
+
+static const struct signal_and_status sigtbl[] = {
+ {SIGSEGV, 0xC0000005}, /* access to invalid address */
+ {SIGSEGV, 0xC0000008}, /* invalid handle */
+ {SIGILL, 0xC000001D}, /* illegal instruction */
+ {SIGILL, 0xC0000025}, /* non-continuable instruction */
+ {SIGSEGV, 0xC000008C}, /* array bounds exceeded */
+ {SIGFPE, 0xC000008D}, /* float denormal */
+ {SIGFPE, 0xC000008E}, /* float divide by zero */
+ {SIGFPE, 0xC000008F}, /* float inexact */
+ {SIGFPE, 0xC0000090}, /* float invalid operation */
+ {SIGFPE, 0xC0000091}, /* float overflow */
+ {SIGFPE, 0xC0000092}, /* float stack check */
+ {SIGFPE, 0xC0000093}, /* float underflow */
+ {SIGFPE, 0xC0000094}, /* integer divide by zero */
+ {SIGFPE, 0xC0000095}, /* integer overflow */
+ {SIGILL, 0xC0000096}, /* privileged instruction */
+ {SIGSEGV, 0xC00000FD}, /* stack overflow */
+ {SIGTERM, 0xC000013A}, /* Ctrl-C exit */
+ {SIGINT, 0xC000013A}
+};
+
+static int
+w32_signal_to_status (int sig)
+{
+ int i;
+
+ for (i = 0; i < sizeof (sigtbl) / sizeof (sigtbl[0]); i++)
+ if (sig == sigtbl[i].sig)
+ return sigtbl[i].status;
+
+ return (int)0xC000013A;
+}
+
+static int
+w32_status_to_termsig (DWORD status)
+{
+ int i;
+
+ for (i = 0; i < sizeof (sigtbl) / sizeof (sigtbl[0]); i++)
+ if (status == sigtbl[i].status)
+ return sigtbl[i].sig;
+
+ return SIGTERM;
+}
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: open-process and related functions for MinGW Guile
2014-08-09 14:13 ` Eli Zaretskii
@ 2014-08-12 18:08 ` Mark H Weaver
2014-08-12 19:44 ` Eli Zaretskii
2014-08-12 19:52 ` Eli Zaretskii
0 siblings, 2 replies; 10+ messages in thread
From: Mark H Weaver @ 2014-08-12 18:08 UTC (permalink / raw)
To: Eli Zaretskii; +Cc: ludo, guile-devel
Hi Eli,
Thanks very much for working on this, and for your patience.
Eli Zaretskii <eliz@gnu.org> writes:
> Here's the updated patch:
>
> --- libguile/posix.c.~1~ 2014-02-28 22:01:27.000000000 +0200
> +++ libguile/posix.c 2014-08-08 17:27:50.339267200 +0300
> @@ -84,6 +84,42 @@
> #if HAVE_SYS_WAIT_H
> # include <sys/wait.h>
> #endif
> +#ifdef __MINGW32__
> +
> +#include <c-strcase.h>
Please add a space after the '#' here.
> +
> +# define WEXITSTATUS(stat_val) ((stat_val) & 255)
> +/* MS-Windows programs that crash due to a fatal exception exit with
> + an exit code whose 2 MSB bits are set. */
> +# define WIFEXITED(stat_val) (((stat_val) & 0xC0000000) == 0)
> +# define WIFSIGNALED(stat_val) (((stat_val) & 0xC0000000) == 0xC0000000)
> +# define WTERMSIG(stat_val) w32_status_to_termsig (stat_val)
> +/* The funny conditional avoids a compiler warning in status:stop_sig. */
> +# define WIFSTOPPED(stat_val) ((stat_val) == (stat_val) ? 0 : 0)
What is the warning? Would ((stat_val), 0) also fix it? If so, I think
that would be preferable.
> +# define WSTOPSIG(stat_var) (0)
> +# include <process.h>
> +# define HAVE_WAITPID 1
> + static int w32_status_to_termsig (DWORD);
> + static int w32_signal_to_status (int);
> +# define getuid() (500) /* Local Administrator */
> +# define getgid() (513) /* None */
> +# define setuid(u) (0)
> +# define setgid(g) (0)
As I've said before, I'm not okay with making 'setuid', 'seteuid',
'setgid', or 'setegid' into no-ops. They should at least raise an error
when called on Windows, and maybe they should be undefined.
> +# define WIN32_LEAN_AND_MEAN
> +# include <windows.h>
> +# define WNOHANG 1
> + int waitpid (intptr_t, int *, int);
> +
> + typedef DWORD_PTR cpu_set_t;
> +
> +#define CPU_ZERO(s) memset(s,0,sizeof(*s))
> +#define CPU_ISSET(b,s) ((*s) & (1U << (b))) != 0
> +#define CPU_SET(b,s) (*s) |= (1U << (b))
There should be parentheses around all occurrences of 's' above.
[...]
> --- /dev/null 1970-01-01 02:00:00 +0200
> +++ libguile/w32-proc.c 2014-06-29 11:26:08 +0300
> @@ -0,0 +1,563 @@
> +/* Run a child process with redirected standard handles, without
> + redirecting standard handles of the parent. This is required in
> + multithreaded programs, where redirecting a standard handle affects
> + all threads. */
Please add a copyright notice and header on this file, like the other
*.c files in libguile.
> +
> +#include <stdlib.h>
> +#include <string.h>
> +
> +/* Prepare a possibly redirected file handle to be passed to a child
> + process. The handle is for the file/device open on file descriptor
> + FD; if FD is invalid, use the null device instead.
> +
> + USE_STD non-zero means we have been passed the descriptor used by
> + the parent.
> +
> + ACCESS is the Windows access mode for opening the null device.
> +
> + Returns the Win32 handle to be passed to CreateProcess. */
> +static HANDLE
> +prepare_child_handle (int fd, int use_std, DWORD access)
> +{
> + HANDLE htem, hret;
> + DWORD err = 0;
> +
> + /* Start with the descriptor, if specified by the caller and valid,
> + otherwise open the null device. */
> + if (fd < 0)
> + htem = INVALID_HANDLE_VALUE;
> + else
> + htem = (HANDLE)_get_osfhandle (fd);
> +
> + /* Duplicate the handle and make it inheritable. */
> + if (DuplicateHandle (GetCurrentProcess (),
> + htem,
> + GetCurrentProcess (),
> + &hret,
> + 0,
> + TRUE,
> + DUPLICATE_SAME_ACCESS) == FALSE)
> + {
> + /* If the original standard handle was invalid (happens, e.g.,
> + in GUI programs), open the null device instead. */
> + if ((err = GetLastError ()) == ERROR_INVALID_HANDLE
> + && use_std)
> + {
> + htem = CreateFile ("NUL", access,
> + FILE_SHARE_READ | FILE_SHARE_WRITE, NULL,
> + OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, NULL);
> + if (htem != INVALID_HANDLE_VALUE
> + && DuplicateHandle (GetCurrentProcess (),
> + htem,
> + GetCurrentProcess (),
> + &hret,
> + 0,
> + TRUE,
> + DUPLICATE_SAME_ACCESS) == FALSE)
> + {
> + err = GetLastError ();
> + CloseHandle (htem);
> + hret = INVALID_HANDLE_VALUE;
> + }
> + }
> + }
> +
> + if (hret == INVALID_HANDLE_VALUE)
> + {
> + switch (err)
> + {
> + case ERROR_NO_MORE_FILES:
> + errno = EMFILE;
> + break;
> + case ERROR_INVALID_HANDLE:
> + default:
> + errno = EBADF;
> + break;
> + }
> + }
> +
> + return hret;
> +}
> +
> +/* A comparison function for sorting the environment. */
> +static int
> +compenv (const void *a1, const void *a2)
> +{
> + return stricmp (*((char**)a1), *((char**)a2));
> +}
> +
> +/* Convert the program's 'environ' array to a block of environment
> + variables suitable to be passed to CreateProcess. This is needed
> + to ensure the child process inherits the up-to-date environment of
> + the parent, including any variables inserted by the parent. */
> +static void
> +prepare_envblk (char **envp, char **envblk)
> +{
> + char **tmp;
> + int size_needed;
> + int envcnt;
> + char *ptr;
> +
> + for (envcnt = 0; envp[envcnt]; envcnt++)
> + ;
> +
> + tmp = scm_calloc ((envcnt + 1) * sizeof (*tmp));
> +
> + for (envcnt = size_needed = 0; envp[envcnt]; envcnt++)
> + {
> + tmp[envcnt] = envp[envcnt];
> + size_needed += strlen (envp[envcnt]) + 1;
> + }
> + size_needed++;
> +
> + /* Windows likes its environment variables sorted. */
> + qsort ((void *) tmp, (size_t) envcnt, sizeof (char *), compenv);
> +
> + /* CreateProcess needs the environment block as a linear array,
> + where each variable is terminated by a null character, and the
> + last one is terminated by 2 null characters. */
> + ptr = *envblk = scm_calloc (size_needed);
> +
> + for (envcnt = 0; tmp[envcnt]; envcnt++)
> + {
> + strcpy (ptr, tmp[envcnt]);
> + ptr += strlen (tmp[envcnt]) + 1;
> + }
> +
> + free (tmp);
> +}
> +
> +/* Find an executable PROGRAM on PATH, return result in malloc'ed
> + storage. If PROGRAM is /bin/sh, and no sh.exe was found on PATH,
> + fall back on the Windows shell and set BIN_SH_REPLACED to non-zero. */
> +static char *
> +lookup_cmd (const char *program, int *bin_sh_replaced)
> +{
> + static const char *extensions[] = {
> + ".exe", ".cmd", ".bat", "", ".com", NULL
> + };
> + int bin_sh_requested = 0;
> + const char *path;
> + char abs_name[MAX_PATH];
> + DWORD abs_namelen;
> + int i;
> +
> + /* If they ask for the Unix system shell, try to find it on PATH. */
> + if (c_strcasecmp (program, "/bin/sh") == 0)
> + {
> + bin_sh_requested = 1;
> + program = "sh.exe";
> + }
Hmm. I'm not so comfortable with such cleverness at this low level.
I'd prefer to have it higher in the stack, perhaps in the Scheme code in
(ice-9 popen) or maybe even in the application code.
> +
> + /* If PROGRAM includes leading directories, the caller already did
> + our job. */
> + if (strchr (program, '/') != NULL
> + || strchr (program, '\\') != NULL)
> + return scm_strdup (program);
> +
> + /* Note: It is OK for getenv below to return NULL -- in that case,
> + SearchPath will search in the directories whose list is specified
> + by the system Registry. */
> + path = getenv ("PATH");
> + for (i = 0; extensions[i]; i++)
> + {
> + abs_namelen = SearchPath (path, program, extensions[i],
> + MAX_PATH, abs_name, NULL);
> + if (0 < abs_namelen && abs_namelen <= MAX_PATH) /* found! */
> + break;
> + }
This search is not done correctly. If I ask to run program "foo", and
"foo.cmd" comes early in the path and "foo.exe" comes late in the path,
it should be "foo.cmd" that gets run, not "foo.exe".
> +
> + /* If they asked for /bin/sh and we didn't find it, fall back on the
> + default Windows shell. */
> + if (abs_namelen <= 0 && bin_sh_requested)
> + {
> + const char *shell = getenv ("ComSpec");
> +
> + if (!shell)
> + shell = "C:\\Windows\\system32\\cmd.exe";
> +
> + *bin_sh_replaced = 1;
> + strcpy (abs_name, shell);
> + abs_namelen = strlen (abs_name);
> + }
Again, I'd prefer to put this at a higher level in the stack. If the
user specifically asks to run "/bin/sh", we should not silently
substitute a different incompatible shell.
> +
> + /* If not found, return the original PROGRAM name. */
> + if (abs_namelen <= 0 || abs_namelen > MAX_PATH)
> + return scm_strdup (program);
> +
> + return scm_strndup (abs_name, abs_namelen);
> +}
> +
> +/* Concatenate command-line arguments in argv[] into a single
> + command-line string, while quoting arguments as needed. The result
> + is malloc'ed. */
> +static char *
> +prepare_cmdline (const char *cmd, const char * const *argv, int bin_sh_replaced)
> +{
> + /* These characters should include anything that is special to _any_
> + program, including both Windows and Unixy shells, and the
> + widlcard expansion in startup code of a typical Windows app. */
> + const char need_quotes[] = " \t#;\"\'*?[]&|<>(){}$`^";
Hmm. This seems unlikely to be correct in the general case. If the
program being run is not a command shell, then I wouldn't expect it to
process backslash escapes at all.
Is there any way to launch a standard C program on Windows where the
individual 'argv' arguments are passed as separate strings, without
having to combine them together with quoting and escapes?
If a standard C program is compiled using Mingw, precisely how is this
combined string converted into that program's 'argv' argument? How can
we ensure that those strings are passed reliably and losslessly, without
relying on heuristics that only work some of the time?
I do not expect that arbitrary byte sequences can be passed losslessly,
but we should do our best to ensure that arbitrary character strings are
passed losslessly.
> + size_t cmdlen = 1; /* for terminating null */
> + char *cmdline = scm_malloc (cmdlen);
> + char *dst = cmdline;
> + int cmd_exe_quoting = 0;
> + int i;
> + const char *p;
> +
> + /* Are we constructing a command line for cmd.exe? */
> + if (bin_sh_replaced)
> + cmd_exe_quoting = 1;
> + else
> + {
> + for (p = cmd + strlen (cmd);
> + p > cmd && p[-1] != '/' && p[-1] != '\\' && p[-1] != ':';
> + p--)
> + ;
> + if (c_strcasecmp (p, "cmd.exe") == 0
> + || c_strcasecmp (p, "cmd") == 0)
> + cmd_exe_quoting = 1;
> + }
If there's really no way to avoid quoting, then probably these
heuristics should be moved into 'lookup_cmd', and 'lookup_cmd' should
return the 'cmd_exe_quoting' flag instead of the 'bin_sh_replaced' flag,
so that it will do the right thing if the higher levels ask for
'cmd.exe' directly.
> +
> + /* Initialize the command line to empty. */
> + *dst = '\0';
> +
> + /* Append arguments, if any, from argv[]. */
> + for (i = 0; argv[i]; i++)
> + {
> + const char *src = argv[i];
> + size_t len;
> + int quote_this = 0, n_backslashes = 0;
> + int j;
> +
> + /* Append the blank separator. We don't do that for argv[0]
> + because that is the command name (will end up in child's
> + argv[0]), and is only recognized as such if there're no
> + blanks before it. */
> + if (i > 0)
> + *dst++ = ' ';
> + len = dst - cmdline;
> +
> + /* How much space is required for this argument? */
> + cmdlen += strlen (argv[i]) + 1; /* 1 for a blank separator */
> + /* cmd.exe needs a different style of quoting: all the arguments
> + beyond the /c switch are enclosed in an extra pair of quotes,
> + and not otherwise quoted/escaped. */
Again, I think this should be handled at a higher level in the stack.
It would be good to have primitives at the C level that don't include
such unreliable heuristics.
> + if (cmd_exe_quoting)
> + {
> + if (i == 2)
> + cmdlen += 2;
> + }
> + else if (strpbrk (argv[i], need_quotes))
> + {
> + quote_this = 1;
> + cmdlen += 2;
> + for ( ; *src; src++)
> + {
> + /* An embedded quote needs to be escaped by a backslash.
> + Any backslashes immediately preceding that quote need
> + each one to be escaped by another backslash. */
> + if (*src == '\"')
> + cmdlen += n_backslashes + 1;
> + if (*src == '\\')
> + n_backslashes++;
> + else
> + n_backslashes = 0;
> + }
> + /* If the closing quote we will add is preceded by
> + backslashes, those backslashes need to be escaped. */
> + cmdlen += n_backslashes;
> + }
> +
> + /* Enlarge the command-line string as needed. */
> + cmdline = scm_realloc (cmdline, cmdlen);
> + dst = cmdline + len;
> +
> + if (i == 0
> + && c_strcasecmp (argv[0], "/bin/sh") == 0
> + && bin_sh_replaced)
> + {
> + strcpy (dst, "cmd.exe");
> + dst += sizeof ("cmd.exe") - 1;
> + continue;
> + }
> + if (i == 1 && bin_sh_replaced && strcmp (argv[1], "-c") == 0)
> + {
> + *dst++ = '/';
> + *dst++ = 'c';
> + *dst = '\0';
> + continue;
> + }
> +
> + /* Add this argument, possibly quoted, to the command line. */
> + if (quote_this || (i == 2 && cmd_exe_quoting))
> + *dst++ = '\"';
> + for (src = argv[i]; *src; src++)
> + {
> + if (quote_this)
> + {
> + if (*src == '\"')
> + for (j = n_backslashes + 1; j > 0; j--)
> + *dst++ = '\\';
> + if (*src == '\\')
> + n_backslashes++;
> + else
> + n_backslashes = 0;
> + }
> + *dst++ = *src;
> + }
> + if (quote_this)
> + {
> + for (j = n_backslashes; j > 0; j--)
> + *dst++ = '\\';
> + *dst++ = '\"';
> + }
> + *dst = '\0';
> + }
> +
> + if (cmd_exe_quoting && i > 2)
> + {
> + /* One extra slot was already reserved when we enlarged cmdlen
> + by 2 in the "if (cmd_exe_quoting)" clause above. So we can
> + safely append a closing quote. */
> + *dst++ = '\"';
> + *dst = '\0';
> + }
> +
> + return cmdline;
> +}
> +
> +/* Start a child process running the program in EXEC_FILE with its
> + standard input and output optionally redirected to a pipe. ARGV is
> + the array of command-line arguments to pass to the child. P2C and
> + C2P are 2 pipes for communicating with the child, and ERRFD is the
> + standard error file descriptor to be inherited by the child.
> + READING and WRITING, if non-zero, mean that the corresponding pipe
> + will be used.
> +
> + Return the PID of the child process, or -1 if couldn't start a
> + process. */
> +static intptr_t
> +start_child (const char *exec_file, char **argv,
> + int reading, int c2p[2], int writing, int p2c[2], int errfd)
> +{
> + HANDLE hin = INVALID_HANDLE_VALUE, hout = INVALID_HANDLE_VALUE;
> + HANDLE herr = INVALID_HANDLE_VALUE;
> + STARTUPINFO si;
> + char *env_block = NULL;
> + char *cmdline = NULL;
> + PROCESS_INFORMATION pi;
> + char *progfile, *p;
> + int errno_save;
> + intptr_t pid;
> + int bin_sh_replaced = 0;
> +
> + /* Prepare standard handles to be passed to the child process. */
> + hin = prepare_child_handle (p2c[0], !writing, GENERIC_READ);
> + if (hin == INVALID_HANDLE_VALUE)
> + return -1;
> + hout = prepare_child_handle (c2p[1], !reading, GENERIC_WRITE);
> + if (hout == INVALID_HANDLE_VALUE)
> + return -1;
> + herr = prepare_child_handle (errfd, 1, GENERIC_WRITE);
> + if (herr == INVALID_HANDLE_VALUE)
> + return -1;
> +
> + /* Make sure the parent side of both pipes is not inherited. This
> + is required because gnulib's 'pipe' creates pipes whose both ends
> + are inheritable, which is traditional on Posix (where pipe
> + descriptors are implicitly duplicated by 'fork'), but wrong on
> + Windows (where pipe handles need to be explicitly
> + duplicated). */
> + if (writing)
> + SetHandleInformation ((HANDLE)_get_osfhandle (p2c[1]),
> + HANDLE_FLAG_INHERIT, 0);
> + if (reading)
> + {
> + SetHandleInformation ((HANDLE)_get_osfhandle (c2p[0]),
> + HANDLE_FLAG_INHERIT, 0);
> + /* Gnulib's 'pipe' opens the pipe in binary mode, but we don't
> + want to read text-mode input of subprocesses in binary more,
> + because then we will get the ^M (a.k.a. "CR") characters we
> + don't expect. */
> + _setmode (c2p[0], _O_TEXT);
> + }
We should do the newline conversion elsewhere in Guile's I/O stack, so
that it is possible to do binary I/O with subprocesses.
> +
> + /* Set up the startup info for the child, using the parent's as the
> + starting point, and specify in it the redirected handles. */
> + GetStartupInfo (&si);
> + si.dwFlags = STARTF_USESTDHANDLES;
> + si.lpReserved = 0;
> + si.cbReserved2 = 0;
> + si.lpReserved2 = 0;
> + si.hStdInput = hin;
> + si.hStdOutput = hout;
> + si.hStdError = herr;
> +
> + /* Create the environment block for the child. This is needed
> + because the environment we have in 'environ' is not in the format
> + expected by CreateProcess. */
> + prepare_envblk (environ, &env_block);
> +
> + /* CreateProcess doesn't search PATH, so we must do that for it. */
> + progfile = lookup_cmd (exec_file, &bin_sh_replaced);
> +
> + /* CreateProcess doesn't like forward slashes in the application
> + file name. */
> + for (p = progfile; *p; p++)
> + if (*p == '/')
> + *p = '\\';
> +
> + /* Construct the command line. */
> + cmdline = prepare_cmdline (exec_file, (const char * const *)argv,
> + bin_sh_replaced);
> +
> + /* All set and ready to fly. Launch the child process. */
> + if (!CreateProcess (progfile, cmdline, NULL, NULL, TRUE, 0, env_block, NULL,
> + &si, &pi))
> + {
> + pid = -1;
> +
> + /* Since we use Win32 APIs directly, we need to translate their
> + errors to errno values by hand. */
> + switch (GetLastError ())
> + {
> + case ERROR_FILE_NOT_FOUND:
> + case ERROR_PATH_NOT_FOUND:
> + case ERROR_INVALID_DRIVE:
> + case ERROR_BAD_PATHNAME:
> + errno = ENOENT;
> + break;
> + case ERROR_ACCESS_DENIED:
> + errno = EACCES;
> + break;
> + case ERROR_BAD_ENVIRONMENT:
> + errno = E2BIG;
> + break;
> + case ERROR_BROKEN_PIPE:
> + errno = EPIPE;
> + break;
> + case ERROR_INVALID_HANDLE:
> + errno = EBADF;
> + break;
> + case ERROR_MAX_THRDS_REACHED:
> + errno = EAGAIN;
> + break;
> + case ERROR_BAD_EXE_FORMAT:
> + case ERROR_BAD_FORMAT:
> + default:
> + errno = ENOEXEC;
> + break;
> + }
> + }
> + else
> + pid = (intptr_t)pi.hProcess;
> +
> + errno_save = errno;
> +
> + /* Free resources. */
> + free (progfile);
> + free (cmdline);
> + free (env_block);
> + CloseHandle (hin);
> + CloseHandle (hout);
> + CloseHandle (herr);
> + CloseHandle (pi.hThread);
> +
> + /* Posix requires to call the shell if execvp fails to invoke EXEC_FILE. */
> + if (errno_save == ENOEXEC || errno_save == ENOENT)
> + {
> + const char *shell = getenv ("ComSpec");
> +
> + if (!shell)
> + shell = "cmd.exe";
> +
> + if (c_strcasecmp (exec_file, shell) != 0)
> + {
> + argv[0] = (char *)exec_file;
> + return start_child (shell, argv, reading, c2p, writing, p2c, errfd);
> + }
> + }
I think this fallback shouldn't be done. We don't do it in the POSIX
version of scm_open_process, and we shouldn't do it here either.
What exactly does POSIX require? Can you provide a reference?
Anyway, thanks again for working on this.
Mark
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: open-process and related functions for MinGW Guile
2014-08-12 18:08 ` Mark H Weaver
@ 2014-08-12 19:44 ` Eli Zaretskii
2014-08-13 15:05 ` Eli Zaretskii
2014-08-15 7:07 ` Eli Zaretskii
2014-08-12 19:52 ` Eli Zaretskii
1 sibling, 2 replies; 10+ messages in thread
From: Eli Zaretskii @ 2014-08-12 19:44 UTC (permalink / raw)
To: Mark H Weaver; +Cc: ludo, guile-devel
> From: Mark H Weaver <mhw@netris.org>
> Cc: ludo@gnu.org, guile-devel@gnu.org
> Date: Tue, 12 Aug 2014 14:08:48 -0400
>
> > +/* The funny conditional avoids a compiler warning in status:stop_sig. */
> > +# define WIFSTOPPED(stat_val) ((stat_val) == (stat_val) ? 0 : 0)
>
> What is the warning?
That stat_val is unused (because the value is always zero).
> Would ((stat_val), 0) also fix it?
I'm not sure, but I will use it if it will.
> > +# define getuid() (500) /* Local Administrator */
> > +# define getgid() (513) /* None */
> > +# define setuid(u) (0)
> > +# define setgid(g) (0)
>
> As I've said before, I'm not okay with making 'setuid', 'seteuid',
> 'setgid', or 'setegid' into no-ops. They should at least raise an error
> when called on Windows, and maybe they should be undefined.
I'll respond to this in a separate thread.
> > + /* If they ask for the Unix system shell, try to find it on PATH. */
> > + if (c_strcasecmp (program, "/bin/sh") == 0)
> > + {
> > + bin_sh_requested = 1;
> > + program = "sh.exe";
> > + }
>
> Hmm. I'm not so comfortable with such cleverness at this low level.
You mean, that we look for 'sh' not in '/bin', but along PATH? What
else is reasonable? there's no standard '/bin' directory on Windows
systems, certainly not on every drive. Without something like that,
we will never be able to support portably shell commands that require
a Unixy shell, because on Unix you _must_ use "/bin/sh" verbatim.
This is very important for running the test suite, since there are
quite a few commands there that explicitly invoke /bin/sh and require
Bourne shell functionality.
> I'd prefer to have it higher in the stack, perhaps in the Scheme code in
> (ice-9 popen) or maybe even in the application code.
I don't think this is practical: too many places to change. And you
will have to keep an eye on it from now to eternity. It's a losing
battle.
> > + for (i = 0; extensions[i]; i++)
> > + {
> > + abs_namelen = SearchPath (path, program, extensions[i],
> > + MAX_PATH, abs_name, NULL);
> > + if (0 < abs_namelen && abs_namelen <= MAX_PATH) /* found! */
> > + break;
> > + }
>
> This search is not done correctly. If I ask to run program "foo", and
> "foo.cmd" comes early in the path and "foo.exe" comes late in the path,
> it should be "foo.cmd" that gets run, not "foo.exe".
No, the search order is exactly the one used by the Windows shell. A
.exe program is invoked in preference to a .cmd batch file. The only
deviation is that I've put .com last, whereas it is actually the
first. But I believe a .com file nowadays is much more likely to be a
VMS command file than a Windows executable.
> > + /* If they asked for /bin/sh and we didn't find it, fall back on the
> > + default Windows shell. */
> > + if (abs_namelen <= 0 && bin_sh_requested)
> > + {
> > + const char *shell = getenv ("ComSpec");
> > +
> > + if (!shell)
> > + shell = "C:\\Windows\\system32\\cmd.exe";
> > +
> > + *bin_sh_replaced = 1;
> > + strcpy (abs_name, shell);
> > + abs_namelen = strlen (abs_name);
> > + }
>
> Again, I'd prefer to put this at a higher level in the stack.
And I again think that this is impractical to place that burden on
higher levels.
> If the user specifically asks to run "/bin/sh", we should not
> silently substitute a different incompatible shell.
If the command is incompatible with cmd, it will fail anyway. But if
it is a simple command, it is likely to work with cmd as well, so this
is better than always failing. People who work on Unix will use
/bin/sh without much thought (what else can they do when they need a
shell?), so if we see it, it doesn't necessarily mean they must have a
Bourne shell.
> > +/* Concatenate command-line arguments in argv[] into a single
> > + command-line string, while quoting arguments as needed. The result
> > + is malloc'ed. */
> > +static char *
> > +prepare_cmdline (const char *cmd, const char * const *argv, int bin_sh_replaced)
> > +{
> > + /* These characters should include anything that is special to _any_
> > + program, including both Windows and Unixy shells, and the
> > + widlcard expansion in startup code of a typical Windows app. */
> > + const char need_quotes[] = " \t#;\"\'*?[]&|<>(){}$`^";
>
> Hmm. This seems unlikely to be correct in the general case. If the
> program being run is not a command shell, then I wouldn't expect it to
> process backslash escapes at all.
That's what happens on Unix. But not on Windows, where the quotes are
handled by the startup code that processes the command-line string
into the argv[] array. With a few exceptions (cmd.exe being one of
them), every Windows program processes quotes and wildcards the same.
> Is there any way to launch a standard C program on Windows where the
> individual 'argv' arguments are passed as separate strings, without
> having to combine them together with quoting and escapes?
No. The low-level API to launch a program accepts a single string as
the command line. It is processed into argv[] by the C library
startup code.
> If a standard C program is compiled using Mingw, precisely how is this
> combined string converted into that program's 'argv' argument?
By the startup code that is part of the C runtime.
> How can we ensure that those strings are passed reliably and
> losslessly, without relying on heuristics that only work some of the
> time?
This is not heuristics, this is the official documented way to quote
and escape quotes on the command line. Working by those rules will
not cause any losses.
> > + /* Are we constructing a command line for cmd.exe? */
> > + if (bin_sh_replaced)
> > + cmd_exe_quoting = 1;
> > + else
> > + {
> > + for (p = cmd + strlen (cmd);
> > + p > cmd && p[-1] != '/' && p[-1] != '\\' && p[-1] != ':';
> > + p--)
> > + ;
> > + if (c_strcasecmp (p, "cmd.exe") == 0
> > + || c_strcasecmp (p, "cmd") == 0)
> > + cmd_exe_quoting = 1;
> > + }
>
> If there's really no way to avoid quoting, then probably these
> heuristics should be moved into 'lookup_cmd', and 'lookup_cmd' should
> return the 'cmd_exe_quoting' flag instead of the 'bin_sh_replaced' flag,
> so that it will do the right thing if the higher levels ask for
> 'cmd.exe' directly.
The design is that lookup_cmd searches for the command, and the
quoting is done elsewhere. I don't understand why it matters how
these tasks are subdivided between functions, but if it does, I submit
that the way I subdivided them is more modular, as each function does
a single well-defined job.
> > + /* cmd.exe needs a different style of quoting: all the arguments
> > + beyond the /c switch are enclosed in an extra pair of quotes,
> > + and not otherwise quoted/escaped. */
>
> Again, I think this should be handled at a higher level in the stack.
> It would be good to have primitives at the C level that don't include
> such unreliable heuristics.
Sorry, I don't understand: what heuristics? The way cmd.exe handles
quotes is well documented, and the code implements those rules.
There's no heuristics here, anywhere.
> > + /* Gnulib's 'pipe' opens the pipe in binary mode, but we don't
> > + want to read text-mode input of subprocesses in binary more,
> > + because then we will get the ^M (a.k.a. "CR") characters we
> > + don't expect. */
> > + _setmode (c2p[0], _O_TEXT);
> > + }
>
> We should do the newline conversion elsewhere in Guile's I/O stack, so
> that it is possible to do binary I/O with subprocesses.
That's fine by me, but the default should still be text I/O. Once
there is a way to propagate the text/binary mode to this level, a
condition can be added not to do the above. IOW, this is something
for future changes; for now, the vast majority of pipes need text-mode
I/O, so leaving this at binary will break much more code than using
text I/O.
> > + /* Posix requires to call the shell if execvp fails to invoke EXEC_FILE. */
> > + if (errno_save == ENOEXEC || errno_save == ENOENT)
> > + {
> > + const char *shell = getenv ("ComSpec");
> > +
> > + if (!shell)
> > + shell = "cmd.exe";
> > +
> > + if (c_strcasecmp (exec_file, shell) != 0)
> > + {
> > + argv[0] = (char *)exec_file;
> > + return start_child (shell, argv, reading, c2p, writing, p2c, errfd);
> > + }
> > + }
>
> I think this fallback shouldn't be done. We don't do it in the POSIX
> version of scm_open_process, and we shouldn't do it here either.
AFAIK, on Posix platforms this is done by the library or the kernel
implementation of execvp.
> What exactly does POSIX require? Can you provide a reference?
See http://pubs.opengroup.org/onlinepubs/009695399/functions/exec.html.
Quote:
In the cases where the other members of the exec family of functions
would fail and set errno to [ENOEXEC], the execlp() and execvp()
functions shall execute a command interpreter and the environment of
the executed command shall be as if the process invoked the sh
utility using execl() as follows:
execl(<shell path>, arg0, file, arg1, ..., (char *)0);
where <shell path> is an unspecified pathname for the sh utility,
file is the process image file, and for execvp(), where arg0, arg1,
and so on correspond to the values passed to execvp() in argv[0],
argv[1], and so on.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: open-process and related functions for MinGW Guile
2014-08-12 18:08 ` Mark H Weaver
2014-08-12 19:44 ` Eli Zaretskii
@ 2014-08-12 19:52 ` Eli Zaretskii
1 sibling, 0 replies; 10+ messages in thread
From: Eli Zaretskii @ 2014-08-12 19:52 UTC (permalink / raw)
To: Mark H Weaver; +Cc: ludo, guile-devel
> From: Mark H Weaver <mhw@netris.org>
> Cc: ludo@gnu.org, guile-devel@gnu.org
> Date: Tue, 12 Aug 2014 14:08:48 -0400
>
> > +# define getuid() (500) /* Local Administrator */
> > +# define getgid() (513) /* None */
> > +# define setuid(u) (0)
> > +# define setgid(g) (0)
>
> As I've said before, I'm not okay with making 'setuid', 'seteuid',
> 'setgid', or 'setegid' into no-ops. They should at least raise an error
> when called on Windows, and maybe they should be undefined.
I explained at length why failing in these functions or having them
undefined (i.e. signaling a fatal error in any code that uses them)
would be a mistake. Please see
http://lists.gnu.org/archive/html/guile-devel/2014-02/msg00072.html
All the cases I know of where such operations were not made no-ops on
Windows are terrible annoyances: they produce useless warning and
error messages that no one can possibly do anything about. It would
be a pity to have such annoyances in Guile.
Maybe you could describe a practical situation where you consider
having these undefined or failing would be TRT? I cannot think of
any.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: open-process and related functions for MinGW Guile
2014-08-12 19:44 ` Eli Zaretskii
@ 2014-08-13 15:05 ` Eli Zaretskii
2014-08-15 7:07 ` Eli Zaretskii
1 sibling, 0 replies; 10+ messages in thread
From: Eli Zaretskii @ 2014-08-13 15:05 UTC (permalink / raw)
To: mhw; +Cc: ludo, guile-devel
> Date: Tue, 12 Aug 2014 22:44:03 +0300
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: ludo@gnu.org, guile-devel@gnu.org
>
> > From: Mark H Weaver <mhw@netris.org>
> > Cc: ludo@gnu.org, guile-devel@gnu.org
> > Date: Tue, 12 Aug 2014 14:08:48 -0400
> >
> > > +/* The funny conditional avoids a compiler warning in status:stop_sig. */
> > > +# define WIFSTOPPED(stat_val) ((stat_val) == (stat_val) ? 0 : 0)
> >
> > What is the warning?
>
> That stat_val is unused (because the value is always zero).
The warning is:
posix.c: In function 'scm_status_stop_sig':
posix.c:832:7: warning: variable 'lstatus' set but not used [-Wunused-but-set-variable]
> > Would ((stat_val), 0) also fix it?
>
> I'm not sure, but I will use it if it will.
This produces a different warning:
posix.c: In function 'scm_status_stop_sig':
posix.c:835:7: warning: left-hand operand of comma expression has no effect [-Wunused-value]
> > > + for (i = 0; extensions[i]; i++)
> > > + {
> > > + abs_namelen = SearchPath (path, program, extensions[i],
> > > + MAX_PATH, abs_name, NULL);
> > > + if (0 < abs_namelen && abs_namelen <= MAX_PATH) /* found! */
> > > + break;
> > > + }
> >
> > This search is not done correctly. If I ask to run program "foo", and
> > "foo.cmd" comes early in the path and "foo.exe" comes late in the path,
> > it should be "foo.cmd" that gets run, not "foo.exe".
>
> No, the search order is exactly the one used by the Windows shell.
It looks like I've misread your comment: you meant that the search for
extensions should be inner to the search of directories on PATH. I
agree; I show below the relevant portion of the patch after fixing
that.
> > How can we ensure that those strings are passed reliably and
> > losslessly, without relying on heuristics that only work some of the
> > time?
>
> This is not heuristics, this is the official documented way to quote
> and escape quotes on the command line. Working by those rules will
> not cause any losses.
The quoting rules are publicly documented here:
http://msdn.microsoft.com/en-us/library/17w5ykft(v=vs.120).aspx
> > > + /* cmd.exe needs a different style of quoting: all the arguments
> > > + beyond the /c switch are enclosed in an extra pair of quotes,
> > > + and not otherwise quoted/escaped. */
> >
> > Again, I think this should be handled at a higher level in the stack.
> > It would be good to have primitives at the C level that don't include
> > such unreliable heuristics.
>
> Sorry, I don't understand: what heuristics? The way cmd.exe handles
> quotes is well documented, and the code implements those rules.
> There's no heuristics here, anywhere.
The way cmd.exe handles quoted command lines is documented here:
http://www.microsoft.com/resources/documentation/windows/xp/all/proddocs/en-us/cmd.mspx?mfr=true
(under "Remarks", see the "Processing quotation marks" bullet).
> > > + /* Gnulib's 'pipe' opens the pipe in binary mode, but we don't
> > > + want to read text-mode input of subprocesses in binary more,
> > > + because then we will get the ^M (a.k.a. "CR") characters we
> > > + don't expect. */
> > > + _setmode (c2p[0], _O_TEXT);
> > > + }
> >
> > We should do the newline conversion elsewhere in Guile's I/O stack, so
> > that it is possible to do binary I/O with subprocesses.
>
> That's fine by me, but the default should still be text I/O. Once
> there is a way to propagate the text/binary mode to this level, a
> condition can be added not to do the above. IOW, this is something
> for future changes; for now, the vast majority of pipes need text-mode
> I/O, so leaving this at binary will break much more code than using
> text I/O.
According to this discussion on guile-user a year ago:
http://lists.gnu.org/archive/html/guile-user/2013-06/msg00070.html
http://lists.gnu.org/archive/html/guile-user/2013-06/msg00080.html
newline conversion for ports is not yet implemented. Did something
change since then? If not, then for now there's only one I/O mode
possible here, and that's got to be text mode.
Here's the search on PATH part of lookup_cmd after fixing the search
order:
+ path = getenv ("PATH");
+ if (!path) /* shouldn't happen, really */
+ path = ".";
+ dir = sep = path = strdup (path);
+ for ( ; sep && *sep; dir = sep + 1)
+ {
+ int i;
+
+ sep = strpbrk (dir, ";");
+ if (sep == dir) /* two or more ;'s in a row */
+ continue;
+ if (sep)
+ *sep = '\0';
+ for (i = 0; extensions[i]; i++)
+ {
+ abs_namelen = SearchPath (dir, program, extensions[i],
+ MAX_PATH, abs_name, NULL);
+ if (0 < abs_namelen && abs_namelen <= MAX_PATH) /* found! */
+ break;
+ }
+ if (extensions[i]) /* found! */
+ break;
+ if (sep)
+ *sep = ';';
+ }
+
+ free (path);
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: open-process and related functions for MinGW Guile
2014-08-12 19:44 ` Eli Zaretskii
2014-08-13 15:05 ` Eli Zaretskii
@ 2014-08-15 7:07 ` Eli Zaretskii
2014-09-15 17:20 ` Eli Zaretskii
1 sibling, 1 reply; 10+ messages in thread
From: Eli Zaretskii @ 2014-08-15 7:07 UTC (permalink / raw)
To: mhw; +Cc: ludo, guile-devel
> Date: Tue, 12 Aug 2014 22:44:03 +0300
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: ludo@gnu.org, guile-devel@gnu.org
>
> > > + /* If they ask for the Unix system shell, try to find it on PATH. */
> > > + if (c_strcasecmp (program, "/bin/sh") == 0)
> > > + {
> > > + bin_sh_requested = 1;
> > > + program = "sh.exe";
> > > + }
> >
> > Hmm. I'm not so comfortable with such cleverness at this low level.
>
> You mean, that we look for 'sh' not in '/bin', but along PATH? What
> else is reasonable? there's no standard '/bin' directory on Windows
> systems, certainly not on every drive. Without something like that,
> we will never be able to support portably shell commands that require
> a Unixy shell, because on Unix you _must_ use "/bin/sh" verbatim.
>
> This is very important for running the test suite, since there are
> quite a few commands there that explicitly invoke /bin/sh and require
> Bourne shell functionality.
>
> > I'd prefer to have it higher in the stack, perhaps in the Scheme code in
> > (ice-9 popen) or maybe even in the application code.
>
> I don't think this is practical: too many places to change. And you
> will have to keep an eye on it from now to eternity. It's a losing
> battle.
> [...]
> > > + /* If they asked for /bin/sh and we didn't find it, fall back on the
> > > + default Windows shell. */
> > > + if (abs_namelen <= 0 && bin_sh_requested)
> > > + {
> > > + const char *shell = getenv ("ComSpec");
> > > +
> > > + if (!shell)
> > > + shell = "C:\\Windows\\system32\\cmd.exe";
> > > +
> > > + *bin_sh_replaced = 1;
> > > + strcpy (abs_name, shell);
> > > + abs_namelen = strlen (abs_name);
> > > + }
> >
> > Again, I'd prefer to put this at a higher level in the stack.
>
> And I again think that this is impractical to place that burden on
> higher levels.
>
> > If the user specifically asks to run "/bin/sh", we should not
> > silently substitute a different incompatible shell.
>
> If the command is incompatible with cmd, it will fail anyway. But if
> it is a simple command, it is likely to work with cmd as well, so this
> is better than always failing. People who work on Unix will use
> /bin/sh without much thought (what else can they do when they need a
> shell?), so if we see it, it doesn't necessarily mean they must have a
> Bourne shell.
If the above still doesn't convince you, I can think of the following
alternative (but will need help in implementing it):
. We define 2 new variables, %shell-file-name and
%shell-command-switch. (The names were stolen from Emacs, which
has these facilities.) %shell-file-name will default to "/bin/sh"
on Posix platforms, and to the absolute file name of cmd.exe on
Windows, determined at startup. %shell-command-switch will default
to "-c" and "/c" respectively.
. We publicly ban the use of literal "/bin/sh -c" in Guile sources
(and in the test suite, except where Bourne shell semantics is
required), and require the use of the above 2 variables instead.
. The test suite, when it runs on Windows, will look for sh.exe along
PATH, and, if found, will set %shell-file-name to its absolute file
name and %shell-command-switch to "-c".
Note that the 2nd item above will require thorough review of the
existing sources that invoke "/bin/sh" directly and indirectly, and
changing them so that they'd work with the Windows shell as well.
Some obvious gotcha's include quoting with ".." rather than '..'.
Some less obvious gotcha's include things like this one (from
autofrisk.scm):
(define (unglob pattern)
(let ((p (open-input-pipe (format #f "echo '(' ~A ')'" pattern))))
(map symbol->string (read p))))
This assumes that 'echo' globs its argument, but the cmd.exe built-in
by the same name does not.
This alternative gives you what you want (AFAIU), but it's not without
a price: it is not 100% reliable, and it will require some maintenance
burden, both initially and in the future (to make sure no hidden
dependencies on /bin/sh creep in). It also places an additional
burden on users and on Scheme programmers. TANSTAAFL.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: open-process and related functions for MinGW Guile
2014-08-15 7:07 ` Eli Zaretskii
@ 2014-09-15 17:20 ` Eli Zaretskii
0 siblings, 0 replies; 10+ messages in thread
From: Eli Zaretskii @ 2014-09-15 17:20 UTC (permalink / raw)
To: mhw; +Cc: ludo, guile-devel
Ping!
It's been a month since the last exchange, and more than a year since
I started working on this. Can we please pick up where this was left
off, and resolve whatever issues remain? Please?
> Date: Fri, 15 Aug 2014 10:07:50 +0300
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: ludo@gnu.org, guile-devel@gnu.org
>
> > Date: Tue, 12 Aug 2014 22:44:03 +0300
> > From: Eli Zaretskii <eliz@gnu.org>
> > Cc: ludo@gnu.org, guile-devel@gnu.org
> >
> > > > + /* If they ask for the Unix system shell, try to find it on PATH. */
> > > > + if (c_strcasecmp (program, "/bin/sh") == 0)
> > > > + {
> > > > + bin_sh_requested = 1;
> > > > + program = "sh.exe";
> > > > + }
> > >
> > > Hmm. I'm not so comfortable with such cleverness at this low level.
> >
> > You mean, that we look for 'sh' not in '/bin', but along PATH? What
> > else is reasonable? there's no standard '/bin' directory on Windows
> > systems, certainly not on every drive. Without something like that,
> > we will never be able to support portably shell commands that require
> > a Unixy shell, because on Unix you _must_ use "/bin/sh" verbatim.
> >
> > This is very important for running the test suite, since there are
> > quite a few commands there that explicitly invoke /bin/sh and require
> > Bourne shell functionality.
> >
> > > I'd prefer to have it higher in the stack, perhaps in the Scheme code in
> > > (ice-9 popen) or maybe even in the application code.
> >
> > I don't think this is practical: too many places to change. And you
> > will have to keep an eye on it from now to eternity. It's a losing
> > battle.
> > [...]
> > > > + /* If they asked for /bin/sh and we didn't find it, fall back on the
> > > > + default Windows shell. */
> > > > + if (abs_namelen <= 0 && bin_sh_requested)
> > > > + {
> > > > + const char *shell = getenv ("ComSpec");
> > > > +
> > > > + if (!shell)
> > > > + shell = "C:\\Windows\\system32\\cmd.exe";
> > > > +
> > > > + *bin_sh_replaced = 1;
> > > > + strcpy (abs_name, shell);
> > > > + abs_namelen = strlen (abs_name);
> > > > + }
> > >
> > > Again, I'd prefer to put this at a higher level in the stack.
> >
> > And I again think that this is impractical to place that burden on
> > higher levels.
> >
> > > If the user specifically asks to run "/bin/sh", we should not
> > > silently substitute a different incompatible shell.
> >
> > If the command is incompatible with cmd, it will fail anyway. But if
> > it is a simple command, it is likely to work with cmd as well, so this
> > is better than always failing. People who work on Unix will use
> > /bin/sh without much thought (what else can they do when they need a
> > shell?), so if we see it, it doesn't necessarily mean they must have a
> > Bourne shell.
>
> If the above still doesn't convince you, I can think of the following
> alternative (but will need help in implementing it):
>
> . We define 2 new variables, %shell-file-name and
> %shell-command-switch. (The names were stolen from Emacs, which
> has these facilities.) %shell-file-name will default to "/bin/sh"
> on Posix platforms, and to the absolute file name of cmd.exe on
> Windows, determined at startup. %shell-command-switch will default
> to "-c" and "/c" respectively.
>
> . We publicly ban the use of literal "/bin/sh -c" in Guile sources
> (and in the test suite, except where Bourne shell semantics is
> required), and require the use of the above 2 variables instead.
>
> . The test suite, when it runs on Windows, will look for sh.exe along
> PATH, and, if found, will set %shell-file-name to its absolute file
> name and %shell-command-switch to "-c".
>
> Note that the 2nd item above will require thorough review of the
> existing sources that invoke "/bin/sh" directly and indirectly, and
> changing them so that they'd work with the Windows shell as well.
> Some obvious gotcha's include quoting with ".." rather than '..'.
> Some less obvious gotcha's include things like this one (from
> autofrisk.scm):
>
> (define (unglob pattern)
> (let ((p (open-input-pipe (format #f "echo '(' ~A ')'" pattern))))
> (map symbol->string (read p))))
>
> This assumes that 'echo' globs its argument, but the cmd.exe built-in
> by the same name does not.
>
> This alternative gives you what you want (AFAIU), but it's not without
> a price: it is not 100% reliable, and it will require some maintenance
> burden, both initially and in the future (to make sure no hidden
> dependencies on /bin/sh creep in). It also places an additional
> burden on users and on Scheme programmers. TANSTAAFL.
>
>
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2014-09-15 17:20 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-06-29 15:43 open-process and related functions for MinGW Guile Eli Zaretskii
2014-06-29 20:21 ` Ludovic Courtès
2014-06-30 2:49 ` Eli Zaretskii
2014-08-09 14:13 ` Eli Zaretskii
2014-08-12 18:08 ` Mark H Weaver
2014-08-12 19:44 ` Eli Zaretskii
2014-08-13 15:05 ` Eli Zaretskii
2014-08-15 7:07 ` Eli Zaretskii
2014-09-15 17:20 ` Eli Zaretskii
2014-08-12 19:52 ` Eli Zaretskii
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).