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, several places in php-src use these functions
despite not actually wanting their KMG behavior such as
session.upload_progress.freq which will happily parse "1 banana"
as a valid value.

For these settings, I've switched them to ZEND_STRTOL which preserves
their existing /intended/ behavior.

  * It won't respect KMG suffixes, but they never really wanted that logic anyway.
  * It will ignore non-numeric suffixes so as to not introduce new failures.

We should probably reexamine that second bullet point separately.

Lastly, 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.

Author: sgolemon

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;

Zend/zend_alloc.c

@@ -2705,7 +2705,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;
@@ -2717,7 +2717,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();

Zend/zend_operators.c

@@ -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] = {
@@ -78,58 +80,72 @@ static const unsigned char tolower_map[256] = {
 		zend_binary_strncasecmp
  */
 
-ZEND_API int ZEND_FASTCALL zend_atoi(const char *str, size_t str_len) /* {{{ */
+static void zend_atol_warning(const char *str, size_t str_len, size_t num_len, char suffix) /* {{{ */
 {
-	int retval;
+	/* Duplicate just in case str[str_len] != 0 */
+	zend_string *strcopy = zend_string_init(str, str_len, 0);
+	zend_string *numonly = zend_string_init(str, num_len, 0);
+	const char suffixstr[2] = { suffix, '\0' };
 
-	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;
+	zend_error(E_WARNING, "Invalid numeric string '%s', interpreting as '%s%s'", ZSTR_VAL(strcopy), ZSTR_VAL(numonly), suffixstr);
+	zend_string_release(numonly);
+	zend_string_release(strcopy);
+}
+/* }}} */
+
+
+ZEND_API int ZEND_FASTCALL zend_atoi(const char *str, size_t str_len) /* {{{ */
+{
+	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);
+
+	/* 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_atol_warning(str, str_len, end - str, 0);
+			return retval;
 	}
+
+	if (end < (str + str_len - 1)) {
+		/* More than one character in suffix */
+		zend_atol_warning(str, str_len, end - str, str[str_len-1]);
+	}
+
 	return retval;
 }
 /* }}} */
@@ -3007,7 +3023,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--;
 	}

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;

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);

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);