From: Jim Deville Date: 2009-01-19T06:21:17+09:00 Subject: [ruby-core:21421] Re: [Bug #907] Various system() and backticks problems on Windows COMSPEC is still cmd by default. Win7 doesn't change that even though Powershell is in the box. Installing Powershell doesn't change it either. JD -----Original Message----- From: Nobuyoshi Nakada Sent: January 18, 2009 12:46 PM To: ruby-core@ruby-lang.org Subject: [ruby-core:21417] Re: [Bug #907] Various system() and backticks problems on Windows 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 #include +#include #ifdef __MINGW32__ #include -#include #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 +#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 -#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