From e8ce754163e8342ac6ed0abd934de643351ea66c Mon Sep 17 00:00:00 2001 From: Sara Golemon Date: Wed, 8 May 2019 14:39:23 -0400 Subject: [PATCH 1/2] Use ZEND_STRTOL instead of zend_atol() zend_ato[il]() don't *just* do number parsing. They also check for a 'K', 'M', or 'G' at the end of the string, and multiply the parsed value out accordingly. None of these use cases actually call for this behavior, so invoke ZEND_STRTOL() separately. --- Zend/zend.c | 4 ++-- Zend/zend_alloc.c | 4 ++-- ext/session/session.c | 4 ++-- ext/standard/basic_functions.c | 2 +- ext/zlib/zlib.c | 2 +- 5 files changed, 8 insertions(+), 8 deletions(-) diff --git a/Zend/zend.c b/Zend/zend.c index fceaae62e44f2..20a5fd56a1156 100644 --- a/Zend/zend.c +++ b/Zend/zend.c @@ -149,7 +149,7 @@ static ZEND_INI_MH(OnUpdateAssertions) /* {{{ */ p = (zend_long *) (base+(size_t) mh_arg1); - val = zend_atol(ZSTR_VAL(new_value), ZSTR_LEN(new_value)); + val = ZEND_STRTOL(ZSTR_VAL(new_value), NULL, 0); if (stage != ZEND_INI_STAGE_STARTUP && stage != ZEND_INI_STAGE_SHUTDOWN && @@ -828,7 +828,7 @@ int zend_startup(zend_utility_functions *utility_functions) /* {{{ */ { char *tmp = getenv("USE_ZEND_DTRACE"); - if (tmp && zend_atoi(tmp, 0)) { + if (tmp && ZEND_STRTOL(tmp, NULL, 0)) { zend_dtrace_enabled = 1; zend_compile_file = dtrace_compile_file; zend_execute_ex = dtrace_execute_ex; diff --git a/Zend/zend_alloc.c b/Zend/zend_alloc.c index c57cfbedbd904..88561059184f9 100644 --- a/Zend/zend_alloc.c +++ b/Zend/zend_alloc.c @@ -2661,7 +2661,7 @@ static void alloc_globals_ctor(zend_alloc_globals *alloc_globals) #if ZEND_MM_CUSTOM tmp = getenv("USE_ZEND_ALLOC"); - if (tmp && !zend_atoi(tmp, 0)) { + if (tmp && !ZEND_STRTOL(tmp, NULL, 0)) { alloc_globals->mm_heap = malloc(sizeof(zend_mm_heap)); memset(alloc_globals->mm_heap, 0, sizeof(zend_mm_heap)); alloc_globals->mm_heap->use_custom_heap = ZEND_MM_CUSTOM_HEAP_STD; @@ -2673,7 +2673,7 @@ static void alloc_globals_ctor(zend_alloc_globals *alloc_globals) #endif tmp = getenv("USE_ZEND_ALLOC_HUGE_PAGES"); - if (tmp && zend_atoi(tmp, 0)) { + if (tmp && ZEND_STRTOL(tmp, NULL, 0)) { zend_mm_use_huge_pages = 1; } alloc_globals->mm_heap = zend_mm_init(); diff --git a/ext/session/session.c b/ext/session/session.c index b639f432c3a42..9f5a9eeb007fc 100644 --- a/ext/session/session.c +++ b/ext/session/session.c @@ -768,8 +768,8 @@ static PHP_INI_MH(OnUpdateLazyWrite) /* {{{ */ static PHP_INI_MH(OnUpdateRfc1867Freq) /* {{{ */ { - int tmp; - tmp = zend_atoi(ZSTR_VAL(new_value), ZSTR_LEN(new_value)); + zend_long tmp = ZEND_STRTOL(ZSTR_VAL(new_value), NULL, 0); + if(tmp < 0) { php_error_docref(NULL, E_WARNING, "session.upload_progress.freq must be greater than or equal to zero"); return FAILURE; diff --git a/ext/standard/basic_functions.c b/ext/standard/basic_functions.c index 4b3804f589555..8696f2bf778e9 100644 --- a/ext/standard/basic_functions.c +++ b/ext/standard/basic_functions.c @@ -5994,7 +5994,7 @@ static void php_simple_ini_parser_cb(zval *arg1, zval *arg2, zval *arg3, int cal } if (!(Z_STRLEN_P(arg1) > 1 && Z_STRVAL_P(arg1)[0] == '0') && is_numeric_string(Z_STRVAL_P(arg1), Z_STRLEN_P(arg1), NULL, NULL, 0) == IS_LONG) { - zend_ulong key = (zend_ulong) zend_atol(Z_STRVAL_P(arg1), Z_STRLEN_P(arg1)); + zend_ulong key = ZEND_STRTOUL(Z_STRVAL_P(arg1), NULL, 0); if ((find_hash = zend_hash_index_find(Z_ARRVAL_P(arr), key)) == NULL) { array_init(&hash); find_hash = zend_hash_index_add_new(Z_ARRVAL_P(arr), key, &hash); diff --git a/ext/zlib/zlib.c b/ext/zlib/zlib.c index 7eb38c3c04dc0..c81a288603664 100644 --- a/ext/zlib/zlib.c +++ b/ext/zlib/zlib.c @@ -1436,7 +1436,7 @@ static PHP_INI_MH(OnUpdate_zlib_output_compression) } else if (!strncasecmp(ZSTR_VAL(new_value), "on", sizeof("on"))) { int_value = 1; } else { - int_value = zend_atoi(ZSTR_VAL(new_value), ZSTR_LEN(new_value)); + int_value = ZEND_STRTOL(ZSTR_VAL(new_value), NULL, 0); } ini_value = zend_ini_string("output_handler", sizeof("output_handler"), 0); From 714d76f400bc9c18641c16954f606869e8824a9b Mon Sep 17 00:00:00 2001 From: Sara Golemon Date: Thu, 9 May 2019 11:25:54 -0400 Subject: [PATCH 2/2] zend_atoi/zend_atol cleanup zend_ato[il]() don't just do number parsing. They also check for a 'K', 'M', or 'G' at the end of the string, and multiply the parsed value out accordingly. Unfortunately, they ignore any other non-numerics between the numeric component and the last character in the string. This means that numbers such as the following are both valid and non-intuitive in their final output. * "123KMG" is interpreted as "123G" -> 132070244352 * "123G " is interpreted as "123 " -> 123 * "123GB" is interpreted as "123B" -> 123 * "123 I like tacos." is also interpreted as "123." -> 123 This diff primarily adds warnings for these cases when the output would be a potentially unexpected, and unhelpful value. Additionally, with these changes, zend_atoi() is no longer used in php-src, but I left it as a valid API for 3rd party extensions. Note that I did make it a proxy to zend_atol() since deferring the truncation till the end is effectively the same as truncation during multiplication, but this avoid code duplication. I think we should consider removing zend_atoi() entirely (perhaps in 8.0?) and rename zend_atol() to something reflecting it's KMG specific behavior. --- Zend/tests/zend_atol.phpt | 193 ++++++++++++++++++++++++++++++++ Zend/tests/zend_atol_error.phpt | 27 +++++ Zend/zend_operators.c | 90 ++++++++------- 3 files changed, 270 insertions(+), 40 deletions(-) create mode 100644 Zend/tests/zend_atol.phpt create mode 100644 Zend/tests/zend_atol_error.phpt diff --git a/Zend/tests/zend_atol.phpt b/Zend/tests/zend_atol.phpt new file mode 100644 index 0000000000000..350c3398b0c19 --- /dev/null +++ b/Zend/tests/zend_atol.phpt @@ -0,0 +1,193 @@ +--TEST-- +Test parsing of INI "size" values (e.g. "16M") +--FILE-- +0) { - switch (str[str_len-1]) { - case 'g': - case 'G': - retval *= 1024; - /* break intentionally missing */ - case 'm': - case 'M': - retval *= 1024; - /* break intentionally missing */ - case 'k': - case 'K': - retval *= 1024; - break; - } - } - return retval; + return (int)zend_atol(str, str_len); } /* }}} */ ZEND_API zend_long ZEND_FASTCALL zend_atol(const char *str, size_t str_len) /* {{{ */ { zend_long retval; + char *end = NULL; if (!str_len) { str_len = strlen(str); } - retval = ZEND_STRTOL(str, NULL, 0); - if (str_len>0) { - switch (str[str_len-1]) { - case 'g': - case 'G': - retval *= 1024; - /* break intentionally missing */ - case 'm': - case 'M': - retval *= 1024; - /* break intentionally missing */ - case 'k': - case 'K': - retval *= 1024; - break; - } + + /* Ignore trailing whitespace */ + while (str_len && ZEND_IS_WHITESPACE(str[str_len-1])) --str_len; + if (!str_len) return 0; + + /* Parse integral portion */ + retval = ZEND_STRTOL(str, &end, 0); + + if (!(end - str)) { + zend_error(E_WARNING, "Invalid numeric string '%.*s', no valid leading digits, interpreting as '0'", + (int)str_len, str); + return 0; } + + /* Allow for whitespace between integer portion and any suffix character */ + while (ZEND_IS_WHITESPACE(*end)) ++end; + + /* No exponent suffix. */ + if (!*end) return retval; + + switch (str[str_len-1]) { + case 'g': + case 'G': + retval *= 1024; + /* break intentionally missing */ + case 'm': + case 'M': + retval *= 1024; + /* break intentionally missing */ + case 'k': + case 'K': + retval *= 1024; + break; + default: + /* Unknown suffix */ + zend_error(E_WARNING, "Invalid numeric string '%.*s', interpreting as '%.*s'", + (int)str_len, str, (int)(end - str), str); + return retval; + } + + if (end < (str + str_len - 1)) { + /* More than one character in suffix */ + zend_error(E_WARNING, "Invalid numeric string '%.*s', interpreting as '%.*s%c'", + (int)str_len, str, (int)(end - str), str, str[str_len-1]); + } + return retval; } /* }}} */ @@ -3007,7 +3017,7 @@ ZEND_API zend_uchar ZEND_FASTCALL _is_numeric_string_ex(const char *str, size_t /* Skip any whitespace * This is much faster than the isspace() function */ - while (*str == ' ' || *str == '\t' || *str == '\n' || *str == '\r' || *str == '\v' || *str == '\f') { + while (ZEND_IS_WHITESPACE(*str)) { str++; length--; }