Hi,

At Tue, 6 Jan 2009 22:31:49 +0900,
James M. Lawrence wrote in [ruby-core:21167]:
> Latest trunk passes the spec except for the following case: I
> expected system("echo", "1") to fail since echo only exists
> as a built-in shell command, while multi-arg system() should
> not invoke the shell.  However I see the lookup on the
> cmd.exe command list and the join on argv, so presumably the
> current behavior is intended.

Yes.

> Note there are other popular Windows shells such the
> PowerShell, so the general strategy of checking for built-in
> shell commands cannot work.

Is COMSPEC the PowerShell by the default on such systems?

> Also, is a backport to 1.8 planned?  This affects Rake.

Could you test the following patch?


Index: io.c =================================================================== --- io.c (revision 21607) +++ io.c (working copy) @@ -3189,5 +3189,5 @@ pipe_open(pstr, pname, mode) #ifdef _WIN32 retry: - pid = pipe_exec(pname, rb_io_mode_modenum(mode), &fpr, &fpw); + pid = rb_w32_pipe_exec(pname, rb_io_mode_modenum(mode), &fpr, &fpw); if (pid == -1) { /* exec failed */ if (errno == EAGAIN) { Index: process.c =================================================================== --- process.c (revision 21607) +++ process.c (working copy) @@ -1036,5 +1036,5 @@ rb_proc_exec(str) #ifdef _WIN32 before_exec(); - do_spawn(P_OVERLAY, (char *)str); + rb_w32_spawn(P_OVERLAY, (char *)str, NULL); after_exec(); #else @@ -1087,4 +1087,7 @@ rb_proc_exec(str) #if defined(__human68k__) || defined(__DJGPP__) || defined(_WIN32) +#if defined(_WIN32) +#define proc_spawn_v(argv, prog) rb_w32_aspawn(P_WAIT, prog, argv) +#elif defined(__human68k__) || defined(__DJGPP__) static int proc_spawn_v(argv, prog) @@ -1129,12 +1132,10 @@ proc_spawn_v(argv, prog) #endif before_exec(); -#if defined(_WIN32) - status = do_aspawn(P_WAIT, prog, argv); -#else status = spawnv(P_WAIT, prog, argv); -#endif + rb_last_status_set(status == -1 ? 127 : status, 0); after_exec(); return status; } +#endif static int @@ -1149,9 +1150,6 @@ proc_spawn_n(argc, argv, prog) args = ALLOCA_N(char*, argc + 1); for (i = 0; i < argc; i++) { - SafeStringValue(argv[i]); - args[i] = StringValueCStr(argv[i]); + args[i] = RSTRING_PTR(argv[i]); } - if (prog) - SafeStringValue(prog); args[i] = (char*) 0; if (args[0]) @@ -1160,16 +1158,15 @@ proc_spawn_n(argc, argv, prog) } -#if !defined(_WIN32) +#if defined(_WIN32) +#define proc_spawn(str) rb_w32_spawn(P_WAIT, str, 0) +#else static int -proc_spawn(sv) - VALUE sv; -{ +proc_spawn(str) char *str; +{ char *s, *t; char **argv, **a; int status; - SafeStringValue(sv); - str = s = StringValueCStr(sv); for (s = str; *s; s++) { if (*s != ' ' && !ISALPHA(*s) && strchr("*?{}[]<>()~&|\\$;'`\"\n",*s)) { @@ -1177,4 +1174,5 @@ proc_spawn(sv) before_exec(); status = shell?spawnl(P_WAIT,shell,"sh","-c",str,(char*)NULL):system(str); + last_status_set(status == -1 ? 127 : status, 0); after_exec(); return status; @@ -1287,5 +1285,4 @@ rb_f_exec(argc, argv) prog = RARRAY(tmp)->ptr[0]; argv[0] = RARRAY(tmp)->ptr[1]; - SafeStringValue(prog); } proc_prepare_args(&earg, argc, argv, prog); @@ -1471,100 +1468,66 @@ rb_f_system(argc, argv) VALUE *argv; { - int status; -#if defined(__EMX__) + int status, i; +#if defined(__EMX__) || defined(__VMS) VALUE cmd; +#else + volatile VALUE prog = 0; +# if !(defined(__human68k__) || defined(__DJGPP__) || defined(_WIN32)) + int pid; + struct rb_exec_arg earg; + RETSIGTYPE (*chfunc)(int); +# endif +#endif +#if !defined(__VMS) fflush(stdout); fflush(stderr); +#endif if (argc == 0) { rb_last_status = Qnil; rb_raise(rb_eArgError, "wrong number of arguments"); } - if (TYPE(argv[0]) == T_ARRAY) { if (RARRAY(argv[0])->len != 2) { rb_raise(rb_eArgError, "wrong first argument"); } +#if defined(__EMX__) || defined(__VMS) argv[0] = RARRAY(argv[0])->ptr[0]; +#else + prog = RARRAY(argv[0])->ptr[0]; + argv[0] = RARRAY(argv[0])->ptr[1]; +#endif + } + if (prog && !(argc > 0 && argv[0] == prog)) { + SafeStringValue(prog); + StringValueCStr(prog); + } + for (i = 0; i < argc; ++i) { + SafeStringValue(argv[i]); + StringValueCStr(argv[i]); } +#if defined(__EMX__) || defined(__VMS) cmd = rb_ary_join(rb_ary_new4(argc, argv), rb_str_new2(" ")); SafeStringValue(cmd); - status = do_spawn(RSTRING(cmd)->ptr); +# if defined(__EMX__) + status = do_spawn(StringValueCStr(cmd)); last_status_set(status, 0); +# else + status = system(StringValueCStr(cmd)); + last_status_set((status & 0xff) << 8, 0); +# endif #elif defined(__human68k__) || defined(__DJGPP__) || defined(_WIN32) - volatile VALUE prog = 0; - - fflush(stdout); - fflush(stderr); - if (argc == 0) { - rb_last_status = Qnil; - rb_raise(rb_eArgError, "wrong number of arguments"); - } - - if (TYPE(argv[0]) == T_ARRAY) { - if (RARRAY(argv[0])->len != 2) { - rb_raise(rb_eArgError, "wrong first argument"); - } - prog = RARRAY(argv[0])->ptr[0]; - argv[0] = RARRAY(argv[0])->ptr[1]; - } - if (argc == 1 && prog == 0) { -#if defined(_WIN32) - SafeStringValue(argv[0]); - status = do_spawn(P_WAIT, StringValueCStr(argv[0])); -#else - status = proc_spawn(argv[0]); -#endif + status = proc_spawn(RSTRING_PTR(argv[0])); } else { status = proc_spawn_n(argc, argv, prog); } -#if !defined(_WIN32) - last_status_set(status == -1 ? 127 : status, 0); -#else +# if defined(_WIN32) if (status == -1) last_status_set(0x7f << 8, 0); -#endif -#elif defined(__VMS) - VALUE cmd; - - if (argc == 0) { - rb_last_status = Qnil; - rb_raise(rb_eArgError, "wrong number of arguments"); - } - - if (TYPE(argv[0]) == T_ARRAY) { - if (RARRAY(argv[0])->len != 2) { - rb_raise(rb_eArgError, "wrong first argument"); - } - argv[0] = RARRAY(argv[0])->ptr[0]; - } - cmd = rb_ary_join(rb_ary_new4(argc, argv), rb_str_new2(" ")); - - SafeStringValue(cmd); - status = system(StringValueCStr(cmd)); - last_status_set((status & 0xff) << 8, 0); +# endif #else - volatile VALUE prog = 0; - int pid; - struct rb_exec_arg earg; - RETSIGTYPE (*chfunc)(int); - - fflush(stdout); - fflush(stderr); - if (argc == 0) { - rb_last_status = Qnil; - rb_raise(rb_eArgError, "wrong number of arguments"); - } - - if (TYPE(argv[0]) == T_ARRAY) { - if (RARRAY(argv[0])->len != 2) { - rb_raise(rb_eArgError, "wrong first argument"); - } - prog = RARRAY(argv[0])->ptr[0]; - argv[0] = RARRAY(argv[0])->ptr[1]; - } proc_prepare_args(&earg, argc, argv, prog); Index: win32/win32.c =================================================================== --- win32/win32.c (revision 21607) +++ win32/win32.c (working copy) @@ -28,7 +28,7 @@ #include <wincon.h> #include <shlobj.h> +#include <mbstring.h> #ifdef __MINGW32__ #include <mswsock.h> -#include <mbstring.h> #endif #include "win32.h" @@ -47,8 +47,4 @@ #undef setsockopt -#ifndef bool -#define bool int -#endif - #if defined __BORLANDC__ || defined _WIN32_WCE # define _filbuf _fgetc @@ -86,5 +82,5 @@ static struct ChildRecord *CreateChild(const char *, const char *, SECURITY_ATTRIBUTES *, HANDLE, HANDLE, HANDLE); -static bool has_redirection(const char *); +static int has_redirection(const char *); static void StartSockets (); static DWORD wait_events(HANDLE event, DWORD timeout); @@ -390,4 +386,15 @@ exit_handler(void) } +static inline char * +translate_char(char *p, int from, int to) +{ + while (*p) { + if ((unsigned char)*p == from) + *p = to; + p = CharNext(p); + } + return p; +} + #ifndef CSIDL_PROFILE #define CSIDL_PROFILE 40 @@ -435,9 +442,5 @@ init_env(void) } if (f) { - char *p = env; - while (*p) { - if (*p == '\\') *p = '/'; - p = CharNext(p); - } + char *p = translate_char(env, '\\', '/'); if (p - env == 2 && env[1] == ':') { *p++ = '/'; @@ -643,8 +646,10 @@ is_command_com(const char *interp) } +static int internal_cmd_match(const char *cmdname, int nt); + static int is_internal_cmd(const char *cmd, int nt) { - char cmdname[9], *b = cmdname, c, **nm; + char cmdname[9], *b = cmdname, c; do { @@ -666,4 +671,12 @@ is_internal_cmd(const char *cmd, int nt) } *b = 0; + return internal_cmd_match(cmdname, nt); +} + +static int +internal_cmd_match(const char *cmdname, int nt) +{ + char **nm; + nm = bsearch(cmdname, szInternalCmds, sizeof(szInternalCmds) / sizeof(*szInternalCmds), @@ -675,5 +688,4 @@ is_internal_cmd(const char *cmd, int nt) } - SOCKET rb_w32_get_osfhandle(int fh) @@ -683,5 +695,5 @@ rb_w32_get_osfhandle(int fh) rb_pid_t -pipe_exec(const char *cmd, int mode, FILE **fpr, FILE **fpw) +rb_w32_pipe_exec(const char *cmd, int mode, FILE **fpr, FILE **fpw) { struct ChildRecord* child; @@ -821,58 +833,10 @@ pipe_exec(const char *cmd, int mode, FIL extern VALUE rb_last_status; -int -do_spawn(int mode, const char *cmd) -{ - struct ChildRecord *child; - DWORD exitcode; - - switch (mode) { - case P_WAIT: - case P_NOWAIT: - case P_OVERLAY: - break; - default: - errno = EINVAL; - return -1; - } - - child = CreateChild(cmd, NULL, NULL, NULL, NULL, NULL); - if (!child) { - return -1; - } - - switch (mode) { - case P_WAIT: - rb_syswait(child->pid); - return NUM2INT(rb_last_status); - case P_NOWAIT: - return child->pid; - case P_OVERLAY: - WaitForSingleObject(child->hProcess, INFINITE); - GetExitCodeProcess(child->hProcess, &exitcode); - CloseChildHandle(child); - _exit(exitcode); - default: - return -1; /* not reached */ - } -} - -int -do_aspawn(int mode, const char *prog, char **argv) +static int +argv_size(char *const *argv, BOOL escape) { - char *cmd, *p, *q, *s, **t; + const char *p; + char *const *t; int len, n, bs, quote; - struct ChildRecord *child; - DWORD exitcode; - - switch (mode) { - case P_WAIT: - case P_NOWAIT: - case P_OVERLAY: - break; - default: - errno = EINVAL; - return -1; - } for (t = argv, len = 0; *t; t++) { @@ -882,4 +846,8 @@ do_aspawn(int mode, const char *prog, ch ++bs; break; + case '<': case '>': case '|': case '^': + bs = 0; + if (escape && !quote) n++; + break; case '"': n += bs + 1; bs = 0; @@ -895,7 +863,16 @@ do_aspawn(int mode, const char *prog, ch } len += p - *t + n + 1; - if (quote) len += 2; + if (p - *t == 0 || quote) len += 2; } - cmd = ALLOCA_N(char, len); + return len; +} + +static char * +join_argv(char *cmd, char *const *argv, BOOL escape) +{ + const char *p, *s; + char *q, *const *t; + int n, bs, quote; + for (t = argv, q = cmd; p = *t; t++) { quote = 0; @@ -914,4 +891,10 @@ do_aspawn(int mode, const char *prog, ch memset(q, '\\', ++bs); q += bs; bs = 0; break; + case '<': case '>': case '|': case '^': + if (escape && !quote) { + memcpy(q, s, n = p - s); q += n; s = p; + *q++ = '^'; + break; + } default: bs = 0; @@ -927,6 +910,35 @@ do_aspawn(int mode, const char *prog, ch if (q > cmd) --q; *q = '\0'; + return cmd; +} + +#ifdef HAVE_SYS_PARAM_H +# include <sys/param.h> +#else +# define MAXPATHLEN 512 +#endif + +#define STRNDUPA(ptr, src, len) \ + (((char *)memcpy(((ptr) = ALLOCA_N(char, (len) + 1)), (src), (len)))[len] = 0) + +static int +check_spawn_mode(int mode) +{ + switch (mode) { + case P_WAIT: + case P_NOWAIT: + case P_OVERLAY: + return 0; + default: + errno = EINVAL; + return -1; + } +} + +static rb_pid_t +child_result(struct ChildRecord *child, int mode) +{ + DWORD exitcode; - child = CreateChild(cmd, prog, NULL, NULL, NULL, NULL); if (!child) { return -1; @@ -958,7 +970,5 @@ CreateChild(const char *cmd, const char PROCESS_INFORMATION aProcessInformation; SECURITY_ATTRIBUTES sa; - const char *shell; struct ChildRecord *child; - char *p = NULL; if (!cmd && !prog) { @@ -1007,4 +1017,37 @@ CreateChild(const char *cmd, const char dwCreationFlags = (NORMAL_PRIORITY_CLASS); + RUBY_CRITICAL({ + fRet = CreateProcess(prog, (char *)cmd, psa, psa, + psa->bInheritHandle, dwCreationFlags, NULL, NULL, + &aStartupInfo, &aProcessInformation); + errno = map_errno(GetLastError()); + }); + + if (!fRet) { + child->pid = 0; /* release the slot */ + return NULL; + } + + CloseHandle(aProcessInformation.hThread); + + child->hProcess = aProcessInformation.hProcess; + child->pid = (rb_pid_t)aProcessInformation.dwProcessId; + + if (!IsWinNT()) { + /* On Win9x, make pid positive similarly to cygwin and perl */ + child->pid = -child->pid; + } + + return child; +} + +rb_pid_t +rb_w32_spawn(int mode, const char *cmd, const char *prog) +{ + char *p = NULL; + const char *shell = NULL; + + if (check_spawn_mode(mode)) return -1; + if (prog) { if (!(p = dln_find_exe(prog, NULL))) { @@ -1014,21 +1057,9 @@ CreateChild(const char *cmd, const char else { int redir = -1; - int len = 0; int nt; while (ISSPACE(*cmd)) cmd++; - for (prog = cmd; *prog; prog = CharNext(prog)) { - if (ISSPACE(*prog)) { - len = prog - cmd; - do ++prog; while (ISSPACE(*prog)); - if (!*prog--) break; - } - else { - len = 0; - } - } - if (!len) len = strlen(cmd); if ((shell = getenv("RUBYSHELL")) && (redir = has_redirection(cmd))) { - char *tmp = ALLOCA_N(char, strlen(shell) + len + sizeof(" -c ") + 2); - sprintf(tmp, "%s -c \"%.*s\"", shell, len, cmd); + char *tmp = ALLOCA_N(char, strlen(shell) + strlen(cmd) + sizeof(" -c ") + 2); + sprintf(tmp, "%s -c \"%s\"", shell, cmd); cmd = tmp; } @@ -1037,10 +1068,30 @@ CreateChild(const char *cmd, const char (redir < 0 ? has_redirection(cmd) : redir) || is_internal_cmd(cmd, nt))) { - char *tmp = ALLOCA_N(char, strlen(shell) + len + sizeof(" /c ") + char *tmp = ALLOCA_N(char, strlen(shell) + strlen(cmd) + sizeof(" /c ") + (nt ? 2 : 0)); - sprintf(tmp, nt ? "%s /c \"%.*s\"" : "%s /c %.*s", shell, len, cmd); + sprintf(tmp, nt ? "%s /c \"%s\"" : "%s /c %s", shell, cmd); cmd = tmp; } else { + int len = 0; + if (*cmd == '"') { + for (prog = cmd + 1; *prog && *prog != '"'; prog = CharNext(prog)); + len = prog - cmd - 1; + STRNDUPA(p, cmd + 1, len); + p = dln_find_exe(p, NULL); + if (p) goto command_found; + } + for (prog = cmd; *prog; prog = CharNext(prog)) { + if (ISSPACE(*prog)) { + len = prog - cmd; + do ++prog; while (ISSPACE(*prog)); + break; + } + else { + len = 0; + } + } + if (!len) len = strlen(cmd); + shell = NULL; prog = cmd; @@ -1050,18 +1101,14 @@ CreateChild(const char *cmd, const char break; } - if (strchr(".:*?\"/\\", *prog)) { + if (strchr(":*?\"/\\", *prog)) { if (cmd[len]) { - char *tmp = ALLOCA_N(char, len + 1); - memcpy(tmp, cmd, len); - tmp[len] = 0; - cmd = tmp; + STRNDUPA(p, cmd, len); } + p = dln_find_exe(p ? p : cmd, NULL); break; } if (ISSPACE(*prog) || strchr("<>|", *prog)) { len = prog - cmd; - p = ALLOCA_N(char, len + 1); - memcpy(p, cmd, len); - p[len] = 0; + STRNDUPA(p, cmd, len); p = dln_find_exe(p, NULL); break; @@ -1072,35 +1119,73 @@ CreateChild(const char *cmd, const char } if (p) { + command_found: shell = p; - while (*p) { - if ((unsigned char)*p == '/') - *p = '\\'; - p = CharNext(p); - } + translate_char(p, '/', '\\'); } - RUBY_CRITICAL({ - fRet = CreateProcess(shell, (char *)cmd, psa, psa, - psa->bInheritHandle, dwCreationFlags, NULL, NULL, - &aStartupInfo, &aProcessInformation); - errno = map_errno(GetLastError()); - }); - - if (!fRet) { - child->pid = 0; /* release the slot */ - return NULL; - } + return child_result(CreateChild(cmd, shell, NULL, NULL, NULL, NULL), mode); +} - CloseHandle(aProcessInformation.hThread); +rb_pid_t +rb_w32_aspawn(int mode, const char *prog, char *const *argv) +{ + int len, differ = 0, c_switch = 0; + BOOL ntcmd = FALSE, tmpnt; + const char *shell; + char *cmd; - child->hProcess = aProcessInformation.hProcess; - child->pid = (rb_pid_t)aProcessInformation.dwProcessId; + if (check_spawn_mode(mode)) return -1; - if (!IsWinNT()) { - /* On Win9x, make pid positive similarly to cygwin and perl */ - child->pid = -child->pid; + if (!prog) prog = argv[0]; + if ((shell = getenv("COMSPEC")) && + internal_cmd_match(prog, tmpnt = !is_command_com(shell))) { + ntcmd = tmpnt; + prog = shell; + c_switch = 1; + differ = 1; + } + else if ((cmd = dln_find_exe(prog, NULL))) { + if (cmd == prog) { + cmd = ALLOCA_N(char, strlen(cmd)); + strcpy(cmd, prog); + } + translate_char(cmd, '/', '\\'); + prog = cmd; + argv++; + differ = 1; + } + else if (strchr(prog, '/')) { + cmd = ALLOCA_N(char, strlen(cmd)); + strcpy(cmd, prog); + translate_char(cmd, '/', '\\'); + argv++; + differ = 1; + } + if (differ) { + char *progs[2]; + int cmdlen; + progs[0] = (char *)prog; + progs[1] = NULL; + len = argv_size(progs, ntcmd); + cmdlen = len - 1; + if (c_switch) len += 3; + if (argv[0]) len += argv_size(argv, ntcmd); + cmd = ALLOCA_N(char, len); + join_argv(cmd, progs, ntcmd); + if (c_switch) { + memcpy(cmd + cmdlen, " /c", 4); + cmdlen += 3; + } + if (argv[0]) { + cmd[cmdlen++] = ' '; + join_argv(cmd + cmdlen, argv, ntcmd); + } } - - return child; + else { + len = argv_size(argv, FALSE); + cmd = ALLOCA_N(char, len); + join_argv(cmd, argv, FALSE); + } + return child_result(CreateChild(cmd, prog, NULL, NULL, NULL, NULL), mode); } @@ -1139,10 +1224,4 @@ insert(const char *path, VALUE vinfo) } -#ifdef HAVE_SYS_PARAM_H -# include <sys/param.h> -#else -# define MAXPATHLEN 512 -#endif - static NtCmdLineElement ** @@ -1157,5 +1236,5 @@ cmdglob(NtCmdLineElement *patt, NtCmdLin if (!(buf = malloc(patt->len + 1))) return 0; - strncpy (buf, patt->str, patt->len); + memcpy(buf, patt->str, patt->len + 1); buf[patt->len] = '\0'; for (p = buf; *p; p = CharNext(p)) @@ -1178,5 +1257,5 @@ cmdglob(NtCmdLineElement *patt, NtCmdLin // -static bool +static int has_redirection(const char *cmd) { @@ -1208,4 +1287,10 @@ has_redirection(const char *cmd) break; + case '%': + if (*++ptr != '_' && !ISALPHA(*ptr)) break; + while (*++ptr == '_' || ISALNUM(*ptr)); + if (*ptr++ == '%') return TRUE; + break; + case '\\': ptr++; Index: win32/win32.h =================================================================== --- win32/win32.h (revision 21607) +++ win32/win32.h (working copy) @@ -139,5 +139,5 @@ extern DWORD rb_w32_osid(void); #define stat(path,st) rb_w32_stat(path,st) #undef execv -#define execv(path,argv) do_aspawn(P_OVERLAY,path,argv) +#define execv(path,argv) rb_w32_aspawn(P_OVERLAY,path,argv) #if !defined(__BORLANDC__) && !defined(_WIN32_WCE) #undef isatty @@ -161,5 +161,5 @@ struct timezone { extern void NtInitialize(int *, char ***); extern int rb_w32_cmdvector(const char *, char ***); -extern rb_pid_t pipe_exec(const char *, int, FILE **, FILE **); +extern rb_pid_t rb_w32_pipe_exec(const char *, int, FILE **, FILE **); extern int flock(int fd, int oper); extern int rb_w32_accept(int, struct sockaddr *, int *); @@ -207,6 +207,6 @@ extern int link(char *, char *); extern int gettimeofday(struct timeval *, struct timezone *); extern rb_pid_t waitpid (rb_pid_t, int *, int); -extern int do_spawn(int, const char *); -extern int do_aspawn(int, const char *, char **); +extern rb_pid_t rb_w32_spawn(int, const char *, const char *prog); +extern rb_pid_t rb_w32_aspawn(int, const char *, char *const *); extern int kill(int, int); extern int fcntl(int, int, ...);
-- Nobu Nakada