From ac03a9aac018a3d310c48bd5d3ea631eef9b7c26 Mon Sep 17 00:00:00 2001 From: Dennis Snell Date: Thu, 28 Apr 2022 11:07:03 -0500 Subject: [PATCH 1/8] Add ini_bytes to translate ini directive byte shorthand to a count With WordPress we have seen a variety of bugs that stem from the fact that we don't reliably parse the memory size values from php.ini directives. We want to know those values in order to communicate to site users what the file upload limits are. In this patch I'm adding a new function `ini_bytes( $shorthand )` to return whatever PHP evaluates that value to be internally, thus exposing the actual byte size to the outside world. This eliminates the need for WordPress and other projects to port the config value parser since they could directly call it. Additionally it will reliably reproduce any quirks due to the platform build and overflow behaviors when handling the `g`, `m`, and `k` suffixes on `post_max_size` and `upload_max_filesize` (and others). This function is a shallow wrapper around `zend_atoi()` and could become out of sync with the actual ini directive parsing. I considered a variation of this function which would take a directive's name and return the stored internal value directly, keeping it in sync. ```php switch ( param_name ) { case 'post_max_size': RETVAL_LONG(SG(post_max_size)); } ``` After looking briefly at this option I dismissed it for being a bit too coupled to those options, too demanding to add all the possible config options to a big switch statement, and too complicated for returning all of the possible return types stored in the SAPI globals. This is my first attempt at patching PHP and I don't know what I'm doing :) I'm not thrilled at the function name but I needed something. I'm not aware of what I need to write for testing, documentation, etc... but I'm happy to add whatever is required. The expecation is to use it in situations roughly like this: ```php function max_upload_size() { $u_bytes = ini_bytes( ini_get( 'upload_max_filesize' ) || 0 ); $p_bytes = ini_bytes( ini_get( 'post_max_size' ) || 0 ); return min( $u_bytes, $p_bytes ); } ``` This patch also makes it easier to perform mathematic comparisons and operations on the config values, which again previously required duplicating the parsing outside of PHP and likely will fail for a variety of config values. ```php function validate_upload_size() { $u_bytes = ini_bytes( ini_get( 'upload_max_filesize' ) || 0 ); $p_bytes = ini_bytes( ini_get( 'post_max_size' ) || 0 ); return $u_bytes <= $p_bytes ? [ true ] : [ false, 'upload_max_filesize must be smaller than post_max_size' ]; } ``` --- ext/standard/basic_functions.c | 14 ++++++++++++++ ext/standard/basic_functions.stub.php | 2 ++ ext/standard/basic_functions_arginfo.h | 6 ++++++ 3 files changed, 22 insertions(+) diff --git a/ext/standard/basic_functions.c b/ext/standard/basic_functions.c index 5123174569fa3..75966f2f7e676 100755 --- a/ext/standard/basic_functions.c +++ b/ext/standard/basic_functions.c @@ -1971,6 +1971,20 @@ PHP_FUNCTION(highlight_string) } /* }}} */ +/* {{{ Get a byte size from the ini byte size shorthand */ +PHP_FUNCTION(ini_bytes) +{ + char *shorthand; + size_t shorthand_len; + + ZEND_PARSE_PARAMETERS_START(1, 1) + Z_PARAM_STRING(shorthand, shorthand_len) + ZEND_PARSE_PARAMETERS_END(); + + RETVAL_LONG(zend_atol(shorthand, shorthand_len)); +} +/* }}} */ + /* {{{ Get a configuration option */ PHP_FUNCTION(ini_get) { diff --git a/ext/standard/basic_functions.stub.php b/ext/standard/basic_functions.stub.php index 5020fa4d2897f..277e6a2540850 100755 --- a/ext/standard/basic_functions.stub.php +++ b/ext/standard/basic_functions.stub.php @@ -488,6 +488,8 @@ function ini_alter(string $option, string|int|float|bool|null $value): string|fa function ini_restore(string $option): void {} +function ini_bytes(string $option): int {} + /** @refcount 1 */ function set_include_path(string $include_path): string|false {} diff --git a/ext/standard/basic_functions_arginfo.h b/ext/standard/basic_functions_arginfo.h index 0b6881a99bd17..40d86630a0523 100644 --- a/ext/standard/basic_functions_arginfo.h +++ b/ext/standard/basic_functions_arginfo.h @@ -489,6 +489,10 @@ ZEND_BEGIN_ARG_WITH_RETURN_TYPE_MASK_EX(arginfo_ini_get, 0, 1, MAY_BE_STRING|MAY ZEND_ARG_TYPE_INFO(0, option, IS_STRING, 0) ZEND_END_ARG_INFO() +ZEND_BEGIN_ARG_WITH_RETURN_TYPE_MASK_EX(arginfo_ini_bytes, 0, 1, MAY_BE_LONG) + ZEND_ARG_TYPE_INFO(0, option, IS_STRING, 0) +ZEND_END_ARG_INFO() + ZEND_BEGIN_ARG_WITH_RETURN_TYPE_MASK_EX(arginfo_ini_get_all, 0, 0, MAY_BE_ARRAY|MAY_BE_FALSE) ZEND_ARG_TYPE_INFO_WITH_DEFAULT_VALUE(0, extension, IS_STRING, 1, "null") ZEND_ARG_TYPE_INFO_WITH_DEFAULT_VALUE(0, details, _IS_BOOL, 0, "true") @@ -2359,6 +2363,7 @@ ZEND_FUNCTION(highlight_file); ZEND_FUNCTION(php_strip_whitespace); ZEND_FUNCTION(highlight_string); ZEND_FUNCTION(ini_get); +ZEND_FUNCTION(ini_bytes); ZEND_FUNCTION(ini_get_all); ZEND_FUNCTION(ini_set); ZEND_FUNCTION(ini_restore); @@ -2990,6 +2995,7 @@ static const zend_function_entry ext_functions[] = { ZEND_FE(php_strip_whitespace, arginfo_php_strip_whitespace) ZEND_FE(highlight_string, arginfo_highlight_string) ZEND_FE(ini_get, arginfo_ini_get) + ZEND_FE(ini_bytes, arginfo_ini_bytes) ZEND_FE(ini_get_all, arginfo_ini_get_all) ZEND_FE(ini_set, arginfo_ini_set) ZEND_FALIAS(ini_alter, ini_set, arginfo_ini_alter) From b7e3e5f033c5d3602d5d9c2d7486078ebcb6453d Mon Sep 17 00:00:00 2001 From: Dennis Snell Date: Wed, 4 May 2022 12:52:46 -0700 Subject: [PATCH 2/8] Rename parameter to "shorthand" --- ext/standard/basic_functions.stub.php | 2 +- ext/standard/basic_functions_arginfo.h | 4 ++++ 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/ext/standard/basic_functions.stub.php b/ext/standard/basic_functions.stub.php index 277e6a2540850..1ecba8d1e4291 100755 --- a/ext/standard/basic_functions.stub.php +++ b/ext/standard/basic_functions.stub.php @@ -488,7 +488,7 @@ function ini_alter(string $option, string|int|float|bool|null $value): string|fa function ini_restore(string $option): void {} -function ini_bytes(string $option): int {} +function ini_bytes(string $shorthand): int {} /** @refcount 1 */ function set_include_path(string $include_path): string|false {} diff --git a/ext/standard/basic_functions_arginfo.h b/ext/standard/basic_functions_arginfo.h index 40d86630a0523..f2ee98e45134d 100644 --- a/ext/standard/basic_functions_arginfo.h +++ b/ext/standard/basic_functions_arginfo.h @@ -509,6 +509,10 @@ ZEND_BEGIN_ARG_WITH_RETURN_TYPE_INFO_EX(arginfo_ini_restore, 0, 1, IS_VOID, 0) ZEND_ARG_TYPE_INFO(0, option, IS_STRING, 0) ZEND_END_ARG_INFO() +ZEND_BEGIN_ARG_WITH_RETURN_TYPE_INFO_EX(arginfo_ini_bytes, 0, 1, IS_LONG, 0) + ZEND_ARG_TYPE_INFO(0, shorthand, IS_STRING, 0) +ZEND_END_ARG_INFO() + ZEND_BEGIN_ARG_WITH_RETURN_TYPE_MASK_EX(arginfo_set_include_path, 0, 1, MAY_BE_STRING|MAY_BE_FALSE) ZEND_ARG_TYPE_INFO(0, include_path, IS_STRING, 0) ZEND_END_ARG_INFO() From 55b76e26a01c309766c2a1a51e462c43743e534e Mon Sep 17 00:00:00 2001 From: Dennis Snell Date: Wed, 4 May 2022 13:13:24 -0700 Subject: [PATCH 3/8] Rename function to `ini_parse_quantity( $shorthand )` Of all the naming suggestions so far this one seems to fit best with the purpose of the function. --- ext/standard/basic_functions.c | 4 ++-- ext/standard/basic_functions.stub.php | 2 +- ext/standard/basic_functions_arginfo.h | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/ext/standard/basic_functions.c b/ext/standard/basic_functions.c index 75966f2f7e676..56d731641f33a 100755 --- a/ext/standard/basic_functions.c +++ b/ext/standard/basic_functions.c @@ -1971,8 +1971,8 @@ PHP_FUNCTION(highlight_string) } /* }}} */ -/* {{{ Get a byte size from the ini byte size shorthand */ -PHP_FUNCTION(ini_bytes) +/* {{{ Get interpreted size from the ini shorthand syntax */ +PHP_FUNCTION(ini_parse_quantity) { char *shorthand; size_t shorthand_len; diff --git a/ext/standard/basic_functions.stub.php b/ext/standard/basic_functions.stub.php index 1ecba8d1e4291..07aefbfbef60b 100755 --- a/ext/standard/basic_functions.stub.php +++ b/ext/standard/basic_functions.stub.php @@ -488,7 +488,7 @@ function ini_alter(string $option, string|int|float|bool|null $value): string|fa function ini_restore(string $option): void {} -function ini_bytes(string $shorthand): int {} +function ini_parse_quantity(string $shorthand): int {} /** @refcount 1 */ function set_include_path(string $include_path): string|false {} diff --git a/ext/standard/basic_functions_arginfo.h b/ext/standard/basic_functions_arginfo.h index f2ee98e45134d..0a599a20bb3a4 100644 --- a/ext/standard/basic_functions_arginfo.h +++ b/ext/standard/basic_functions_arginfo.h @@ -509,7 +509,7 @@ ZEND_BEGIN_ARG_WITH_RETURN_TYPE_INFO_EX(arginfo_ini_restore, 0, 1, IS_VOID, 0) ZEND_ARG_TYPE_INFO(0, option, IS_STRING, 0) ZEND_END_ARG_INFO() -ZEND_BEGIN_ARG_WITH_RETURN_TYPE_INFO_EX(arginfo_ini_bytes, 0, 1, IS_LONG, 0) +ZEND_BEGIN_ARG_WITH_RETURN_TYPE_INFO_EX(arginfo_ini_parse_quantity, 0, 1, IS_LONG, 0) ZEND_ARG_TYPE_INFO(0, shorthand, IS_STRING, 0) ZEND_END_ARG_INFO() From 6ee5a87e0634d8f055e19697d5f4e98d1291d0ba Mon Sep 17 00:00:00 2001 From: Dennis Snell Date: Wed, 25 May 2022 17:00:38 -0700 Subject: [PATCH 4/8] Attempt to add basic tests for ini_parse_quantity() --- tests/basic/ini_parse_quantity_basic.phpt | 29 +++++++++++++++++++++++ 1 file changed, 29 insertions(+) create mode 100644 tests/basic/ini_parse_quantity_basic.phpt diff --git a/tests/basic/ini_parse_quantity_basic.phpt b/tests/basic/ini_parse_quantity_basic.phpt new file mode 100644 index 0000000000000..096a94232fd03 --- /dev/null +++ b/tests/basic/ini_parse_quantity_basic.phpt @@ -0,0 +1,29 @@ +--TEST-- +ini_parse_quantity() function - basic test for ini_parse_quantity() +--FILE-- + +--EXPECT-- +0 +0 +1 +1 +1024 +1048576 +1073741824 +14 +0 From b840f146b0d43a50ed2d76f1cee2149ca00c0449 Mon Sep 17 00:00:00 2001 From: Dennis Snell Date: Thu, 9 Jun 2022 14:30:30 +0100 Subject: [PATCH 5/8] Update tests to run successfully --- tests/basic/ini_parse_quantity_basic.phpt | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/tests/basic/ini_parse_quantity_basic.phpt b/tests/basic/ini_parse_quantity_basic.phpt index 096a94232fd03..b6012215a9eac 100644 --- a/tests/basic/ini_parse_quantity_basic.phpt +++ b/tests/basic/ini_parse_quantity_basic.phpt @@ -12,18 +12,21 @@ foreach (array( '1g', '1gb', '14.2mb', + '14.2bm', 'boat' ) as $input) { echo ini_parse_quantity( $input ) . PHP_EOL; } ?> --EXPECT-- -0 +-1 0 1 1 1024 1048576 1073741824 +1 14 +14680064 0 From bf36ac1619c4a972a5e752453f2bf400ab6045f0 Mon Sep 17 00:00:00 2001 From: Dennis Snell Date: Tue, 21 Jun 2022 18:11:37 +0200 Subject: [PATCH 6/8] Use zend_ini_parse_quantity --- ext/standard/basic_functions.c | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/ext/standard/basic_functions.c b/ext/standard/basic_functions.c index 56d731641f33a..f8f6747200d7d 100755 --- a/ext/standard/basic_functions.c +++ b/ext/standard/basic_functions.c @@ -30,6 +30,7 @@ #include "ext/session/php_session.h" #include "zend_exceptions.h" #include "zend_attributes.h" +#include "zend_ini.h" #include "zend_operators.h" #include "ext/standard/php_dns.h" #include "ext/standard/php_uuencode.h" @@ -1974,14 +1975,19 @@ PHP_FUNCTION(highlight_string) /* {{{ Get interpreted size from the ini shorthand syntax */ PHP_FUNCTION(ini_parse_quantity) { - char *shorthand; - size_t shorthand_len; + zend_string *shorthand; + zend_string *errstr; ZEND_PARSE_PARAMETERS_START(1, 1) - Z_PARAM_STRING(shorthand, shorthand_len) + Z_PARAM_STR(shorthand) ZEND_PARSE_PARAMETERS_END(); - RETVAL_LONG(zend_atol(shorthand, shorthand_len)); + RETVAL_LONG(zend_ini_parse_quantity(shorthand, &errstr)); + + if (errstr) { + zend_error(E_WARNING, "%s", ZSTR_VAL(errstr)); + zend_string_release(errstr); + } } /* }}} */ From ceda792e3b134d16510f3c4826d727ddf33b9e40 Mon Sep 17 00:00:00 2001 From: Dennis Snell Date: Tue, 21 Jun 2022 19:07:53 +0200 Subject: [PATCH 7/8] Add tests to test for warnings thrown --- tests/basic/ini_parse_quantity_basic.phpt | 4 ++++ tests/basic/ini_parse_quantity_warnings.phpt | 14 ++++++++++++++ 2 files changed, 18 insertions(+) create mode 100644 tests/basic/ini_parse_quantity_warnings.phpt diff --git a/tests/basic/ini_parse_quantity_basic.phpt b/tests/basic/ini_parse_quantity_basic.phpt index b6012215a9eac..af9a2776f113b 100644 --- a/tests/basic/ini_parse_quantity_basic.phpt +++ b/tests/basic/ini_parse_quantity_basic.phpt @@ -1,9 +1,12 @@ --TEST-- ini_parse_quantity() function - basic test for ini_parse_quantity() +--INI-- +error_reporting = E_ALL ^ E_WARNING --FILE-- --EXPECT-- -1 +-1042 0 1 1 diff --git a/tests/basic/ini_parse_quantity_warnings.phpt b/tests/basic/ini_parse_quantity_warnings.phpt new file mode 100644 index 0000000000000..f18753c853151 --- /dev/null +++ b/tests/basic/ini_parse_quantity_warnings.phpt @@ -0,0 +1,14 @@ +--TEST-- +ini_parse_quantity() function - warns when given inappropriate values +--INI-- +error_reporting = E_ALL +--FILE-- + +--EXPECTF-- +Warning: Invalid quantity "1mb": unknown multipler "b", interpreting as "1" for backwards compatibility in %sini_parse_quantity_warnings.php on line 3 + +Warning: Invalid quantity "256 then skip a few then g", interpreting as "256 g" for backwards compatibility in %sini_parse_quantity_warnings.php on line 4 From 618a8e094e6cfe3271d0c7807ec4bb19d8aea4f6 Mon Sep 17 00:00:00 2001 From: Dennis Snell Date: Mon, 27 Jun 2022 19:21:27 +0200 Subject: [PATCH 8/8] Regenerate arginfo --- ext/standard/basic_functions_arginfo.h | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/ext/standard/basic_functions_arginfo.h b/ext/standard/basic_functions_arginfo.h index 0a599a20bb3a4..5d17552629c2a 100644 --- a/ext/standard/basic_functions_arginfo.h +++ b/ext/standard/basic_functions_arginfo.h @@ -1,5 +1,5 @@ /* This is a generated file, edit the .stub.php file instead. - * Stub hash: 90093c63384f7ba56b1c4073b60219cb82843b98 */ + * Stub hash: 978052ddd734a50694d59f6e92bbf5f1cc946bd2 */ ZEND_BEGIN_ARG_WITH_RETURN_TYPE_INFO_EX(arginfo_set_time_limit, 0, 1, _IS_BOOL, 0) ZEND_ARG_TYPE_INFO(0, seconds, IS_LONG, 0) @@ -489,10 +489,6 @@ ZEND_BEGIN_ARG_WITH_RETURN_TYPE_MASK_EX(arginfo_ini_get, 0, 1, MAY_BE_STRING|MAY ZEND_ARG_TYPE_INFO(0, option, IS_STRING, 0) ZEND_END_ARG_INFO() -ZEND_BEGIN_ARG_WITH_RETURN_TYPE_MASK_EX(arginfo_ini_bytes, 0, 1, MAY_BE_LONG) - ZEND_ARG_TYPE_INFO(0, option, IS_STRING, 0) -ZEND_END_ARG_INFO() - ZEND_BEGIN_ARG_WITH_RETURN_TYPE_MASK_EX(arginfo_ini_get_all, 0, 0, MAY_BE_ARRAY|MAY_BE_FALSE) ZEND_ARG_TYPE_INFO_WITH_DEFAULT_VALUE(0, extension, IS_STRING, 1, "null") ZEND_ARG_TYPE_INFO_WITH_DEFAULT_VALUE(0, details, _IS_BOOL, 0, "true") @@ -2367,10 +2363,10 @@ ZEND_FUNCTION(highlight_file); ZEND_FUNCTION(php_strip_whitespace); ZEND_FUNCTION(highlight_string); ZEND_FUNCTION(ini_get); -ZEND_FUNCTION(ini_bytes); ZEND_FUNCTION(ini_get_all); ZEND_FUNCTION(ini_set); ZEND_FUNCTION(ini_restore); +ZEND_FUNCTION(ini_parse_quantity); ZEND_FUNCTION(set_include_path); ZEND_FUNCTION(get_include_path); ZEND_FUNCTION(print_r); @@ -2999,11 +2995,11 @@ static const zend_function_entry ext_functions[] = { ZEND_FE(php_strip_whitespace, arginfo_php_strip_whitespace) ZEND_FE(highlight_string, arginfo_highlight_string) ZEND_FE(ini_get, arginfo_ini_get) - ZEND_FE(ini_bytes, arginfo_ini_bytes) ZEND_FE(ini_get_all, arginfo_ini_get_all) ZEND_FE(ini_set, arginfo_ini_set) ZEND_FALIAS(ini_alter, ini_set, arginfo_ini_alter) ZEND_FE(ini_restore, arginfo_ini_restore) + ZEND_FE(ini_parse_quantity, arginfo_ini_parse_quantity) ZEND_FE(set_include_path, arginfo_set_include_path) ZEND_FE(get_include_path, arginfo_get_include_path) ZEND_FE(print_r, arginfo_print_r)