]> granicus.if.org Git - php/commitdiff
Further refactoring of proc_open.c
authorAlex Dowad <alexinbeijing@gmail.com>
Tue, 12 May 2020 06:43:29 +0000 (08:43 +0200)
committerNikita Popov <nikita.ppv@gmail.com>
Thu, 14 May 2020 08:25:52 +0000 (10:25 +0200)
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
ext/standard/proc_open.h
ext/standard/tests/general_functions/proc_open_pipes3.phpt

index 5a574cb2ffee7337c44b5e97f685a0d5fc48aaa8..f1ecfd932e8c2e632ec5190c036043817b436c3b 100644 (file)
 # if HAVE_PTY_H
 #  include <pty.h>
 # else
-/* Mac OS X defines openpty() in <util.h> */
+/* Mac OS X defines `openpty` in <util.h> */
 #  include <util.h>
 # 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 */
-static php_process_env_t _php_array_to_envp(zval *environment)
+/* {{{ _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;
-       php_process_env_t env;
+       php_process_env env;
        zend_string *key, *str;
 #ifndef PHP_WIN32
        char **ep;
 #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,8 +142,9 @@ static php_process_env_t _php_array_to_envp(zval *environment)
 }
 /* }}} */
 
-/* {{{ _php_free_envp */
-static void _php_free_envp(php_process_env_t env)
+/* {{{ _php_free_envp
+ * Free the structures allocated by `_php_array_to_envp` */
+static void _php_free_envp(php_process_env env)
 {
 #ifndef PHP_WIN32
        if (env.envarray) {
@@ -153,10 +157,12 @@ static void _php_free_envp(php_process_env_t 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)
 {
-       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
@@ -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,36 +207,38 @@ 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;
-       struct php_process_handle *proc;
+       php_process_handle *proc;
        zend_long sig_no = SIGTERM;
 
        ZEND_PARSE_PARAMETERS_START(1, 2)
@@ -236,42 +247,36 @@ 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) {
+       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;
-       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) {
+       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,11 +284,11 @@ 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;
-       struct php_process_handle *proc;
+       php_process_handle *proc;
 #ifdef PHP_WIN32
        DWORD wstatus;
 #elif HAVE_SYS_WAIT_H
@@ -297,25 +302,21 @@ 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) {
+       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 @@ finish:
        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;
-       php_process_env_t env;
+       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;
@@ -868,7 +916,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)
@@ -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,26 +1101,26 @@ 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) {
                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;
@@ -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
index e6bfd4c74229b10f2af02a77c3bf6ba485c69585..d10cb0ecf06ca035781790372edd0e8cacf8e76e 100644 (file)
@@ -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;
index 307fbbaa1d221ba9e4aa79ecf3cf7363a7d0449c..96ce75bd0ccac27575d68c8350a516a08750059f 100644 (file)
@@ -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)