Skip to content

zend_atoi/zend_atol cleanup #4132

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 2 commits into from
Closed

zend_atoi/zend_atol cleanup #4132

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e8ce754
Use ZEND_STRTOL instead of zend_atol()
sgolemon May 8, 2019
714d76f
zend_atoi/zend_atol cleanup
sgolemon May 9, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
193 changes: 193 additions & 0 deletions Zend/tests/zend_atol.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,193 @@
--TEST--
Test parsing of INI "size" values (e.g. "16M")
--FILE--
<?php

// This test checks valid formats do not throw any warnings.
foreach (['', ' '] as $leadingWS) {
foreach (['', '+', '-'] as $sign) {
foreach (['', ' '] as $midWS) {
foreach (['', 'K', 'k', 'M', 'm', 'G', 'g'] as $exp) {
foreach (['', ' '] as $trailingWS) {
$setting = sprintf('%s%s1%s%s%s',
$leadingWS, $sign, $midWS, $exp, $trailingWS);
var_dump($setting);

// Technically, negative values don't make sense for socket timeout,
// but it doesn't validate that so use it anyway.
ini_set('default_socket_timeout', $setting);
}
}
}
}
}

--EXPECT--
string(1) "1"
string(2) "1 "
string(2) "1K"
string(3) "1K "
string(2) "1k"
string(3) "1k "
string(2) "1M"
string(3) "1M "
string(2) "1m"
string(3) "1m "
string(2) "1G"
string(3) "1G "
string(2) "1g"
string(3) "1g "
string(2) "1 "
string(3) "1 "
string(3) "1 K"
string(4) "1 K "
string(3) "1 k"
string(4) "1 k "
string(3) "1 M"
string(4) "1 M "
string(3) "1 m"
string(4) "1 m "
string(3) "1 G"
string(4) "1 G "
string(3) "1 g"
string(4) "1 g "
string(2) "+1"
string(3) "+1 "
string(3) "+1K"
string(4) "+1K "
string(3) "+1k"
string(4) "+1k "
string(3) "+1M"
string(4) "+1M "
string(3) "+1m"
string(4) "+1m "
string(3) "+1G"
string(4) "+1G "
string(3) "+1g"
string(4) "+1g "
string(3) "+1 "
string(4) "+1 "
string(4) "+1 K"
string(5) "+1 K "
string(4) "+1 k"
string(5) "+1 k "
string(4) "+1 M"
string(5) "+1 M "
string(4) "+1 m"
string(5) "+1 m "
string(4) "+1 G"
string(5) "+1 G "
string(4) "+1 g"
string(5) "+1 g "
string(2) "-1"
string(3) "-1 "
string(3) "-1K"
string(4) "-1K "
string(3) "-1k"
string(4) "-1k "
string(3) "-1M"
string(4) "-1M "
string(3) "-1m"
string(4) "-1m "
string(3) "-1G"
string(4) "-1G "
string(3) "-1g"
string(4) "-1g "
string(3) "-1 "
string(4) "-1 "
string(4) "-1 K"
string(5) "-1 K "
string(4) "-1 k"
string(5) "-1 k "
string(4) "-1 M"
string(5) "-1 M "
string(4) "-1 m"
string(5) "-1 m "
string(4) "-1 G"
string(5) "-1 G "
string(4) "-1 g"
string(5) "-1 g "
string(2) " 1"
string(3) " 1 "
string(3) " 1K"
string(4) " 1K "
string(3) " 1k"
string(4) " 1k "
string(3) " 1M"
string(4) " 1M "
string(3) " 1m"
string(4) " 1m "
string(3) " 1G"
string(4) " 1G "
string(3) " 1g"
string(4) " 1g "
string(3) " 1 "
string(4) " 1 "
string(4) " 1 K"
string(5) " 1 K "
string(4) " 1 k"
string(5) " 1 k "
string(4) " 1 M"
string(5) " 1 M "
string(4) " 1 m"
string(5) " 1 m "
string(4) " 1 G"
string(5) " 1 G "
string(4) " 1 g"
string(5) " 1 g "
string(3) " +1"
string(4) " +1 "
string(4) " +1K"
string(5) " +1K "
string(4) " +1k"
string(5) " +1k "
string(4) " +1M"
string(5) " +1M "
string(4) " +1m"
string(5) " +1m "
string(4) " +1G"
string(5) " +1G "
string(4) " +1g"
string(5) " +1g "
string(4) " +1 "
string(5) " +1 "
string(5) " +1 K"
string(6) " +1 K "
string(5) " +1 k"
string(6) " +1 k "
string(5) " +1 M"
string(6) " +1 M "
string(5) " +1 m"
string(6) " +1 m "
string(5) " +1 G"
string(6) " +1 G "
string(5) " +1 g"
string(6) " +1 g "
string(3) " -1"
string(4) " -1 "
string(4) " -1K"
string(5) " -1K "
string(4) " -1k"
string(5) " -1k "
string(4) " -1M"
string(5) " -1M "
string(4) " -1m"
string(5) " -1m "
string(4) " -1G"
string(5) " -1G "
string(4) " -1g"
string(5) " -1g "
string(4) " -1 "
string(5) " -1 "
string(5) " -1 K"
string(6) " -1 K "
string(5) " -1 k"
string(6) " -1 k "
string(5) " -1 M"
string(6) " -1 M "
string(5) " -1 m"
string(6) " -1 m "
string(5) " -1 G"
string(6) " -1 G "
string(5) " -1 g"
string(6) " -1 g "
27 changes: 27 additions & 0 deletions Zend/tests/zend_atol_error.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
--TEST--
Test parsing failures of INI "size" values (e.g. "16M")
--FILE--
<?php

