From 7ba9e9653d55a7aea263aafebcf35486fb1b7d57 Mon Sep 17 00:00:00 2001 From: Alex Dowad Date: Sun, 3 May 2020 17:54:06 +0200 Subject: [PATCH 1/4] Add PTY support to proc_open (again after 16 long years) Back in 2004, a feature was added to proc_open which allowed it to open a PTY, connecting specific FDs in the child process to the slave end of the PTY and returning the master end of the PTY (wrapped as a PHP stream) in the `$pipes` array. However, this feature was disabled just about a month later. Little information is available about why this was done, but from talking to the original implementer, it seems there were portability problems with some rare flavors of Unix. Re-enable this feature with a simplified implementation which uses openpty(). No attempt is made to support PTYs if the platform does not have openpty(). The configure script checks if linking with -lutil is necessary to use openpty(), but if anything else is required, like including some special header or linking with some other library, PTY support will be disabled. The original PTY support for proc_open automatically daemonized the child process (disassociating it from the TTY session and process group of the parent). However, I don't think this is a good idea. Just because a user opens a child process in a PTY, it doesn't mean they want it to continue running even when the parent process is killed. Of course, if the child process is some kind of server, it will likely daemonize itself; but we have no reason to preempt that decision. It turns out that since 2015, there has been one test case for PTY support in proc_open() in the test suite. This test was added in GitHub PR #1588 (https://github.com/php/php-src/pull/1588). That PR mentioned that the PHP binary in the Debian/Ubuntu repositories is patched to *enable* PTY support. Checking the Debian PHP repository (https://salsa.debian.org/php-team/php.git) shows that this is still true. Debian's patch does not modify the implementation from 2004 in any way; it just removes the #if 0 line which disables it. Naturally, the test case is skipped if PTY support is not enabled. This means that ever since it was added, every test run against the 'vanilla' PHP codebase has skipped it. Interestingly, the test case which was added in 2015 fails on my Linux Mint PC... both with this simplified implementation *and* when enabling the original implementation. Investigation reveals the reason: when the child process using the slave end of the PTY exits and its FDs are all closed, and all buffered data is read from the master end of the PTY, any further attempt to read from the master end fails with EIO. The test case seems to expect that reading from the master end will always return an empty string if no data is available. Likely this is because PHP's fread() was updated to report errors from the underlying system calls only recently. One way out of this dilemma: IF at least one FD referring to the slave end of the PTY is kept open *in the parent process*, the failure with EIO will not occur even after the child process exits. However, that would raise another issue: we would need a way to ensure the FD will be closed eventually in long-running programs. Another discovery made while testing this code is that fread() does not always return all the data written to the slave end of the PTY in a single call, even if the data was written with a single syscall and it is only a few bytes long. Specifically, when the child process in the test case writes "foo\n" to the PTY, the parent sometimes receives "foo" (3 bytes) and sometimes "foo\r\n" (5 bytes). (The "\r" is from the TTY line discipline converting "\n" to "\r\n".) A second call to fread() does return the remaining bytes, though sometimes all the data is read in the first call, and by the time the second call is made, the child process has already exited. It seems that liberal use of the @ operator is needed when using fread() on pipes. Thanks to Nikita Popov for suggesting that we should just use openpty() rather than grantpt(), unlockpt(), etc. --- configure.ac | 7 +- ext/standard/proc_open.c | 119 ++++++++++---------------- ext/standard/tests/file/bug69442.phpt | 34 +++++--- 3 files changed, 75 insertions(+), 85 deletions(-) diff --git a/configure.ac b/configure.ac index 1a17aa904da4..1143972bbf08 100644 --- a/configure.ac +++ b/configure.ac @@ -392,6 +392,7 @@ malloc.h \ monetary.h \ netdb.h \ poll.h \ +pty.h \ pwd.h \ resolv.h \ strings.h \ @@ -560,7 +561,6 @@ getgrnam_r \ getpwuid_r \ getwd \ glob \ -grantpt \ inet_ntoa \ inet_ntop \ inet_pton \ @@ -572,7 +572,6 @@ mmap \ nice \ nl_langinfo \ poll \ -ptsname \ putenv \ scandir \ setitimer \ @@ -588,7 +587,6 @@ strptime \ strtok_r \ symlink \ tzset \ -unlockpt \ unsetenv \ usleep \ utime \ @@ -707,6 +705,9 @@ if test "$PHP_VALGRIND" != "no"; then fi fi +dnl Check for openpty. It may require linking against libutil. +PHP_CHECK_FUNC(openpty, util) + dnl General settings. dnl ---------------------------------------------------------------------------- PHP_CONFIGURE_PART(General settings) diff --git a/ext/standard/proc_open.c b/ext/standard/proc_open.c index fb650e3a7450..186501acbd45 100644 --- a/ext/standard/proc_open.c +++ b/ext/standard/proc_open.c @@ -43,10 +43,13 @@ * */ #ifdef PHP_CAN_SUPPORT_PROC_OPEN -#if 0 && HAVE_PTSNAME && HAVE_GRANTPT && HAVE_UNLOCKPT && HAVE_SYS_IOCTL_H && HAVE_TERMIOS_H -# include -# include -# define PHP_CAN_DO_PTS 1 +#if HAVE_OPENPTY +# if HAVE_PTY_H +# include +# else +/* Mac OS X defines openpty() in */ +# include +# endif #endif #include "proc_open.h" @@ -592,6 +595,27 @@ static int set_proc_descriptor_to_blackhole(struct php_proc_open_descriptor_item return SUCCESS; } +static int set_proc_descriptor_to_pty(struct php_proc_open_descriptor_item *desc, int *master_fd, int *slave_fd) +{ +#if HAVE_OPENPTY + if (*master_fd == -1) { + if (openpty(master_fd, slave_fd, NULL, NULL, NULL)) { + php_error_docref(NULL, E_WARNING, "Could not open PTY (pseudoterminal) - %s", strerror(errno)); + return FAILURE; + } + } + + desc->is_pipe = 1; + desc->childend = dup(*slave_fd); + desc->parentend = dup(*master_fd); + desc->mode_flags = O_RDWR; + return SUCCESS; +#else + php_error_docref(NULL, E_WARNING, "PTY (pseudoterminal) not supported on this system"); + return FAILURE; +#endif +} + static int set_proc_descriptor_to_pipe(struct php_proc_open_descriptor_item *desc, zend_string *zmode) { php_file_descriptor_t newpipe[2]; @@ -708,9 +732,9 @@ static int redirect_proc_descriptor(struct php_proc_open_descriptor_item *desc, return dup_proc_descriptor(redirect_to, &desc->childend, nindex); } - int set_proc_descriptor_from_array( - zval *descitem, struct php_proc_open_descriptor_item *descriptors, int ndesc, int nindex) { + zval *descitem, struct php_proc_open_descriptor_item *descriptors, int ndesc, + int nindex, int *pty_master_fd, int *pty_slave_fd) { zend_string *ztype = get_string_parameter(descitem, 0, "handle qualifier"); if (!ztype) { return FAILURE; @@ -749,32 +773,7 @@ int set_proc_descriptor_from_array( } else if (zend_string_equals_literal(ztype, "null")) { retval = set_proc_descriptor_to_blackhole(&descriptors[ndesc]); } else if (zend_string_equals_literal(ztype, "pty")) { -#if PHP_CAN_DO_PTS - if (dev_ptmx == -1) { - /* open things up */ - dev_ptmx = open("/dev/ptmx", O_RDWR); - if (dev_ptmx == -1) { - php_error_docref(NULL, E_WARNING, "Failed to open /dev/ptmx, errno %d", errno); - goto finish; - } - grantpt(dev_ptmx); - unlockpt(dev_ptmx); - slave_pty = open(ptsname(dev_ptmx), O_RDWR); - - if (slave_pty == -1) { - php_error_docref(NULL, E_WARNING, "Failed to open slave pty, errno %d", errno); - goto finish; - } - } - descriptors[ndesc].is_pipe = 1; - descriptors[ndesc].childend = dup(slave_pty); - descriptors[ndesc].parentend = dup(dev_ptmx); - descriptors[ndesc].mode_flags = O_RDWR; - retval = SUCCESS; -#else - php_error_docref(NULL, E_WARNING, "PTY pseudo terminal not supported on this system"); - goto finish; -#endif + retval = set_proc_descriptor_to_pty(&descriptors[ndesc], pty_master_fd, pty_slave_fd); } else { php_error_docref(NULL, E_WARNING, "%s is not a valid descriptor spec/mode", ZSTR_VAL(ztype)); goto finish; @@ -867,14 +866,10 @@ PHP_FUNCTION(proc_open) #else char **argv = NULL; #endif + int pty_master_fd = -1, pty_slave_fd = -1; php_process_id_t child; struct php_process_handle *proc; -#if PHP_CAN_DO_PTS - php_file_descriptor_t dev_ptmx = -1; /* master */ - php_file_descriptor_t slave_pty = -1; -#endif - ZEND_PARSE_PARAMETERS_START(3, 6) Z_PARAM_ZVAL(command_zv) Z_PARAM_ARRAY(descriptorspec) @@ -957,7 +952,8 @@ PHP_FUNCTION(proc_open) goto exit_fail; } } else if (Z_TYPE_P(descitem) == IS_ARRAY) { - if (set_proc_descriptor_from_array(descitem, descriptors, ndesc, nindex) == FAILURE) { + if (set_proc_descriptor_from_array(descitem, descriptors, ndesc, nindex, + &pty_master_fd, &pty_slave_fd) == FAILURE) { goto exit_fail; } } else { @@ -1044,27 +1040,6 @@ PHP_FUNCTION(proc_open) if (child == 0) { /* this is the child process */ -#if PHP_CAN_DO_PTS - if (dev_ptmx >= 0) { - int my_pid = getpid(); - -#ifdef TIOCNOTTY - /* detach from original tty. Might only need this if isatty(0) is true */ - ioctl(0,TIOCNOTTY,NULL); -#else - setsid(); -#endif - /* become process group leader */ - setpgid(my_pid, my_pid); - tcsetpgrp(0, my_pid); - } - - if (dev_ptmx >= 0) { - close(dev_ptmx); - close(slave_pty); - } -#endif - if (close_parent_ends_of_pipes_in_child(descriptors, ndesc) == FAILURE) { /* We are already in child process and can't do anything to make * proc_open() return an error in the parent @@ -1121,13 +1096,6 @@ PHP_FUNCTION(proc_open) #endif proc->env = env; -#if PHP_CAN_DO_PTS - if (dev_ptmx >= 0) { - close(dev_ptmx); - close(slave_pty); - } -#endif - /* clean up all the child ends and then open streams on the parent * ends, where appropriate */ for (i = 0; i < ndesc; i++) { @@ -1191,7 +1159,14 @@ PHP_FUNCTION(proc_open) #else efree_argv(argv); #endif - +#if HAVE_OPENPTY + if (pty_master_fd != -1) { + close(pty_master_fd); + } + if (pty_slave_fd != -1) { + close(pty_slave_fd); + } +#endif efree(descriptors); ZVAL_RES(return_value, zend_register_resource(proc, le_proc_open)); return; @@ -1211,12 +1186,12 @@ PHP_FUNCTION(proc_open) #else efree_argv(argv); #endif -#if PHP_CAN_DO_PTS - if (dev_ptmx >= 0) { - close(dev_ptmx); +#if HAVE_OPENPTY + if (pty_master_fd != -1) { + close(pty_master_fd); } - if (slave_pty >= 0) { - close(slave_pty); + if (pty_slave_fd != -1) { + close(pty_slave_fd); } #endif RETURN_FALSE; diff --git a/ext/standard/tests/file/bug69442.phpt b/ext/standard/tests/file/bug69442.phpt index 60192d3c9b29..8b6ae3f7d78d 100644 --- a/ext/standard/tests/file/bug69442.phpt +++ b/ext/standard/tests/file/bug69442.phpt @@ -17,7 +17,7 @@ EOC; $output = join("\n", $output); unlink($tmpFile); - if (strstr($output, "PTY pseudo terminal not supported on this system") !== false) { + if (strstr($output, "PTY (pseudoterminal) not supported on this system") !== false) { die("skip PTY pseudo terminals are not supported"); } --FILE-- @@ -28,17 +28,31 @@ $pipes = array(); $process = proc_open($cmd, $descriptors, $pipes); -foreach ($pipes as $type => $pipe) { - $data = fread($pipe, 999); - echo 'type ' . $type . ' '; - var_dump($data); - fclose($pipe); +function read_from_pipe($pipe) { + $result = fread($pipe, 1000); + /* We can't guarantee that everything written to the pipe will be returned by a single call + * to fread(), even if it was written with a single syscall and the number of bytes written + * was small */ + $again = @fread($pipe, 1000); + if ($again) { + $result .= $again; + } + return $result; } + +$data0 = read_from_pipe($pipes[0]); +echo 'read from pipe 0: '; +var_dump($data0); +fclose($pipes[0]); + +$data3 = read_from_pipe($pipes[3]); +echo 'read from pipe 3: '; +var_dump($data3); +fclose($pipes[3]); + proc_close($process); --EXPECT-- -type 0 string(5) "foo +read from pipe 0: string(5) "foo " -type 1 string(0) "" -type 2 string(0) "" -type 3 string(3) "42 +read from pipe 3: string(3) "42 " From d29d72b382edd10417c27cb80f9d9f28d4a0f4ff Mon Sep 17 00:00:00 2001 From: Alex Dowad Date: Sat, 9 May 2020 18:16:45 +0200 Subject: [PATCH 2/4] Don't leak memory if wrong resource type is passed to proc_open proc_open can accept stream resources in the descriptorspec, like this: proc_open("command", array(0 => $resource), $pipes); Previously, if a resource which was *not* of type "stream" was passed, proc_open would return without freeing dynamically allocated memory. It's fixed now. --- ext/standard/proc_open.c | 8 +++++--- .../file/proc_open_with_wrong_resource_type.phpt | 14 ++++++++++++++ 2 files changed, 19 insertions(+), 3 deletions(-) create mode 100644 ext/standard/tests/file/proc_open_with_wrong_resource_type.phpt diff --git a/ext/standard/proc_open.c b/ext/standard/proc_open.c index 186501acbd45..5a574cb2ffee 100644 --- a/ext/standard/proc_open.c +++ b/ext/standard/proc_open.c @@ -933,12 +933,14 @@ PHP_FUNCTION(proc_open) if (Z_TYPE_P(descitem) == IS_RESOURCE) { /* should be a stream - try and dup the descriptor */ - php_stream *stream; + php_stream *stream = (php_stream*)zend_fetch_resource(Z_RES_P(descitem), "stream", php_file_le_stream()); + if (stream == NULL) { + goto exit_fail; + } + php_socket_t fd; php_file_descriptor_t desc; - php_stream_from_zval(stream, descitem); - if (FAILURE == php_stream_cast(stream, PHP_STREAM_AS_FD, (void **)&fd, REPORT_ERRORS)) { goto exit_fail; } diff --git a/ext/standard/tests/file/proc_open_with_wrong_resource_type.phpt b/ext/standard/tests/file/proc_open_with_wrong_resource_type.phpt new file mode 100644 index 000000000000..f48c7b872003 --- /dev/null +++ b/ext/standard/tests/file/proc_open_with_wrong_resource_type.phpt @@ -0,0 +1,14 @@ +--TEST-- +proc_open does not leak memory when called with wrong resource type in descriptorspec +--FILE-- + $context), $pipes); + echo "Not reached"; + } catch (TypeError $e) { + echo $e->getMessage(), "\n"; + } +?> +--EXPECT-- +proc_open(): supplied resource is not a valid stream resource From ba0bbaf0957e60a93f7d48abcd931c40ba9f36ef Mon Sep 17 00:00:00 2001 From: Alex Dowad Date: Tue, 12 May 2020 08:43:29 +0200 Subject: [PATCH 3/4] Adjust struct names in proc_open.h (makes proc_open.c more terse) --- ext/standard/proc_open.c | 26 +++++++++++++------------- ext/standard/proc_open.h | 15 +++++++-------- 2 files changed, 20 insertions(+), 21 deletions(-) diff --git a/ext/standard/proc_open.c b/ext/standard/proc_open.c index 5a574cb2ffee..e2fa139004a6 100644 --- a/ext/standard/proc_open.c +++ b/ext/standard/proc_open.c @@ -57,10 +57,10 @@ static int le_proc_open; /* {{{ _php_array_to_envp */ -static php_process_env_t _php_array_to_envp(zval *environment) +static php_process_env _php_array_to_envp(zval *environment) { zval *element; - php_process_env_t env; + php_process_env env; zend_string *key, *str; #ifndef PHP_WIN32 char **ep; @@ -140,7 +140,7 @@ static php_process_env_t _php_array_to_envp(zval *environment) /* }}} */ /* {{{ _php_free_envp */ -static void _php_free_envp(php_process_env_t env) +static void _php_free_envp(php_process_env env) { #ifndef PHP_WIN32 if (env.envarray) { @@ -156,7 +156,7 @@ static void _php_free_envp(php_process_env_t env) /* {{{ proc_open_rsrc_dtor */ static void proc_open_rsrc_dtor(zend_resource *rsrc) { - struct php_process_handle *proc = (struct php_process_handle*)rsrc->ptr; + php_process_handle *proc = (php_process_handle*)rsrc->ptr; #ifdef PHP_WIN32 DWORD wstatus; #elif HAVE_SYS_WAIT_H @@ -227,7 +227,7 @@ PHP_MINIT_FUNCTION(proc_open) PHP_FUNCTION(proc_terminate) { zval *zproc; - struct php_process_handle *proc; + php_process_handle *proc; zend_long sig_no = SIGTERM; ZEND_PARSE_PARAMETERS_START(1, 2) @@ -236,7 +236,7 @@ PHP_FUNCTION(proc_terminate) Z_PARAM_LONG(sig_no) ZEND_PARSE_PARAMETERS_END(); - if ((proc = (struct php_process_handle *)zend_fetch_resource(Z_RES_P(zproc), "process", le_proc_open)) == NULL) { + if ((proc = (php_process_handle*)zend_fetch_resource(Z_RES_P(zproc), "process", le_proc_open)) == NULL) { RETURN_THROWS(); } @@ -261,13 +261,13 @@ PHP_FUNCTION(proc_terminate) PHP_FUNCTION(proc_close) { zval *zproc; - struct php_process_handle *proc; + php_process_handle *proc; ZEND_PARSE_PARAMETERS_START(1, 1) Z_PARAM_RESOURCE(zproc) ZEND_PARSE_PARAMETERS_END(); - if ((proc = (struct php_process_handle *)zend_fetch_resource(Z_RES_P(zproc), "process", le_proc_open)) == NULL) { + if ((proc = (php_process_handle*)zend_fetch_resource(Z_RES_P(zproc), "process", le_proc_open)) == NULL) { RETURN_THROWS(); } @@ -283,7 +283,7 @@ PHP_FUNCTION(proc_close) PHP_FUNCTION(proc_get_status) { zval *zproc; - struct php_process_handle *proc; + php_process_handle *proc; #ifdef PHP_WIN32 DWORD wstatus; #elif HAVE_SYS_WAIT_H @@ -297,7 +297,7 @@ PHP_FUNCTION(proc_get_status) Z_PARAM_RESOURCE(zproc) ZEND_PARSE_PARAMETERS_END(); - if ((proc = (struct php_process_handle *)zend_fetch_resource(Z_RES_P(zproc), "process", le_proc_open)) == NULL) { + if ((proc = (php_process_handle*)zend_fetch_resource(Z_RES_P(zproc), "process", le_proc_open)) == NULL) { RETURN_THROWS(); } @@ -841,7 +841,7 @@ PHP_FUNCTION(proc_open) zval *pipes; zval *environment = NULL; zval *other_options = NULL; - php_process_env_t env; + php_process_env env; int ndesc = 0; int i; zval *descitem = NULL; @@ -868,7 +868,7 @@ PHP_FUNCTION(proc_open) #endif int pty_master_fd = -1, pty_slave_fd = -1; php_process_id_t child; - struct php_process_handle *proc; + php_process_handle *proc; ZEND_PARSE_PARAMETERS_START(3, 6) Z_PARAM_ZVAL(command_zv) @@ -1088,7 +1088,7 @@ PHP_FUNCTION(proc_open) goto exit_fail; } - proc = (struct php_process_handle*) emalloc(sizeof(struct php_process_handle)); + proc = (php_process_handle*) emalloc(sizeof(php_process_handle)); proc->command = command; proc->pipes = emalloc(sizeof(zend_resource *) * ndesc); proc->npipes = ndesc; diff --git a/ext/standard/proc_open.h b/ext/standard/proc_open.h index e6bfd4c74229..d10cb0ecf06c 100644 --- a/ext/standard/proc_open.h +++ b/ext/standard/proc_open.h @@ -24,18 +24,17 @@ typedef pid_t php_process_id_t; # define PHP_INVALID_FD (-1) #endif -/* Environment block under win32 is a NUL terminated sequence of NUL terminated - * name=value strings. - * Under unix, it is an argv style array. - * */ +/* Environment block under Win32 is a NUL terminated sequence of NUL terminated + * name=value strings. + * Under Unix, it is an argv style array. */ typedef struct _php_process_env { char *envp; #ifndef PHP_WIN32 char **envarray; #endif -} php_process_env_t; +} php_process_env; -struct php_process_handle { +typedef struct _php_process_handle { php_process_id_t child; #ifdef PHP_WIN32 HANDLE childHandle; @@ -43,5 +42,5 @@ struct php_process_handle { int npipes; zend_resource **pipes; char *command; - php_process_env_t env; -}; + php_process_env env; +} php_process_handle; From 507cac01563ced1e4a81a8a0b9c0b45770efa7ee Mon Sep 17 00:00:00 2001 From: Alex Dowad Date: Tue, 12 May 2020 08:47:33 +0200 Subject: [PATCH 4/4] Further refactoring of proc_open.c This time a number of comments have been added to make it easy for new devs to understand what is going on. Also adjusted error message to use colons rather than dashes. --- ext/standard/proc_open.c | 309 ++++++++++-------- .../general_functions/proc_open_pipes3.phpt | 2 +- 2 files changed, 172 insertions(+), 139 deletions(-) diff --git a/ext/standard/proc_open.c b/ext/standard/proc_open.c index e2fa139004a6..f1ecfd932e8c 100644 --- a/ext/standard/proc_open.c +++ b/ext/standard/proc_open.c @@ -47,16 +47,19 @@ # if HAVE_PTY_H # include # else -/* Mac OS X defines openpty() in */ +/* Mac OS X defines `openpty` in */ # include # endif #endif #include "proc_open.h" -static int le_proc_open; +static int le_proc_open; /* Resource number for `proc` resources */ -/* {{{ _php_array_to_envp */ +/* {{{ _php_array_to_envp + * Process the `environment` argument to `proc_open` + * Convert into data structures which can be passed to underlying OS APIs like `exec` on POSIX or + * `CreateProcessW` on Win32 */ static php_process_env _php_array_to_envp(zval *environment) { zval *element; @@ -67,7 +70,7 @@ static php_process_env _php_array_to_envp(zval *environment) #endif char *p; size_t cnt, sizeenv = 0; - HashTable *env_hash; + HashTable *env_hash; /* temporary PHP array used as helper */ memset(&env, 0, sizeof(env)); @@ -139,7 +142,8 @@ static php_process_env _php_array_to_envp(zval *environment) } /* }}} */ -/* {{{ _php_free_envp */ +/* {{{ _php_free_envp + * Free the structures allocated by `_php_array_to_envp` */ static void _php_free_envp(php_process_env env) { #ifndef PHP_WIN32 @@ -153,7 +157,9 @@ static void _php_free_envp(php_process_env env) } /* }}} */ -/* {{{ proc_open_rsrc_dtor */ +/* {{{ proc_open_rsrc_dtor + * Free `proc` resource, either because all references to it were dropped or because `pclose` or + * `proc_close` were called */ static void proc_open_rsrc_dtor(zend_resource *rsrc) { php_process_handle *proc = (php_process_handle*)rsrc->ptr; @@ -174,6 +180,10 @@ static void proc_open_rsrc_dtor(zend_resource *rsrc) } } + /* `pclose_wait` tells us: Are we freeing this resource because `pclose` or `proc_close` were + * called? If so, we need to wait until the child process exits, because its exit code is + * needed as the return value of those functions. + * But if we're freeing the resource because of GC, don't wait. */ #ifdef PHP_WIN32 if (FG(pclose_wait)) { WaitForSingleObject(proc->childHandle, INFINITE); @@ -187,7 +197,6 @@ static void proc_open_rsrc_dtor(zend_resource *rsrc) CloseHandle(proc->childHandle); #elif HAVE_SYS_WAIT_H - if (!FG(pclose_wait)) { waitpid_options = WNOHANG; } @@ -198,32 +207,34 @@ static void proc_open_rsrc_dtor(zend_resource *rsrc) if (wait_pid <= 0) { FG(pclose_ret) = -1; } else { - if (WIFEXITED(wstatus)) + if (WIFEXITED(wstatus)) { wstatus = WEXITSTATUS(wstatus); + } FG(pclose_ret) = wstatus; } #else FG(pclose_ret) = -1; #endif + _php_free_envp(proc->env); efree(proc->pipes); efree(proc->command); efree(proc); - } /* }}} */ /* {{{ PHP_MINIT_FUNCTION(proc_open) */ PHP_MINIT_FUNCTION(proc_open) { - le_proc_open = zend_register_list_destructors_ex(proc_open_rsrc_dtor, NULL, "process", module_number); + le_proc_open = zend_register_list_destructors_ex(proc_open_rsrc_dtor, NULL, "process", + module_number); return SUCCESS; } /* }}} */ /* {{{ proto bool proc_terminate(resource process [, int signal]) - kill a process opened by proc_open */ + Kill a process opened by `proc_open` */ PHP_FUNCTION(proc_terminate) { zval *zproc; @@ -236,28 +247,21 @@ PHP_FUNCTION(proc_terminate) Z_PARAM_LONG(sig_no) ZEND_PARSE_PARAMETERS_END(); - if ((proc = (php_process_handle*)zend_fetch_resource(Z_RES_P(zproc), "process", le_proc_open)) == NULL) { + proc = (php_process_handle*)zend_fetch_resource(Z_RES_P(zproc), "process", le_proc_open); + if (proc == NULL) { RETURN_THROWS(); } #ifdef PHP_WIN32 - if (TerminateProcess(proc->childHandle, 255)) { - RETURN_TRUE; - } else { - RETURN_FALSE; - } + RETURN_BOOL(TerminateProcess(proc->childHandle, 255)); #else - if (kill(proc->child, sig_no) == 0) { - RETURN_TRUE; - } else { - RETURN_FALSE; - } + RETURN_BOOL(kill(proc->child, sig_no) == 0); #endif } /* }}} */ /* {{{ proto int|false proc_close(resource process) - close a process opened by proc_open */ + Close a process opened by `proc_open` */ PHP_FUNCTION(proc_close) { zval *zproc; @@ -267,11 +271,12 @@ PHP_FUNCTION(proc_close) Z_PARAM_RESOURCE(zproc) ZEND_PARSE_PARAMETERS_END(); - if ((proc = (php_process_handle*)zend_fetch_resource(Z_RES_P(zproc), "process", le_proc_open)) == NULL) { + proc = (php_process_handle*)zend_fetch_resource(Z_RES_P(zproc), "process", le_proc_open); + if (proc == NULL) { RETURN_THROWS(); } - FG(pclose_wait) = 1; + FG(pclose_wait) = 1; /* See comment in `proc_open_rsrc_dtor` */ zend_list_close(Z_RES_P(zproc)); FG(pclose_wait) = 0; RETURN_LONG(FG(pclose_ret)); @@ -279,7 +284,7 @@ PHP_FUNCTION(proc_close) /* }}} */ /* {{{ proto array|false proc_get_status(resource process) - get information about a process opened by proc_open */ + Get information about a process opened by `proc_open` */ PHP_FUNCTION(proc_get_status) { zval *zproc; @@ -297,25 +302,21 @@ PHP_FUNCTION(proc_get_status) Z_PARAM_RESOURCE(zproc) ZEND_PARSE_PARAMETERS_END(); - if ((proc = (php_process_handle*)zend_fetch_resource(Z_RES_P(zproc), "process", le_proc_open)) == NULL) { + proc = (php_process_handle*)zend_fetch_resource(Z_RES_P(zproc), "process", le_proc_open); + if (proc == NULL) { RETURN_THROWS(); } array_init(return_value); - add_assoc_string(return_value, "command", proc->command); - add_assoc_long(return_value, "pid", (zend_long) proc->child); + add_assoc_long(return_value, "pid", (zend_long)proc->child); #ifdef PHP_WIN32 - GetExitCodeProcess(proc->childHandle, &wstatus); - running = wstatus == STILL_ACTIVE; exitcode = running ? -1 : wstatus; #elif HAVE_SYS_WAIT_H - - errno = 0; wait_pid = waitpid(proc->child, &wstatus, WNOHANG|WUNTRACED); if (wait_pid == proc->child) { @@ -326,7 +327,6 @@ PHP_FUNCTION(proc_get_status) if (WIFSIGNALED(wstatus)) { running = 0; signaled = 1; - termsig = WTERMSIG(wstatus); } if (WIFSTOPPED(wstatus)) { @@ -334,6 +334,8 @@ PHP_FUNCTION(proc_get_status) stopsig = WSTOPSIG(wstatus); } } else if (wait_pid == -1) { + /* The only error which could occur here is ECHILD, which means that the PID we were + * looking for either does not exist or is not a child of this process */ running = 0; } #endif @@ -347,10 +349,11 @@ PHP_FUNCTION(proc_get_status) } /* }}} */ -/* {{{ handy definitions for portability/readability */ #ifdef PHP_WIN32 -/* we use this to allow child processes to inherit handles */ +/* We use this to allow child processes to inherit handles + * One static instance can be shared and used for all calls to `proc_open`, since the values are + * never changed */ SECURITY_ATTRIBUTES php_proc_open_security = { .nLength = sizeof(SECURITY_ATTRIBUTES), .lpSecurityDescriptor = NULL, @@ -377,17 +380,21 @@ static inline HANDLE dup_fd_as_handle(int fd) } # define close_descriptor(fd) CloseHandle(fd) -#else +#else /* !PHP_WIN32 */ # define close_descriptor(fd) close(fd) #endif -struct php_proc_open_descriptor_item { - int index; /* desired FD number in child process */ +/* One instance of this struct is created for each item in `$descriptorspec` argument to `proc_open` + * They are used within `proc_open` and freed before it returns */ +typedef struct _descriptorspec_item { + int index; /* desired FD # in child process */ int is_pipe; - php_file_descriptor_t parentend, childend; /* FDs for pipes in parent/child */ - int mode_flags; /* mode flags for opening FDs */ -}; -/* }}} */ + php_file_descriptor_t childend; /* FD # opened for use in child + * (will be copied to `index` in child) */ + php_file_descriptor_t parentend; /* FD # opened for use in parent + * (for pipes only; will be 0 otherwise) */ + int mode_flags; /* mode for opening FDs: r/o, r/w, binary (on Win32), etc */ +} descriptorspec_item; static zend_string *get_valid_arg_string(zval *zv, int elem_num) { zend_string *str = zval_get_string(zv); @@ -463,18 +470,20 @@ static char *create_win_command_from_args(HashTable *args) return str.c; } -static int get_option(zval *other_options, char *option_name) +/* Get a boolean option from the `other_options` array which can be passed to `proc_open`. + * (Currently, all options apply on Windows only.) */ +static int get_option(zval *other_options, char *opt_name) { - zval *item = zend_hash_str_find_deref(Z_ARRVAL_P(other_options), option_name, strlen(option_name)); - if (item != NULL) { - if (Z_TYPE_P(item) == IS_TRUE || ((Z_TYPE_P(item) == IS_LONG) && Z_LVAL_P(item))) { - return 1; - } - } - return 0; + HashTable *opt_ary = Z_ARRVAL_P(other_options); + zval *item = zend_hash_str_find_deref(opt_ary, opt_name, strlen(opt_name)); + return item != NULL && + (Z_TYPE_P(item) == IS_TRUE || + ((Z_TYPE_P(item) == IS_LONG) && Z_LVAL_P(item))); } -static void init_startup_info(STARTUPINFOW *si, struct php_proc_open_descriptor_item *descriptors, int ndesc) +/* Initialize STARTUPINFOW struct, used on Windows when spawning a process. + * Ref: https://docs.microsoft.com/en-us/windows/win32/api/processthreadsapi/ns-processthreadsapi-startupinfow */ +static void init_startup_info(STARTUPINFOW *si, descriptorspec_item *descriptors, int ndesc) { memset(si, 0, sizeof(STARTUPINFOW)); si->cb = sizeof(STARTUPINFOW); @@ -528,6 +537,7 @@ static int convert_command_to_use_shell(wchar_t **cmdw, size_t cmdw_len) } #endif +/* Convert command parameter array passed as first argument to `proc_open` into command string */ static char* get_command_from_array(zval *array, char ***argv, int num_elems) { zval *arg_zv; @@ -539,7 +549,7 @@ static char* get_command_from_array(zval *array, char ***argv, int num_elems) ZEND_HASH_FOREACH_VAL(Z_ARRVAL_P(array), arg_zv) { zend_string *arg_str = get_valid_arg_string(arg_zv, i + 1); if (!arg_str) { - /* terminate argv with NULL so exit_fail code knows how many entries to free */ + /* Terminate with NULL so exit_fail code knows how many entries to free */ (*argv)[i] = NULL; if (command != NULL) { efree(command); @@ -559,10 +569,10 @@ static char* get_command_from_array(zval *array, char ***argv, int num_elems) return command; } -static struct php_proc_open_descriptor_item* alloc_descriptor_array(zval *descriptorspec) +static descriptorspec_item* alloc_descriptor_array(zval *descriptorspec) { int ndescriptors = zend_hash_num_elements(Z_ARRVAL_P(descriptorspec)); - return ecalloc(sizeof(struct php_proc_open_descriptor_item), ndescriptors); + return ecalloc(sizeof(descriptorspec_item), ndescriptors); } static zend_string* get_string_parameter(zval *array, int index, char *param_name) @@ -575,12 +585,11 @@ static zend_string* get_string_parameter(zval *array, int index, char *param_nam return zval_try_get_string(array_item); } -static int set_proc_descriptor_to_blackhole(struct php_proc_open_descriptor_item *desc) +static int set_proc_descriptor_to_blackhole(descriptorspec_item *desc) { #ifdef PHP_WIN32 - desc->childend = CreateFileA( - "nul", GENERIC_READ | GENERIC_WRITE, FILE_SHARE_READ | FILE_SHARE_WRITE, - NULL, OPEN_EXISTING, 0, NULL); + desc->childend = CreateFileA("nul", GENERIC_READ | GENERIC_WRITE, + FILE_SHARE_READ | FILE_SHARE_WRITE, NULL, OPEN_EXISTING, 0, NULL); if (desc->childend == NULL) { php_error_docref(NULL, E_WARNING, "Failed to open nul"); return FAILURE; @@ -588,19 +597,24 @@ static int set_proc_descriptor_to_blackhole(struct php_proc_open_descriptor_item #else desc->childend = open("/dev/null", O_RDWR); if (desc->childend < 0) { - php_error_docref(NULL, E_WARNING, "Failed to open /dev/null - %s", strerror(errno)); + php_error_docref(NULL, E_WARNING, "Failed to open /dev/null: %s", strerror(errno)); return FAILURE; } #endif return SUCCESS; } -static int set_proc_descriptor_to_pty(struct php_proc_open_descriptor_item *desc, int *master_fd, int *slave_fd) +static int set_proc_descriptor_to_pty(descriptorspec_item *desc, int *master_fd, int *slave_fd) { #if HAVE_OPENPTY + /* All FDs set to PTY in the child process will go to the slave end of the same PTY. + * Likewise, all the corresponding entries in `$pipes` in the parent will all go to the master + * end of the same PTY. + * If this is the first descriptorspec set to 'pty', find an available PTY and get master and + * slave FDs. */ if (*master_fd == -1) { if (openpty(master_fd, slave_fd, NULL, NULL, NULL)) { - php_error_docref(NULL, E_WARNING, "Could not open PTY (pseudoterminal) - %s", strerror(errno)); + php_error_docref(NULL, E_WARNING, "Could not open PTY (pseudoterminal): %s", strerror(errno)); return FAILURE; } } @@ -616,7 +630,7 @@ static int set_proc_descriptor_to_pty(struct php_proc_open_descriptor_item *desc #endif } -static int set_proc_descriptor_to_pipe(struct php_proc_open_descriptor_item *desc, zend_string *zmode) +static int set_proc_descriptor_to_pipe(descriptorspec_item *desc, zend_string *zmode) { php_file_descriptor_t newpipe[2]; @@ -648,19 +662,21 @@ static int set_proc_descriptor_to_pipe(struct php_proc_open_descriptor_item *des return SUCCESS; } -static int set_proc_descriptor_to_file(struct php_proc_open_descriptor_item *desc, zend_string *zfile, zend_string *zmode) +static int set_proc_descriptor_to_file(descriptorspec_item *desc, zend_string *file_path, + zend_string *file_mode) { php_socket_t fd; /* try a wrapper */ - php_stream *stream = php_stream_open_wrapper(ZSTR_VAL(zfile), ZSTR_VAL(zmode), - REPORT_ERRORS|STREAM_WILL_CAST, NULL); + php_stream *stream = php_stream_open_wrapper(ZSTR_VAL(file_path), ZSTR_VAL(file_mode), + REPORT_ERRORS|STREAM_WILL_CAST, NULL); if (stream == NULL) { return FAILURE; } /* force into an fd */ - if (php_stream_cast(stream, PHP_STREAM_CAST_RELEASE|PHP_STREAM_AS_FD, (void **)&fd, REPORT_ERRORS) == FAILURE) { + if (php_stream_cast(stream, PHP_STREAM_CAST_RELEASE|PHP_STREAM_AS_FD, (void **)&fd, + REPORT_ERRORS) == FAILURE) { return FAILURE; } @@ -668,9 +684,9 @@ static int set_proc_descriptor_to_file(struct php_proc_open_descriptor_item *des desc->childend = dup_fd_as_handle((int)fd); _close((int)fd); - /* simulate the append mode by fseeking to the end of the file - this introduces a potential race-condition, but it is the best we can do, though */ - if (strchr(ZSTR_VAL(zmode), 'a')) { + /* Simulate the append mode by fseeking to the end of the file + * This introduces a potential race condition, but it is the best we can do */ + if (strchr(ZSTR_VAL(file_mode), 'a')) { SetFilePointer(desc->childend, 0, NULL, FILE_END); } #else @@ -679,7 +695,8 @@ static int set_proc_descriptor_to_file(struct php_proc_open_descriptor_item *des return SUCCESS; } -static int dup_proc_descriptor(php_file_descriptor_t from, php_file_descriptor_t *to, zend_ulong nindex) +static int dup_proc_descriptor(php_file_descriptor_t from, php_file_descriptor_t *to, + zend_ulong nindex) { #ifdef PHP_WIN32 *to = dup_handle(from, TRUE, FALSE); @@ -690,15 +707,16 @@ static int dup_proc_descriptor(php_file_descriptor_t from, php_file_descriptor_t #else *to = dup(from); if (*to < 0) { - php_error_docref(NULL, E_WARNING, - "Failed to dup() for descriptor " ZEND_LONG_FMT " - %s", nindex, strerror(errno)); + php_error_docref(NULL, E_WARNING, "Failed to dup() for descriptor " ZEND_LONG_FMT ": %s", + nindex, strerror(errno)); return FAILURE; } #endif return SUCCESS; } -static int redirect_proc_descriptor(struct php_proc_open_descriptor_item *desc, int target, struct php_proc_open_descriptor_item *descriptors, int ndesc, int nindex) +static int redirect_proc_descriptor(descriptorspec_item *desc, int target, + descriptorspec_item *descriptors, int ndesc, int nindex) { php_file_descriptor_t redirect_to = PHP_INVALID_FD; @@ -709,7 +727,7 @@ static int redirect_proc_descriptor(struct php_proc_open_descriptor_item *desc, } } - if (redirect_to == PHP_INVALID_FD) { /* didn't find the index we wanted */ + if (redirect_to == PHP_INVALID_FD) { /* Didn't find the index we wanted */ if (target < 0 || target > 2) { php_error_docref(NULL, E_WARNING, "Redirection target %d not found", target); return FAILURE; @@ -732,9 +750,9 @@ static int redirect_proc_descriptor(struct php_proc_open_descriptor_item *desc, return dup_proc_descriptor(redirect_to, &desc->childend, nindex); } -int set_proc_descriptor_from_array( - zval *descitem, struct php_proc_open_descriptor_item *descriptors, int ndesc, - int nindex, int *pty_master_fd, int *pty_slave_fd) { +/* Process one item from `$descriptorspec` argument to `proc_open` */ +static int set_proc_descriptor_from_array(zval *descitem, descriptorspec_item *descriptors, + int ndesc, int nindex, int *pty_master_fd, int *pty_slave_fd) { zend_string *ztype = get_string_parameter(descitem, 0, "handle qualifier"); if (!ztype) { return FAILURE; @@ -743,21 +761,23 @@ int set_proc_descriptor_from_array( zend_string *zmode = NULL, *zfile = NULL; int retval = FAILURE; if (zend_string_equals_literal(ztype, "pipe")) { - if ((zmode = get_string_parameter(descitem, 1, "mode parameter for 'pipe'")) == NULL) { + /* Set descriptor to pipe */ + zmode = get_string_parameter(descitem, 1, "mode parameter for 'pipe'"); + if (zmode == NULL) { goto finish; } - retval = set_proc_descriptor_to_pipe(&descriptors[ndesc], zmode); } else if (zend_string_equals_literal(ztype, "file")) { + /* Set descriptor to file */ if ((zfile = get_string_parameter(descitem, 1, "file name parameter for 'file'")) == NULL) { goto finish; } if ((zmode = get_string_parameter(descitem, 2, "mode parameter for 'file'")) == NULL) { goto finish; } - retval = set_proc_descriptor_to_file(&descriptors[ndesc], zfile, zmode); } else if (zend_string_equals_literal(ztype, "redirect")) { + /* Redirect descriptor to whatever another descriptor is set to */ zval *ztarget = zend_hash_index_find_deref(Z_ARRVAL_P(descitem), 1); if (!ztarget) { zend_value_error("Missing redirection target"); @@ -771,8 +791,10 @@ int set_proc_descriptor_from_array( retval = redirect_proc_descriptor( &descriptors[ndesc], Z_LVAL_P(ztarget), descriptors, ndesc, nindex); } else if (zend_string_equals_literal(ztype, "null")) { + /* Set descriptor to blackhole (discard all data written) */ retval = set_proc_descriptor_to_blackhole(&descriptors[ndesc]); } else if (zend_string_equals_literal(ztype, "pty")) { + /* Set descriptor to slave end of PTY */ retval = set_proc_descriptor_to_pty(&descriptors[ndesc], pty_master_fd, pty_slave_fd); } else { php_error_docref(NULL, E_WARNING, "%s is not a valid descriptor spec/mode", ZSTR_VAL(ztype)); @@ -786,20 +808,47 @@ int set_proc_descriptor_from_array( return retval; } -static int close_parent_ends_of_pipes_in_child(struct php_proc_open_descriptor_item *descriptors, int ndesc) +static int set_proc_descriptor_from_resource(zval *resource, descriptorspec_item *desc, int nindex) { - /* we are running in child process - * close the 'parent end' of all pipes which were opened before forking/spawning - * also, dup() the child end of all pipes as necessary so they will use the FD number - * which the user requested */ + /* Should be a stream - try and dup the descriptor */ + php_stream *stream = (php_stream*)zend_fetch_resource(Z_RES_P(resource), "stream", + php_file_le_stream()); + if (stream == NULL) { + return FAILURE; + } + + php_socket_t fd; + int status = php_stream_cast(stream, PHP_STREAM_AS_FD, (void **)&fd, REPORT_ERRORS); + if (status == FAILURE) { + return FAILURE; + } + +#ifdef PHP_WIN32 + php_file_descriptor_t fd_t = (HANDLE)_get_osfhandle(fd); +#else + php_file_descriptor_t fd_t = fd; +#endif + if (dup_proc_descriptor(fd_t, &desc->childend, nindex) == FAILURE) { + return FAILURE; + } + + return SUCCESS; +} + +static int close_parentends_of_pipes(descriptorspec_item *descriptors, int ndesc) +{ + /* We are running in child process + * Close the 'parent end' of pipes which were opened before forking/spawning + * Also, dup() the child end of all pipes as necessary so they will use the FD + * number which the user requested */ for (int i = 0; i < ndesc; i++) { if (descriptors[i].is_pipe) { close(descriptors[i].parentend); } if (descriptors[i].childend != descriptors[i].index) { if (dup2(descriptors[i].childend, descriptors[i].index) < 0) { - php_error_docref(NULL, E_WARNING, "Unable to copy file descriptor %d (for pipe) into file descriptor %d - %s", - descriptors[i].childend, descriptors[i].index, strerror(errno)); + php_error_docref(NULL, E_WARNING, "Unable to copy file descriptor %d (for pipe) into " \ + "file descriptor %d: %s", descriptors[i].childend, descriptors[i].index, strerror(errno)); return FAILURE; } close(descriptors[i].childend); @@ -809,7 +858,7 @@ static int close_parent_ends_of_pipes_in_child(struct php_proc_open_descriptor_i return SUCCESS; } -static void close_all_descriptors(struct php_proc_open_descriptor_item *descriptors, int ndesc) +static void close_all_descriptors(descriptorspec_item *descriptors, int ndesc) { for (int i = 0; i < ndesc; i++) { close_descriptor(descriptors[i].childend); @@ -831,23 +880,22 @@ static void efree_argv(char **argv) } /* {{{ proto resource|false proc_open(string|array command, array descriptorspec, array &pipes [, string cwd [, array env [, array other_options]]]) - Run a process with more control over it's file descriptors */ + Execute a command, with specified files used for input/output */ PHP_FUNCTION(proc_open) { - zval *command_zv; - char *command = NULL, *cwd = NULL; - size_t cwd_len = 0; - zval *descriptorspec; - zval *pipes; - zval *environment = NULL; - zval *other_options = NULL; + zval *command_zv, *descriptorspec, *pipes; /* Mandatory arguments */ + char *cwd = NULL; /* Optional argument */ + size_t cwd_len = 0; /* Optional argument */ + zval *environment = NULL, *other_options = NULL; /* Optional arguments */ + + char *command = NULL; php_process_env env; int ndesc = 0; int i; zval *descitem = NULL; zend_string *str_index; zend_ulong nindex; - struct php_proc_open_descriptor_item *descriptors = NULL; + descriptorspec_item *descriptors = NULL; #ifdef PHP_WIN32 PROCESS_INFORMATION pi; HANDLE childHandle; @@ -890,13 +938,15 @@ PHP_FUNCTION(proc_open) } #ifdef PHP_WIN32 + /* Automatically bypass shell if command is given as an array */ bypass_shell = 1; command = create_win_command_from_args(Z_ARRVAL_P(command_zv)); if (!command) { RETURN_FALSE; } #else - if ((command = get_command_from_array(command_zv, &argv, num_elems)) == NULL) { + command = get_command_from_array(command_zv, &argv, num_elems); + if (command == NULL) { goto exit_fail; } #endif @@ -908,7 +958,7 @@ PHP_FUNCTION(proc_open) #ifdef PHP_WIN32 if (other_options) { suppress_errors = get_option(other_options, "suppress_errors"); - /* TODO Deprecate in favor of array command? */ + /* TODO: Deprecate in favor of array command? */ bypass_shell = bypass_shell || get_option(other_options, "bypass_shell"); blocking_pipes = get_option(other_options, "blocking_pipes"); create_process_group = get_option(other_options, "create_process_group"); @@ -922,7 +972,7 @@ PHP_FUNCTION(proc_open) descriptors = alloc_descriptor_array(descriptorspec); - /* walk the descriptor spec and set up files/pipes */ + /* Walk the descriptor spec and set up files/pipes */ ZEND_HASH_FOREACH_KEY_VAL(Z_ARRVAL_P(descriptorspec), nindex, str_index, descitem) { if (str_index) { zend_argument_value_error(2, "must be an integer indexed array"); @@ -932,34 +982,16 @@ PHP_FUNCTION(proc_open) descriptors[ndesc].index = (int)nindex; if (Z_TYPE_P(descitem) == IS_RESOURCE) { - /* should be a stream - try and dup the descriptor */ - php_stream *stream = (php_stream*)zend_fetch_resource(Z_RES_P(descitem), "stream", php_file_le_stream()); - if (stream == NULL) { - goto exit_fail; - } - - php_socket_t fd; - php_file_descriptor_t desc; - - if (FAILURE == php_stream_cast(stream, PHP_STREAM_AS_FD, (void **)&fd, REPORT_ERRORS)) { - goto exit_fail; - } - -#ifdef PHP_WIN32 - desc = (HANDLE)_get_osfhandle(fd); -#else - desc = fd; -#endif - if (dup_proc_descriptor(desc, &descriptors[ndesc].childend, nindex) == FAILURE) { + if (set_proc_descriptor_from_resource(descitem, &descriptors[ndesc], ndesc) == FAILURE) { goto exit_fail; } } else if (Z_TYPE_P(descitem) == IS_ARRAY) { - if (set_proc_descriptor_from_array(descitem, descriptors, ndesc, nindex, - &pty_master_fd, &pty_slave_fd) == FAILURE) { + if (set_proc_descriptor_from_array(descitem, descriptors, ndesc, nindex, &pty_master_fd, + &pty_slave_fd) == FAILURE) { goto exit_fail; } } else { - zend_argument_value_error(2, "must only contain arrays and File-Handles"); + zend_argument_value_error(2, "must only contain arrays and streams"); goto exit_fail; } ndesc++; @@ -1018,17 +1050,17 @@ PHP_FUNCTION(proc_open) goto exit_fail; } } - newprocok = CreateProcessW(NULL, cmdw, &php_proc_open_security, &php_proc_open_security, - TRUE, dwCreateFlags, envpw, cwdw, &si, &pi); + newprocok = CreateProcessW(NULL, cmdw, &php_proc_open_security, + &php_proc_open_security, TRUE, dwCreateFlags, envpw, cwdw, &si, &pi); if (suppress_errors) { SetErrorMode(old_error_mode); } - if (FALSE == newprocok) { + if (newprocok == FALSE) { DWORD dw = GetLastError(); close_all_descriptors(descriptors, ndesc); - php_error_docref(NULL, E_WARNING, "CreateProcess failed, error code - %u", dw); + php_error_docref(NULL, E_WARNING, "CreateProcess failed, error code: %u", dw); goto exit_fail; } @@ -1036,15 +1068,15 @@ PHP_FUNCTION(proc_open) child = pi.dwProcessId; CloseHandle(pi.hThread); #elif HAVE_FORK - /* the unix way */ + /* the Unix way */ child = fork(); if (child == 0) { - /* this is the child process */ + /* This is the child process */ - if (close_parent_ends_of_pipes_in_child(descriptors, ndesc) == FAILURE) { + if (close_parentends_of_pipes(descriptors, ndesc) == FAILURE) { /* We are already in child process and can't do anything to make - * proc_open() return an error in the parent + * `proc_open` return an error in the parent * All we can do is exit with a non-zero (error) exit code */ _exit(127); } @@ -1069,19 +1101,19 @@ PHP_FUNCTION(proc_open) /* If execvp/execle/execl are successful, we will never reach here * Display error and exit with non-zero (error) status code */ - php_error_docref(NULL, E_WARNING, "Exec failed - %s", strerror(errno)); + php_error_docref(NULL, E_WARNING, "Exec failed: %s", strerror(errno)); _exit(127); } else if (child < 0) { - /* failed to fork() */ + /* Failed to fork() */ close_all_descriptors(descriptors, ndesc); - php_error_docref(NULL, E_WARNING, "Fork failed - %s", strerror(errno)); + php_error_docref(NULL, E_WARNING, "Fork failed: %s", strerror(errno)); goto exit_fail; } #else # error You lose (configure should not have let you get here) #endif - /* we forked/spawned and this is the parent */ + /* We forked/spawned and this is the parent */ pipes = zend_try_array_init(pipes); if (!pipes) { @@ -1098,8 +1130,8 @@ PHP_FUNCTION(proc_open) #endif proc->env = env; - /* clean up all the child ends and then open streams on the parent - * ends, where appropriate */ + /* Clean up all the child ends and then open streams on the parent + * ends, where appropriate */ for (i = 0; i < ndesc; i++) { char *mode_string = NULL; php_stream *stream = NULL; @@ -1133,7 +1165,8 @@ PHP_FUNCTION(proc_open) #else stream = php_stream_fopen_from_fd(descriptors[i].parentend, mode_string, NULL); # if defined(F_SETFD) && defined(FD_CLOEXEC) - /* mark the descriptor close-on-exec, so that it won't be inherited by potential other children */ + /* Mark the descriptor close-on-exec, so it won't be inherited by + * potential other children */ fcntl(descriptors[i].parentend, F_SETFD, FD_CLOEXEC); # endif #endif diff --git a/ext/standard/tests/general_functions/proc_open_pipes3.phpt b/ext/standard/tests/general_functions/proc_open_pipes3.phpt index 307fbbaa1d22..96ce75bd0cca 100644 --- a/ext/standard/tests/general_functions/proc_open_pipes3.phpt +++ b/ext/standard/tests/general_functions/proc_open_pipes3.phpt @@ -32,7 +32,7 @@ echo "END\n"; ?> --EXPECTF-- Warning: proc_open(): pi is not a valid descriptor spec/mode in %s on line %d -proc_open(): Argument #2 ($descriptorspec) must only contain arrays and File-Handles +proc_open(): Argument #2 ($descriptorspec) must only contain arrays and streams array(4) { [3]=> resource(%d) of type (Unknown)