From b90387080a81952b330af225d4dc2bcbde14892d Mon Sep 17 00:00:00 2001 From: jvoisin Date: Tue, 9 Oct 2018 12:14:35 +0000 Subject: Don't check the return values of functions that might not return (#255) This is due to our modifications to the logging system--- src/sp_disabled_functions.c | 100 +++++++++++++++++++++----------------------- src/sp_disabled_functions.h | 8 +--- src/sp_execute.c | 41 +++++++----------- 3 files changed, 64 insertions(+), 85 deletions(-) diff --git a/src/sp_disabled_functions.c b/src/sp_disabled_functions.c index b5cbe14..c483612 100644 --- a/src/sp_disabled_functions.c +++ b/src/sp_disabled_functions.c @@ -5,6 +5,18 @@ ZEND_DECLARE_MODULE_GLOBALS(snuffleupagus) +static void should_disable(zend_execute_data* execute_data, + const char* complete_function_path, + const zend_string* builtin_param, + const char* builtin_param_name, + const sp_list_node* config, + const zend_string* current_filename); + +static void should_drop_on_ret(const zval* return_value, + const sp_list_node* config, + const char* complete_function_path, + zend_execute_data* execute_data); + char* get_complete_function_path(zend_execute_data const* const execute_data) { if (zend_is_executing() && !EG(current_execute_data)->func) { return NULL; // LCOV_EXCL_LINE @@ -249,17 +261,16 @@ static bool check_is_builtin_name( return false; } -bool should_disable_ht(zend_execute_data* execute_data, +void should_disable_ht(zend_execute_data* execute_data, const char* function_name, const zend_string* builtin_param, const char* builtin_param_name, const sp_list_node* config, const HashTable* ht) { const sp_list_node* ht_entry = NULL; - bool ret = false; zend_string* current_filename; if (!execute_data) { - return false; // LCOV_EXCL_LINE + return; // LCOV_EXCL_LINE } if (UNEXPECTED(builtin_param && !strcmp(function_name, "eval"))) { @@ -271,24 +282,23 @@ bool should_disable_ht(zend_execute_data* execute_data, ht_entry = zend_hash_str_find_ptr(ht, function_name, strlen(function_name)); - if (ht_entry && - should_disable(execute_data, function_name, builtin_param, - builtin_param_name, ht_entry, current_filename)) { - ret = true; + if (ht_entry) { + should_disable(execute_data, function_name, builtin_param, + builtin_param_name, ht_entry, current_filename); } else if (config && config->data) { - ret = should_disable(execute_data, function_name, builtin_param, - builtin_param_name, config, current_filename); + should_disable(execute_data, function_name, builtin_param, + builtin_param_name, config, current_filename); } efree(current_filename); - return ret; } -bool should_disable(zend_execute_data* execute_data, - const char* complete_function_path, - const zend_string* builtin_param, - const char* builtin_param_name, const sp_list_node* config, - const zend_string* current_filename) { +static void should_disable(zend_execute_data* execute_data, + const char* complete_function_path, + const zend_string* builtin_param, + const char* builtin_param_name, + const sp_list_node* config, + const zend_string* current_filename) { char current_file_hash[SHA256_SIZE * 2 + 1] = {0}; while (config) { @@ -381,7 +391,7 @@ bool should_disable(zend_execute_data* execute_data, /* Everything matched.*/ if (true == config_node->allow) { - goto allow; + return; } if (config_node->functions_list) { @@ -391,43 +401,34 @@ bool should_disable(zend_execute_data* execute_data, sp_log_disable(complete_function_path, arg_name, arg_value_str, config_node); } - if (true == config_node->simulation) { - goto next; - } else { // We've got a match, the function won't be executed - return true; - } + next: config = config->next; } -allow: - return false; } -bool should_drop_on_ret_ht(const zval* return_value, const char* function_name, +void should_drop_on_ret_ht(const zval* return_value, const char* function_name, const sp_list_node* config, const HashTable* ht, zend_execute_data* execute_data) { const sp_list_node* ht_entry = NULL; - bool ret = false; if (!function_name) { - return ret; + return; } ht_entry = zend_hash_str_find_ptr(ht, function_name, strlen(function_name)); - if (ht_entry && - should_drop_on_ret(return_value, ht_entry, function_name, execute_data)) { - ret = true; + if (ht_entry) { + should_drop_on_ret(return_value, ht_entry, function_name, execute_data); } else if (config && config->data) { - ret = should_drop_on_ret(return_value, config, function_name, execute_data); + should_drop_on_ret(return_value, config, function_name, execute_data); } - - return ret; } -bool should_drop_on_ret(const zval* return_value, const sp_list_node* config, - const char* complete_function_path, - zend_execute_data* execute_data) { +static void should_drop_on_ret(const zval* return_value, + const sp_list_node* config, + const char* complete_function_path, + zend_execute_data* execute_data) { const char* current_filename = zend_get_executed_filename(TSRMLS_C); char current_file_hash[SHA256_SIZE * 2 + 1] = {0}; bool match_type = false, match_value = false; @@ -487,41 +488,34 @@ bool should_drop_on_ret(const zval* return_value, const sp_list_node* config, if (true == match_type || true == match_value) { if (true == config_node->allow) { - return false; + return; } sp_log_disable_ret(complete_function_path, ret_value_str, config_node); - if (false == config_node->simulation) { - return true; - } } next: config = config->next; } - return false; } ZEND_FUNCTION(check_disabled_function) { zif_handler orig_handler; const char* current_function_name = get_active_function_name(TSRMLS_C); - if (true == should_disable_ht( - execute_data, current_function_name, NULL, NULL, - SNUFFLEUPAGUS_G(config) - .config_disabled_functions_reg->disabled_functions, - SNUFFLEUPAGUS_G(config).config_disabled_functions_hooked)) { - } + should_disable_ht( + execute_data, current_function_name, NULL, NULL, + SNUFFLEUPAGUS_G(config).config_disabled_functions_reg->disabled_functions, + SNUFFLEUPAGUS_G(config).config_disabled_functions_hooked); orig_handler = zend_hash_str_find_ptr( SNUFFLEUPAGUS_G(disabled_functions_hook), current_function_name, strlen(current_function_name)); orig_handler(INTERNAL_FUNCTION_PARAM_PASSTHRU); - if (true == should_drop_on_ret_ht( - return_value, current_function_name, - SNUFFLEUPAGUS_G(config) - .config_disabled_functions_reg_ret->disabled_functions, - SNUFFLEUPAGUS_G(config).config_disabled_functions_ret_hooked, - execute_data)) { - } + should_drop_on_ret_ht( + return_value, current_function_name, + SNUFFLEUPAGUS_G(config) + .config_disabled_functions_reg_ret->disabled_functions, + SNUFFLEUPAGUS_G(config).config_disabled_functions_ret_hooked, + execute_data); } static int hook_functions_regexp(const sp_list_node* config) { diff --git a/src/sp_disabled_functions.h b/src/sp_disabled_functions.h index b7901dd..3999aec 100644 --- a/src/sp_disabled_functions.h +++ b/src/sp_disabled_functions.h @@ -5,15 +5,11 @@ extern zend_write_func_t zend_write_default; int hook_disabled_functions(); int hook_echo(const char *, size_t); -bool should_disable(zend_execute_data *, const char *, const zend_string *, - const char *, const sp_list_node *, const zend_string *); -bool should_disable_ht(zend_execute_data *, const char *, const zend_string *, +void should_disable_ht(zend_execute_data *, const char *, const zend_string *, const char *, const sp_list_node *, const HashTable *); -bool should_drop_on_ret_ht(const zval *, const char *, +void should_drop_on_ret_ht(const zval *, const char *, const sp_list_node *config, const HashTable *, zend_execute_data *); -bool should_drop_on_ret(const zval *, const sp_list_node *config, const char *, - zend_execute_data *); char *get_complete_function_path(zend_execute_data const *const); #endif /* __SP_DISABLE_FUNCTIONS_H */ diff --git a/src/sp_execute.c b/src/sp_execute.c index cc2d9b7..544d8c2 100644 --- a/src/sp_execute.c +++ b/src/sp_execute.c @@ -49,13 +49,10 @@ inline static void is_builtin_matching( return; } - if (true == - should_disable_ht(EG(current_execute_data), function_name, param_value, - param_name, - SNUFFLEUPAGUS_G(config) - .config_disabled_functions_reg->disabled_functions, - ht)) { - } + should_disable_ht( + EG(current_execute_data), function_name, param_value, param_name, + SNUFFLEUPAGUS_G(config).config_disabled_functions_reg->disabled_functions, + ht); } static void ZEND_HOT @@ -168,11 +165,9 @@ static void sp_execute_ex(zend_execute_data *execute_data) { !execute_data->prev_execute_data->func || !ZEND_USER_CODE(execute_data->prev_execute_data->func->type) || !execute_data->prev_execute_data->opline) { - if (UNEXPECTED(true == should_disable_ht(execute_data, function_name, - NULL, NULL, - config_disabled_functions_reg, - config_disabled_functions))) { - } + should_disable_ht(execute_data, function_name, NULL, NULL, + config_disabled_functions_reg, + config_disabled_functions); } else if ((execute_data->prev_execute_data->opline->opcode == ZEND_DO_FCALL || execute_data->prev_execute_data->opline->opcode == @@ -181,11 +176,9 @@ static void sp_execute_ex(zend_execute_data *execute_data) { ZEND_DO_ICALL || execute_data->prev_execute_data->opline->opcode == ZEND_DO_FCALL_BY_NAME)) { - if (UNEXPECTED(true == should_disable_ht(execute_data, function_name, - NULL, NULL, - config_disabled_functions_reg, - config_disabled_functions))) { - } + should_disable_ht(execute_data, function_name, NULL, NULL, + config_disabled_functions_reg, + config_disabled_functions); } // When a function's return value isn't used, php doesn't store it in the @@ -198,15 +191,11 @@ static void sp_execute_ex(zend_execute_data *execute_data) { orig_execute_ex(execute_data); - if (UNEXPECTED( - true == - should_drop_on_ret_ht( - EX(return_value), function_name, - SNUFFLEUPAGUS_G(config) - .config_disabled_functions_reg_ret->disabled_functions, - SNUFFLEUPAGUS_G(config).config_disabled_functions_ret, - execute_data))) { - } + should_drop_on_ret_ht( + EX(return_value), function_name, + SNUFFLEUPAGUS_G(config) + .config_disabled_functions_reg_ret->disabled_functions, + SNUFFLEUPAGUS_G(config).config_disabled_functions_ret, execute_data); efree(function_name); if (EX(return_value) == &ret_val) { -- cgit v1.3