From a22fa389a9014a44873f79a188eeec145b2d0ca5 Mon Sep 17 00:00:00 2001 From: Calvin Buckley Date: Fri, 22 Sep 2023 15:04:40 -0300 Subject: [PATCH 1/8] Aggressively use actual func params in verror PHP errors used to not show parameter info consistently. Make it so that it uses a backtrace to get function info, similar to how exceptions work. This makes the docref error functions' parameter argument mostly vestigal, being used only if allocation fails basically. Several tests will fail from the fact we include function params. One annoyance is that _build_trace_args truncates strings according to exception_string_param_max_len. See GH-12048 --- Zend/zend_exceptions.c | 33 +++++++++++++++++++++++++++++++++ Zend/zend_exceptions.h | 1 + main/main.c | 28 +++++++++++++++++++++++++++- 3 files changed, 61 insertions(+), 1 deletion(-) diff --git a/Zend/zend_exceptions.c b/Zend/zend_exceptions.c index c66509c50eaed..89331d2430d0e 100644 --- a/Zend/zend_exceptions.c +++ b/Zend/zend_exceptions.c @@ -590,6 +590,39 @@ static void _build_trace_string(smart_str *str, const HashTable *ht, uint32_t nu } /* }}} */ +ZEND_API zend_string *zend_trace_function_args_to_string(HashTable *frame) { + zval *tmp; + smart_str str = {0}; + /* init because ASan will complain */ + smart_str_appends(&str, ""); + + tmp = zend_hash_find_known_hash(frame, ZSTR_KNOWN(ZEND_STR_ARGS)); + if (tmp) { + if (Z_TYPE_P(tmp) == IS_ARRAY) { + size_t last_len = ZSTR_LEN(str.s); + zend_string *name; + zval *arg; + + ZEND_HASH_FOREACH_STR_KEY_VAL(Z_ARRVAL_P(tmp), name, arg) { + if (name) { + smart_str_append(&str, name); + smart_str_appends(&str, ": "); + } + _build_trace_args(arg, &str); + } ZEND_HASH_FOREACH_END(); + + if (last_len != ZSTR_LEN(str.s)) { + ZSTR_LEN(str.s) -= 2; /* remove last ', ' */ + } + } else { + smart_str_appends(&str, "<>"); + } + } + + smart_str_0(&str); + return str.s ? str.s : ZSTR_EMPTY_ALLOC(); +} + ZEND_API zend_string *zend_trace_to_string(const HashTable *trace, bool include_main) { zend_ulong index; zval *frame; diff --git a/Zend/zend_exceptions.h b/Zend/zend_exceptions.h index d0138021d1ea3..31d118a62f2af 100644 --- a/Zend/zend_exceptions.h +++ b/Zend/zend_exceptions.h @@ -72,6 +72,7 @@ extern ZEND_API void (*zend_throw_exception_hook)(zend_object *ex); /* show an exception using zend_error(severity,...), severity should be E_ERROR */ ZEND_API ZEND_COLD zend_result zend_exception_error(zend_object *exception, int severity); ZEND_NORETURN void zend_exception_uncaught_error(const char *prefix, ...) ZEND_ATTRIBUTE_FORMAT(printf, 1, 2); +ZEND_API zend_string *zend_trace_function_args_to_string(HashTable *frame); ZEND_API zend_string *zend_trace_to_string(const HashTable *trace, bool include_main); ZEND_API ZEND_COLD zend_object *zend_create_unwind_exit(void); diff --git a/main/main.c b/main/main.c index eabd0e998736c..b6230fc69faf5 100644 --- a/main/main.c +++ b/main/main.c @@ -59,6 +59,7 @@ #include "ext/standard/flock_compat.h" #endif #include "php_syslog.h" +#include "Zend/zend_builtin_functions.h" #include "Zend/zend_exceptions.h" #if PHP_SIGCHILD @@ -985,6 +986,26 @@ static zend_string *escape_html(const char *buffer, size_t buffer_len) { return result; } +static zend_string *build_dynamic_parameters(void) { + zend_string *dynamic_params = NULL; + /* get a backtrace to snarf function args */ + zval backtrace, *first_frame; + zend_fetch_debug_backtrace(&backtrace, /* skip_last */ 0, /* options */ 0, /* limit */ 1); + /* can fail esp if low memory condition */ + if (Z_TYPE(backtrace) != IS_ARRAY) { + return NULL; /* don't need to free */ + } + first_frame = zend_hash_index_find(Z_ARRVAL(backtrace), 0); + if (!first_frame) { + goto free_backtrace; + } + dynamic_params = zend_trace_function_args_to_string(Z_ARRVAL_P(first_frame)); +free_backtrace: + zval_ptr_dtor(&backtrace); + /* free the string after we use it */ + return dynamic_params; +} + /* {{{ php_verror */ /* php_verror is called from php_error_docref functions. * Its purpose is to unify error messages and automatically generate clickable @@ -1067,7 +1088,12 @@ PHPAPI ZEND_COLD void php_verror(const char *docref, const char *params, int typ /* if we still have memory then format the origin */ if (is_function) { - origin_len = (int)spprintf(&origin, 0, "%s%s%s(%s)", class_name, space, function, params); + zend_string *dynamic_params = NULL; + dynamic_params = build_dynamic_parameters(); + origin_len = (int)spprintf(&origin, 0, "%s%s%s(%s)", class_name, space, function, dynamic_params ? ZSTR_VAL(dynamic_params) : params); + if (dynamic_params) { + zend_string_release(dynamic_params); + } } else { origin_len = (int)spprintf(&origin, 0, "%s", function); } From 5281590e06b0ff68cd5aa44f16b4ada790672cd1 Mon Sep 17 00:00:00 2001 From: Calvin Buckley Date: Mon, 25 Sep 2023 16:34:16 -0300 Subject: [PATCH 2/8] share code path for arg list string building --- Zend/zend_exceptions.c | 65 ++++++++++++++++++------------------------ 1 file changed, 27 insertions(+), 38 deletions(-) diff --git a/Zend/zend_exceptions.c b/Zend/zend_exceptions.c index 89331d2430d0e..4a6f58cb4fb49 100644 --- a/Zend/zend_exceptions.c +++ b/Zend/zend_exceptions.c @@ -529,6 +529,31 @@ static void _build_trace_args(zval *arg, smart_str *str) /* {{{ */ } /* }}} */ +static void _build_trace_args_list(zval *tmp, smart_str *str) /* {{{ */ +{ + if (EXPECTED(Z_TYPE_P(tmp) == IS_ARRAY)) { + size_t last_len = ZSTR_LEN(str->s); + zend_string *name; + zval *arg; + + ZEND_HASH_FOREACH_STR_KEY_VAL(Z_ARRVAL_P(tmp), name, arg) { + if (name) { + smart_str_append(str, name); + smart_str_appends(str, ": "); + } + _build_trace_args(arg, str); + } ZEND_HASH_FOREACH_END(); + + if (last_len != ZSTR_LEN(str->s)) { + ZSTR_LEN(str->s) -= 2; /* remove last ', ' */ + } + } else { + /* only happens w/ reflection abuse (Zend/tests/bug63762.phpt) */ + zend_error(E_WARNING, "args element is not an array"); + } +} +/* }}} */ + static void _build_trace_string(smart_str *str, const HashTable *ht, uint32_t num) /* {{{ */ { zval *file, *tmp; @@ -566,25 +591,7 @@ static void _build_trace_string(smart_str *str, const HashTable *ht, uint32_t nu smart_str_appendc(str, '('); tmp = zend_hash_find_known_hash(ht, ZSTR_KNOWN(ZEND_STR_ARGS)); if (tmp) { - if (EXPECTED(Z_TYPE_P(tmp) == IS_ARRAY)) { - size_t last_len = ZSTR_LEN(str->s); - zend_string *name; - zval *arg; - - ZEND_HASH_FOREACH_STR_KEY_VAL(Z_ARRVAL_P(tmp), name, arg) { - if (name) { - smart_str_append(str, name); - smart_str_appends(str, ": "); - } - _build_trace_args(arg, str); - } ZEND_HASH_FOREACH_END(); - - if (last_len != ZSTR_LEN(str->s)) { - ZSTR_LEN(str->s) -= 2; /* remove last ', ' */ - } - } else { - zend_error(E_WARNING, "args element is not an array"); - } + _build_trace_args_list(tmp, str); } smart_str_appends(str, ")\n"); } @@ -598,25 +605,7 @@ ZEND_API zend_string *zend_trace_function_args_to_string(HashTable *frame) { tmp = zend_hash_find_known_hash(frame, ZSTR_KNOWN(ZEND_STR_ARGS)); if (tmp) { - if (Z_TYPE_P(tmp) == IS_ARRAY) { - size_t last_len = ZSTR_LEN(str.s); - zend_string *name; - zval *arg; - - ZEND_HASH_FOREACH_STR_KEY_VAL(Z_ARRVAL_P(tmp), name, arg) { - if (name) { - smart_str_append(&str, name); - smart_str_appends(&str, ": "); - } - _build_trace_args(arg, &str); - } ZEND_HASH_FOREACH_END(); - - if (last_len != ZSTR_LEN(str.s)) { - ZSTR_LEN(str.s) -= 2; /* remove last ', ' */ - } - } else { - smart_str_appends(&str, "<>"); - } + _build_trace_args_list(tmp, &str); } smart_str_0(&str); From 6819ad9aa2ddcf413372042894b0c328b722ad0a Mon Sep 17 00:00:00 2001 From: Calvin Buckley Date: Mon, 25 Sep 2023 16:42:35 -0300 Subject: [PATCH 3/8] Make suggested refactors - move build_dynamic_parameters to zend_exceptions, avoid the dependency on the BIF header in main.c - (though i'm not sure if exceptions is best place for this function) - add documentation comments to new API surface from this PR --- Zend/zend_exceptions.c | 24 ++++++++++++++++++++++++ Zend/zend_exceptions.h | 1 + main/main.c | 23 +---------------------- 3 files changed, 26 insertions(+), 22 deletions(-) diff --git a/Zend/zend_exceptions.c b/Zend/zend_exceptions.c index 4a6f58cb4fb49..d28224c913288 100644 --- a/Zend/zend_exceptions.c +++ b/Zend/zend_exceptions.c @@ -597,6 +597,7 @@ static void _build_trace_string(smart_str *str, const HashTable *ht, uint32_t nu } /* }}} */ +/* {{{ Gets the function arguments printed as a string from a backtrace frame. */ ZEND_API zend_string *zend_trace_function_args_to_string(HashTable *frame) { zval *tmp; smart_str str = {0}; @@ -611,6 +612,29 @@ ZEND_API zend_string *zend_trace_function_args_to_string(HashTable *frame) { smart_str_0(&str); return str.s ? str.s : ZSTR_EMPTY_ALLOC(); } +/* }}} */ + +/* {{{ Gets the currently executing function's arguments as a string. Used by php_verror. */ +ZEND_API zend_string *zend_trace_current_function_args_string(void) { + zend_string *dynamic_params = NULL; + /* get a backtrace to snarf function args */ + zval backtrace, *first_frame; + zend_fetch_debug_backtrace(&backtrace, /* skip_last */ 0, /* options */ 0, /* limit */ 1); + /* can fail esp if low memory condition */ + if (Z_TYPE(backtrace) != IS_ARRAY) { + return NULL; /* don't need to free */ + } + first_frame = zend_hash_index_find(Z_ARRVAL(backtrace), 0); + if (!first_frame) { + goto free_backtrace; + } + dynamic_params = zend_trace_function_args_to_string(Z_ARRVAL_P(first_frame)); +free_backtrace: + zval_ptr_dtor(&backtrace); + /* free the string after we use it */ + return dynamic_params; +} +/* }}} */ ZEND_API zend_string *zend_trace_to_string(const HashTable *trace, bool include_main) { zend_ulong index; diff --git a/Zend/zend_exceptions.h b/Zend/zend_exceptions.h index 31d118a62f2af..00f1990d88f4c 100644 --- a/Zend/zend_exceptions.h +++ b/Zend/zend_exceptions.h @@ -73,6 +73,7 @@ extern ZEND_API void (*zend_throw_exception_hook)(zend_object *ex); ZEND_API ZEND_COLD zend_result zend_exception_error(zend_object *exception, int severity); ZEND_NORETURN void zend_exception_uncaught_error(const char *prefix, ...) ZEND_ATTRIBUTE_FORMAT(printf, 1, 2); ZEND_API zend_string *zend_trace_function_args_to_string(HashTable *frame); +ZEND_API zend_string *zend_trace_current_function_args_string(void); ZEND_API zend_string *zend_trace_to_string(const HashTable *trace, bool include_main); ZEND_API ZEND_COLD zend_object *zend_create_unwind_exit(void); diff --git a/main/main.c b/main/main.c index b6230fc69faf5..f19f0d8a8df62 100644 --- a/main/main.c +++ b/main/main.c @@ -59,7 +59,6 @@ #include "ext/standard/flock_compat.h" #endif #include "php_syslog.h" -#include "Zend/zend_builtin_functions.h" #include "Zend/zend_exceptions.h" #if PHP_SIGCHILD @@ -986,26 +985,6 @@ static zend_string *escape_html(const char *buffer, size_t buffer_len) { return result; } -static zend_string *build_dynamic_parameters(void) { - zend_string *dynamic_params = NULL; - /* get a backtrace to snarf function args */ - zval backtrace, *first_frame; - zend_fetch_debug_backtrace(&backtrace, /* skip_last */ 0, /* options */ 0, /* limit */ 1); - /* can fail esp if low memory condition */ - if (Z_TYPE(backtrace) != IS_ARRAY) { - return NULL; /* don't need to free */ - } - first_frame = zend_hash_index_find(Z_ARRVAL(backtrace), 0); - if (!first_frame) { - goto free_backtrace; - } - dynamic_params = zend_trace_function_args_to_string(Z_ARRVAL_P(first_frame)); -free_backtrace: - zval_ptr_dtor(&backtrace); - /* free the string after we use it */ - return dynamic_params; -} - /* {{{ php_verror */ /* php_verror is called from php_error_docref functions. * Its purpose is to unify error messages and automatically generate clickable @@ -1089,7 +1068,7 @@ PHPAPI ZEND_COLD void php_verror(const char *docref, const char *params, int typ /* if we still have memory then format the origin */ if (is_function) { zend_string *dynamic_params = NULL; - dynamic_params = build_dynamic_parameters(); + dynamic_params = zend_trace_current_function_args_string(); origin_len = (int)spprintf(&origin, 0, "%s%s%s(%s)", class_name, space, function, dynamic_params ? ZSTR_VAL(dynamic_params) : params); if (dynamic_params) { zend_string_release(dynamic_params); From df4e2dde5a2275adba995d7de7f334562d3cc105 Mon Sep 17 00:00:00 2001 From: Calvin Buckley Date: Wed, 27 Sep 2023 13:45:50 -0300 Subject: [PATCH 4/8] Change bless to clean up absolute paths With the change to php_verror, it now prints the arguments to the function. The strings in an argument can have absolute paths, and containing user-specific things like a home directory isn't very good for unit tests. Try to cut that out as much as possible. --- scripts/dev/bless_tests.php | 3 +++ 1 file changed, 3 insertions(+) diff --git a/scripts/dev/bless_tests.php b/scripts/dev/bless_tests.php index 03927cfd0055c..3353aacb0213f 100755 --- a/scripts/dev/bless_tests.php +++ b/scripts/dev/bless_tests.php @@ -74,6 +74,9 @@ function normalizeOutput(string $out): string { 'Resource ID#%d used as offset, casting to integer (%d)', $out); $out = preg_replace('/string\(\d+\) "([^"]*%d)/', 'string(%d) "$1', $out); + // Inside of strings, replace absolute paths that have been truncated with + // any string. These tend to contain homedirs with usernames, not good. + $out = preg_replace("/'\\/.*\.\\.\\.'/", "'%s'", $out); $out = str_replace("\0", '%0', $out); return $out; } From e9b1e352f04f139c039480ca1502215a68c6651b Mon Sep 17 00:00:00 2001 From: Calvin Buckley Date: Wed, 27 Sep 2023 17:31:51 -0300 Subject: [PATCH 5/8] Handle file:// URIs too --- scripts/dev/bless_tests.php | 1 + 1 file changed, 1 insertion(+) diff --git a/scripts/dev/bless_tests.php b/scripts/dev/bless_tests.php index 3353aacb0213f..5610d7499fcee 100755 --- a/scripts/dev/bless_tests.php +++ b/scripts/dev/bless_tests.php @@ -77,6 +77,7 @@ function normalizeOutput(string $out): string { // Inside of strings, replace absolute paths that have been truncated with // any string. These tend to contain homedirs with usernames, not good. $out = preg_replace("/'\\/.*\.\\.\\.'/", "'%s'", $out); + $out = preg_replace("/'file:\/\\/.*\.\\.\\.'/", "'%s'", $out); $out = str_replace("\0", '%0', $out); return $out; } From 985f48f00753a21da846d87c7cd28bf0cb2e59ac Mon Sep 17 00:00:00 2001 From: Calvin Buckley Date: Thu, 12 Dec 2024 14:17:44 -0400 Subject: [PATCH 6/8] Make this const to match refactors since rebase --- Zend/zend_exceptions.c | 2 +- Zend/zend_exceptions.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/Zend/zend_exceptions.c b/Zend/zend_exceptions.c index d28224c913288..e922faf330210 100644 --- a/Zend/zend_exceptions.c +++ b/Zend/zend_exceptions.c @@ -598,7 +598,7 @@ static void _build_trace_string(smart_str *str, const HashTable *ht, uint32_t nu /* }}} */ /* {{{ Gets the function arguments printed as a string from a backtrace frame. */ -ZEND_API zend_string *zend_trace_function_args_to_string(HashTable *frame) { +ZEND_API zend_string *zend_trace_function_args_to_string(const HashTable *frame) { zval *tmp; smart_str str = {0}; /* init because ASan will complain */ diff --git a/Zend/zend_exceptions.h b/Zend/zend_exceptions.h index 00f1990d88f4c..59b188863d7f8 100644 --- a/Zend/zend_exceptions.h +++ b/Zend/zend_exceptions.h @@ -72,7 +72,7 @@ extern ZEND_API void (*zend_throw_exception_hook)(zend_object *ex); /* show an exception using zend_error(severity,...), severity should be E_ERROR */ ZEND_API ZEND_COLD zend_result zend_exception_error(zend_object *exception, int severity); ZEND_NORETURN void zend_exception_uncaught_error(const char *prefix, ...) ZEND_ATTRIBUTE_FORMAT(printf, 1, 2); -ZEND_API zend_string *zend_trace_function_args_to_string(HashTable *frame); +ZEND_API zend_string *zend_trace_function_args_to_string(const HashTable *frame); ZEND_API zend_string *zend_trace_current_function_args_string(void); ZEND_API zend_string *zend_trace_to_string(const HashTable *trace, bool include_main); From 44b8dcde6eec2942d4a566509bddafb91b3f5775 Mon Sep 17 00:00:00 2001 From: Calvin Buckley Date: Thu, 12 Dec 2024 14:26:46 -0400 Subject: [PATCH 7/8] Gate new error func arg display behind display_error_function_args This is a useful feature, but enabling it by default requires rewriting every PHPT file's output section. Since that would be a hellish diff to make and to review, I think the best option is unfortunately, another INI option. We can enable this for prod/dev recommended INIs, but make sure it's disabled for the test runner. This takes some inspiration from the discussion in GH-17056, which has similar problems to this PR. --- main/main.c | 9 ++++++++- main/php_globals.h | 1 + 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/main/main.c b/main/main.c index f19f0d8a8df62..04bce9db738ab 100644 --- a/main/main.c +++ b/main/main.c @@ -738,6 +738,7 @@ PHP_INI_BEGIN() STD_PHP_INI_ENTRY_EX("display_errors", "1", PHP_INI_ALL, OnUpdateDisplayErrors, display_errors, php_core_globals, core_globals, display_errors_mode) STD_PHP_INI_BOOLEAN("display_startup_errors", "1", PHP_INI_ALL, OnUpdateBool, display_startup_errors, php_core_globals, core_globals) + STD_PHP_INI_BOOLEAN("display_error_function_args", "0", PHP_INI_ALL, OnUpdateBool, display_error_function_args, php_core_globals, core_globals) STD_PHP_INI_BOOLEAN("enable_dl", "1", PHP_INI_SYSTEM, OnUpdateBool, enable_dl, php_core_globals, core_globals) STD_PHP_INI_BOOLEAN("expose_php", "1", PHP_INI_SYSTEM, OnUpdateBool, expose_php, php_core_globals, core_globals) STD_PHP_INI_ENTRY("docref_root", "", PHP_INI_ALL, OnUpdateString, docref_root, php_core_globals, core_globals) @@ -1068,7 +1069,13 @@ PHPAPI ZEND_COLD void php_verror(const char *docref, const char *params, int typ /* if we still have memory then format the origin */ if (is_function) { zend_string *dynamic_params = NULL; - dynamic_params = zend_trace_current_function_args_string(); + /* + * By default, this is disabled because it requires rewriting + * almost all PHPT files. + */ + if (PG(display_error_function_args)) { + dynamic_params = zend_trace_current_function_args_string(); + } origin_len = (int)spprintf(&origin, 0, "%s%s%s(%s)", class_name, space, function, dynamic_params ? ZSTR_VAL(dynamic_params) : params); if (dynamic_params) { zend_string_release(dynamic_params); diff --git a/main/php_globals.h b/main/php_globals.h index b2f2696c2db2c..1490e579a09d0 100644 --- a/main/php_globals.h +++ b/main/php_globals.h @@ -61,6 +61,7 @@ struct _php_core_globals { uint8_t display_errors; bool display_startup_errors; + bool display_error_function_args; bool log_errors; bool ignore_repeated_errors; bool ignore_repeated_source; From 90ec7b509f29aa0c2f7d8b4bc8ed802e4ed19ce3 Mon Sep 17 00:00:00 2001 From: Calvin Buckley Date: Thu, 12 Dec 2024 14:59:31 -0400 Subject: [PATCH 8/8] Add this to the hardcoded options for test runner --- run-tests.php | 1 + 1 file changed, 1 insertion(+) diff --git a/run-tests.php b/run-tests.php index 9dffecca8d2c0..77179b051d837 100755 --- a/run-tests.php +++ b/run-tests.php @@ -274,6 +274,7 @@ function main(): void 'error_reporting=' . E_ALL, 'display_errors=1', 'display_startup_errors=1', + 'display_error_function_args=0', 'log_errors=0', 'html_errors=0', 'track_errors=0',