// This test checks invalid formats do throw warnings.

$tests = [
'K', # No digits
'1KM', # Multiple multipliers.
'1X', # Unknown multiplier.
'1.0K', # Non integral digits.
];

foreach ($tests as $setting) {
ini_set('default_socket_timeout', $setting);
}

--EXPECTF--

Warning: Invalid numeric string 'K', no valid leading digits, interpreting as '0' in %s/zend_atol_error.php on line %d

Warning: Invalid numeric string '1KM', interpreting as '1M' in %s/zend_atol_error.php on line %d

Warning: Invalid numeric string '1X', interpreting as '1' in %s/zend_atol_error.php on line %d

Warning: Invalid numeric string '1.0K', interpreting as '1K' in %s/zend_atol_error.php on line %d
4 changes: 2 additions & 2 deletions Zend/zend.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Commit these fixes separately?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Happy to split 'em up, sure.


if (stage != ZEND_INI_STAGE_STARTUP &&
stage != ZEND_INI_STAGE_SHUTDOWN &&
Expand Down Expand Up @@ -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)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't the base argument to ZEND_STRTOL be 10 (elsewhere as well)?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For these changes I chose not to make any unnecessary changes.
If we set "10" here, then ONLY base 10 strings will be respected.
That's probably fine for most of these sites, but I don't have a pressing need to say "You can't use octal/hex".

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, indeed. I thought zend_atoi() would only accept base-10 numbers (the misnomer is rather confusing).

zend_dtrace_enabled = 1;
zend_compile_file = dtrace_compile_file;
zend_execute_ex = dtrace_execute_ex;
Expand Down
4 changes: 2 additions & 2 deletions Zend/zend_alloc.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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();
Expand Down
90 changes: 50 additions & 40 deletions Zend/zend_operators.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,8 @@ static _locale_t current_locale = NULL;
#define zend_tolower(c) tolower(c)
#endif

#define ZEND_IS_WHITESPACE(c) (((c) == ' ') || ((c) == '\t') || ((c) == '\n') || ((c) == '\r') || ((c) == '\v') || ((c) == '\f'))

#define TYPE_PAIR(t1,t2) (((t1) << 4) | (t2))

static const unsigned char tolower_map[256] = {
Expand Down Expand Up @@ -80,56 +82,64 @@ static const unsigned char tolower_map[256] = {

ZEND_API int ZEND_FASTCALL zend_atoi(const char *str, size_t str_len) /* {{{ */
{
int retval;

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;
}
}
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'",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure about the warning message ("Invalid numeric string"). Should that be more like "invalid suffix"? Same a few lines below.

(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;
}
/* }}} */
Expand Down Expand Up @@ -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--;
}
Expand Down
4 changes: 2 additions & 2 deletions ext/session/session.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
2 changes: 1 addition & 1 deletion ext/standard/basic_functions.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
Loading