From 98eb60a635d0b83530cf667d699bd4e80ef7401e Mon Sep 17 00:00:00 2001 From: George Peter Banyard Date: Wed, 11 Aug 2021 14:51:55 +0200 Subject: [PATCH] Refactor proc_open() implementation (#7255) * Convert int return types to zend_result in proc_open.c * Use bool instead of int type * Use HashTable directly instead of zval * Convert command field of process handle to zend_string * proc_open() micro-optimization for Windows Prevents some calls to strlen() on Windows --- ext/standard/proc_open.c | 165 +++++++++++++++++++-------------------- ext/standard/proc_open.h | 2 +- 2 files changed, 82 insertions(+), 85 deletions(-) diff --git a/ext/standard/proc_open.c b/ext/standard/proc_open.c index cd9955594ba..a57e66bd979 100644 --- a/ext/standard/proc_open.c +++ b/ext/standard/proc_open.c @@ -26,7 +26,7 @@ #include "php_globals.h" #include "SAPI.h" #include "main/php_network.h" -#include "zend_smart_string.h" +#include "zend_smart_str.h" #if HAVE_SYS_WAIT_H #include @@ -288,7 +288,7 @@ static void proc_open_rsrc_dtor(zend_resource *rsrc) _php_free_envp(proc->env); efree(proc->pipes); - efree(proc->command); + zend_string_release_ex(proc->command, false); efree(proc); } /* }}} */ @@ -361,7 +361,7 @@ PHP_FUNCTION(proc_get_status) int wstatus; pid_t wait_pid; #endif - int running = 1, signaled = 0, stopped = 0; + bool running = 1, signaled = 0, stopped = 0; int exitcode = -1, termsig = 0, stopsig = 0; ZEND_PARSE_PARAMETERS_START(1, 1) @@ -374,7 +374,7 @@ PHP_FUNCTION(proc_get_status) } array_init(return_value); - add_assoc_string(return_value, "command", proc->command); + add_assoc_str(return_value, "command", zend_string_copy(proc->command)); add_assoc_long(return_value, "pid", (zend_long)proc->child); #ifdef PHP_WIN32 @@ -485,40 +485,41 @@ static zend_string *get_valid_arg_string(zval *zv, int elem_num) { } #ifdef PHP_WIN32 -static void append_backslashes(smart_string *str, size_t num_bs) +static void append_backslashes(smart_str *str, size_t num_bs) { for (size_t i = 0; i < num_bs; i++) { - smart_string_appendc(str, '\\'); + smart_str_appendc(str, '\\'); } } /* See https://docs.microsoft.com/en-us/cpp/cpp/parsing-cpp-command-line-arguments */ -static void append_win_escaped_arg(smart_string *str, char *arg) +static void append_win_escaped_arg(smart_str *str, zend_string *arg) { - char c; size_t num_bs = 0; - smart_string_appendc(str, '"'); - while ((c = *arg)) { + + smart_str_appendc(str, '"'); + for (size_t i = 0; i < ZSTR_LEN(arg); ++i) { + char c = ZSTR_VAL(arg)[i]; if (c == '\\') { num_bs++; - } else { - if (c == '"') { - /* Backslashes before " need to be doubled. */ - num_bs = num_bs * 2 + 1; - } - append_backslashes(str, num_bs); - smart_string_appendc(str, c); - num_bs = 0; + continue; } - arg++; + + if (c == '"') { + /* Backslashes before " need to be doubled. */ + num_bs = num_bs * 2 + 1; + } + append_backslashes(str, num_bs); + smart_str_appendc(str, c); + num_bs = 0; } append_backslashes(str, num_bs * 2); - smart_string_appendc(str, '"'); + smart_str_appendc(str, '"'); } -static char *create_win_command_from_args(HashTable *args) +static zend_string *create_win_command_from_args(HashTable *args) { - smart_string str = {0}; + smart_str str = {0}; zval *arg_zv; bool is_prog_name = 1; int elem_num = 0; @@ -526,29 +527,29 @@ static char *create_win_command_from_args(HashTable *args) ZEND_HASH_FOREACH_VAL(args, arg_zv) { zend_string *arg_str = get_valid_arg_string(arg_zv, ++elem_num); if (!arg_str) { - smart_string_free(&str); + smart_str_free(&str); return NULL; } if (!is_prog_name) { - smart_string_appendc(&str, ' '); + smart_str_appendc(&str, ' '); } - append_win_escaped_arg(&str, ZSTR_VAL(arg_str)); + append_win_escaped_arg(&str, arg_str); is_prog_name = 0; zend_string_release(arg_str); } ZEND_HASH_FOREACH_END(); - smart_string_0(&str); - return str.c; + smart_str_0(&str); + return str.s; } /* 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) +static bool get_option(zval *other_options, char *opt_name, size_t opt_name_len) { HashTable *opt_ary = Z_ARRVAL_P(other_options); - zval *item = zend_hash_str_find_deref(opt_ary, opt_name, strlen(opt_name)); + zval *item = zend_hash_str_find_deref(opt_ary, opt_name, opt_name_len); return item != NULL && (Z_TYPE_P(item) == IS_TRUE || ((Z_TYPE_P(item) == IS_LONG) && Z_LVAL_P(item))); @@ -587,7 +588,7 @@ static void init_process_info(PROCESS_INFORMATION *pi) memset(&pi, 0, sizeof(pi)); } -static int convert_command_to_use_shell(wchar_t **cmdw, size_t cmdw_len) +static zend_result convert_command_to_use_shell(wchar_t **cmdw, size_t cmdw_len) { size_t len = sizeof(COMSPEC_NT) + sizeof(" /s /c ") + cmdw_len + 3; wchar_t *cmdw_shell = (wchar_t *)malloc(len * sizeof(wchar_t)); @@ -611,10 +612,10 @@ 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(HashTable *array, char ***argv, int num_elems) +static zend_string* get_command_from_array(HashTable *array, char ***argv, int num_elems) { zval *arg_zv; - char *command = NULL; + zend_string *command = NULL; int i = 0; *argv = safe_emalloc(sizeof(char *), num_elems + 1, 0); @@ -625,13 +626,13 @@ static char* get_command_from_array(HashTable *array, char ***argv, int num_elem /* Terminate with NULL so exit_fail code knows how many entries to free */ (*argv)[i] = NULL; if (command != NULL) { - efree(command); + zend_string_release_ex(command, false); } return NULL; } if (i == 0) { - command = estrdup(ZSTR_VAL(arg_str)); + command = zend_string_copy(arg_str); } (*argv)[i++] = estrdup(ZSTR_VAL(arg_str)); @@ -642,9 +643,9 @@ static char* get_command_from_array(HashTable *array, char ***argv, int num_elem return command; } -static descriptorspec_item* alloc_descriptor_array(zval *descriptorspec) +static descriptorspec_item* alloc_descriptor_array(HashTable *descriptorspec) { - int ndescriptors = zend_hash_num_elements(Z_ARRVAL_P(descriptorspec)); + uint32_t ndescriptors = zend_hash_num_elements(descriptorspec); return ecalloc(sizeof(descriptorspec_item), ndescriptors); } @@ -658,7 +659,7 @@ 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(descriptorspec_item *desc) +static zend_result set_proc_descriptor_to_blackhole(descriptorspec_item *desc) { #ifdef PHP_WIN32 desc->childend = CreateFileA("nul", GENERIC_READ | GENERIC_WRITE, @@ -677,7 +678,7 @@ static int set_proc_descriptor_to_blackhole(descriptorspec_item *desc) return SUCCESS; } -static int set_proc_descriptor_to_pty(descriptorspec_item *desc, int *master_fd, int *slave_fd) +static zend_result 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. @@ -716,7 +717,7 @@ static php_file_descriptor_t make_descriptor_cloexec(php_file_descriptor_t fd) #endif } -static int set_proc_descriptor_to_pipe(descriptorspec_item *desc, zend_string *zmode) +static zend_result set_proc_descriptor_to_pipe(descriptorspec_item *desc, zend_string *zmode) { php_file_descriptor_t newpipe[2]; @@ -753,7 +754,7 @@ static int set_proc_descriptor_to_pipe(descriptorspec_item *desc, zend_string *z #define create_socketpair(socks) socketpair(AF_UNIX, SOCK_STREAM, 0, (socks)) #endif -static int set_proc_descriptor_to_socket(descriptorspec_item *desc) +static zend_result set_proc_descriptor_to_socket(descriptorspec_item *desc) { php_socket_t sock[2]; @@ -773,7 +774,7 @@ static int set_proc_descriptor_to_socket(descriptorspec_item *desc) return SUCCESS; } -static int set_proc_descriptor_to_file(descriptorspec_item *desc, zend_string *file_path, +static zend_result set_proc_descriptor_to_file(descriptorspec_item *desc, zend_string *file_path, zend_string *file_mode) { php_socket_t fd; @@ -806,7 +807,7 @@ static int set_proc_descriptor_to_file(descriptorspec_item *desc, zend_string *f return SUCCESS; } -static int dup_proc_descriptor(php_file_descriptor_t from, php_file_descriptor_t *to, +static zend_result dup_proc_descriptor(php_file_descriptor_t from, php_file_descriptor_t *to, zend_ulong nindex) { #ifdef PHP_WIN32 @@ -826,7 +827,7 @@ static int dup_proc_descriptor(php_file_descriptor_t from, php_file_descriptor_t return SUCCESS; } -static int redirect_proc_descriptor(descriptorspec_item *desc, int target, +static zend_result redirect_proc_descriptor(descriptorspec_item *desc, int target, descriptorspec_item *descriptors, int ndesc, int nindex) { php_file_descriptor_t redirect_to = PHP_INVALID_FD; @@ -862,7 +863,7 @@ static int redirect_proc_descriptor(descriptorspec_item *desc, int target, } /* Process one item from `$descriptorspec` argument to `proc_open` */ -static int set_proc_descriptor_from_array(zval *descitem, descriptorspec_item *descriptors, +static zend_result 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) { @@ -870,7 +871,8 @@ static int set_proc_descriptor_from_array(zval *descitem, descriptorspec_item *d } zend_string *zmode = NULL, *zfile = NULL; - int retval = FAILURE; + zend_result retval = FAILURE; + if (zend_string_equals_literal(ztype, "pipe")) { /* Set descriptor to pipe */ zmode = get_string_parameter(descitem, 1, "mode parameter for 'pipe'"); @@ -922,7 +924,7 @@ finish: return retval; } -static int set_proc_descriptor_from_resource(zval *resource, descriptorspec_item *desc, int nindex) +static zend_result set_proc_descriptor_from_resource(zval *resource, descriptorspec_item *desc, int nindex) { /* Should be a stream - try and dup the descriptor */ php_stream *stream = (php_stream*)zend_fetch_resource(Z_RES_P(resource), "stream", @@ -932,7 +934,7 @@ static int set_proc_descriptor_from_resource(zval *resource, descriptorspec_item } php_socket_t fd; - int status = php_stream_cast(stream, PHP_STREAM_AS_FD, (void **)&fd, REPORT_ERRORS); + zend_result status = php_stream_cast(stream, PHP_STREAM_AS_FD, (void **)&fd, REPORT_ERRORS); if (status == FAILURE) { return FAILURE; } @@ -942,15 +944,11 @@ static int set_proc_descriptor_from_resource(zval *resource, descriptorspec_item #else php_file_descriptor_t fd_t = fd; #endif - if (dup_proc_descriptor(fd_t, &desc->childend, nindex) == FAILURE) { - return FAILURE; - } - - return SUCCESS; + return dup_proc_descriptor(fd_t, &desc->childend, nindex); } #ifndef PHP_WIN32 -static int close_parentends_of_pipes(descriptorspec_item *descriptors, int ndesc) +static zend_result 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 @@ -1000,12 +998,12 @@ PHP_FUNCTION(proc_open) { zend_string *command_str; HashTable *command_ht; - zval *descriptorspec, *pipes; /* Mandatory arguments */ + HashTable *descriptorspec; /* Mandatory argument */ + zval *pipes; /* Mandatory argument */ 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; @@ -1023,11 +1021,11 @@ PHP_FUNCTION(proc_open) char cur_cwd[MAXPATHLEN]; wchar_t *cmdw = NULL, *cwdw = NULL, *envpw = NULL; size_t cmdw_len; - int suppress_errors = 0; - int bypass_shell = 0; - int blocking_pipes = 0; - int create_process_group = 0; - int create_new_console = 0; + bool suppress_errors = 0; + bool bypass_shell = 0; + bool blocking_pipes = 0; + bool create_process_group = 0; + bool create_new_console = 0; #else char **argv = NULL; #endif @@ -1037,7 +1035,7 @@ PHP_FUNCTION(proc_open) ZEND_PARSE_PARAMETERS_START(3, 6) Z_PARAM_ARRAY_HT_OR_STR(command_ht, command_str) - Z_PARAM_ARRAY(descriptorspec) + Z_PARAM_ARRAY_HT(descriptorspec) Z_PARAM_ZVAL(pipes) Z_PARAM_OPTIONAL Z_PARAM_STRING_OR_NULL(cwd, cwd_len) @@ -1057,28 +1055,29 @@ 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(command_ht); - if (!command) { + command_str = create_win_command_from_args(command_ht); +#else + command_str = get_command_from_array(command_ht, &argv, num_elems); +#endif + + if (!command_str) { +#ifndef PHP_WIN32 + efree_argv(argv); +#endif RETURN_FALSE; } -#else - command = get_command_from_array(command_ht, &argv, num_elems); - if (command == NULL) { - goto exit_fail; - } -#endif } else { - command = estrdup(ZSTR_VAL(command_str)); + zend_string_addref(command_str); } #ifdef PHP_WIN32 if (other_options) { - suppress_errors = get_option(other_options, "suppress_errors"); + suppress_errors = get_option(other_options, "suppress_errors", strlen("suppress_errors")); /* 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"); - create_new_console = get_option(other_options, "create_new_console"); + bypass_shell = bypass_shell || get_option(other_options, "bypass_shell", strlen("bypass_shell")); + blocking_pipes = get_option(other_options, "blocking_pipes", strlen("blocking_pipes")); + create_process_group = get_option(other_options, "create_process_group", strlen("create_process_group")); + create_new_console = get_option(other_options, "create_new_console", strlen("create_new_console")); } #endif @@ -1089,7 +1088,7 @@ PHP_FUNCTION(proc_open) descriptors = alloc_descriptor_array(descriptorspec); /* Walk the descriptor spec and set up files/pipes */ - ZEND_HASH_FOREACH_KEY_VAL(Z_ARRVAL_P(descriptorspec), nindex, str_index, descitem) { + ZEND_HASH_FOREACH_KEY_VAL(descriptorspec, nindex, str_index, descitem) { if (str_index) { zend_argument_value_error(2, "must be an integer indexed array"); goto exit_fail; @@ -1155,7 +1154,7 @@ PHP_FUNCTION(proc_open) } } - cmdw = php_win32_cp_conv_any_to_w(command, strlen(command), &cmdw_len); + cmdw = php_win32_cp_conv_any_to_w(ZSTR_VAL(command_str), ZSTR_LEN(command_str), &cmdw_len); if (!cmdw) { php_error_docref(NULL, E_WARNING, "Command conversion failed"); goto exit_fail; @@ -1206,12 +1205,12 @@ PHP_FUNCTION(proc_open) if (env.envarray) { environ = env.envarray; } - execvp(command, argv); + execvp(ZSTR_VAL(command_str), argv); } else { if (env.envarray) { - execle("/bin/sh", "sh", "-c", command, NULL, env.envarray); + execle("/bin/sh", "sh", "-c", ZSTR_VAL(command_str), NULL, env.envarray); } else { - execl("/bin/sh", "sh", "-c", command, NULL); + execl("/bin/sh", "sh", "-c", ZSTR_VAL(command_str), NULL); } } @@ -1237,7 +1236,7 @@ PHP_FUNCTION(proc_open) } proc = (php_process_handle*) emalloc(sizeof(php_process_handle)); - proc->command = command; + proc->command = zend_string_copy(command_str); proc->pipes = emalloc(sizeof(zend_resource *) * ndesc); proc->npipes = ndesc; proc->child = child; @@ -1308,12 +1307,10 @@ PHP_FUNCTION(proc_open) } else { exit_fail: _php_free_envp(env); - if (command) { - efree(command); - } RETVAL_FALSE; } + zend_string_release_ex(command_str, false); #ifdef PHP_WIN32 free(cwdw); free(cmdw); diff --git a/ext/standard/proc_open.h b/ext/standard/proc_open.h index 6912f28b63f..d2e75ca1f3a 100644 --- a/ext/standard/proc_open.h +++ b/ext/standard/proc_open.h @@ -41,6 +41,6 @@ typedef struct _php_process_handle { #endif int npipes; zend_resource **pipes; - char *command; + zend_string *command; php_process_env env; } php_process_handle;