ext/session: reject non-numeric session.upload_progress.freq

ext/session/session.c

@@ -865,16 +865,44 @@ static PHP_INI_MH(OnUpdateSessionDivisor)
 	return SUCCESS;
 }
 
+static bool php_session_is_ini_whitespace(char c)
+{
+	return c == ' ' || c == '\t' || c == '\n' || c == '\r' || c == '\v' || c == '\f';
+}
+
 static PHP_INI_MH(OnUpdateRfc1867Freq)
 {
-	int new_freq = ZEND_ATOL(ZSTR_VAL(new_value));
+	const char *str = ZSTR_VAL(new_value);
+	size_t len = ZSTR_LEN(new_value);
+
+	/* Ignore trailing whitespace so it doesn't hide the '%' suffix (leading
+	 * whitespace is already tolerated by is_numeric_string_ex() below). */
+	while (len > 0 && php_session_is_ini_whitespace(str[len - 1])) {
+		len--;
+	}
+
+	bool is_percentage = len > 0 && str[len - 1] == '%';
+	size_t numeric_len = is_percentage ? len - 1 : len;
+
+	/* Whitespace directly before the '%' (e.g. "5 %") is rejected. */
+	if (is_percentage && numeric_len > 0 && php_session_is_ini_whitespace(str[numeric_len - 1])) {
+		php_error_docref(NULL, E_WARNING, "session.upload_progress.freq must be of type int");
+		return FAILURE;
+	}
+
+	zend_long new_freq = 0;
+	uint8_t type = is_numeric_string_ex(str, numeric_len, &new_freq, NULL, false, NULL, NULL);
+	if (type != IS_LONG) {
+		php_error_docref(NULL, E_WARNING, "session.upload_progress.freq must be of type int");
+		return FAILURE;
+	}
 
 	if (new_freq < 0) {
 		php_error_docref(NULL, E_WARNING, "session.upload_progress.freq must be greater than or equal to 0");
 		return FAILURE;
 	}
 
-	if (ZSTR_LEN(new_value) > 0 && ZSTR_VAL(new_value)[ZSTR_LEN(new_value) - 1] == '%') {
+	if (is_percentage) {
 		if (new_freq > 100) {
 			php_error_docref(NULL, E_WARNING, "session.upload_progress.freq must be less than or equal to 100%%");
 			return FAILURE;
@@ -884,6 +912,12 @@ static PHP_INI_MH(OnUpdateRfc1867Freq)
 		PS(rfc1867_freq) = new_freq;
 	}
 
+	/* Store a whitespace-free canonical value so ini_get() doesn't echo back stray whitespace. */
+	char buf[MAX_LENGTH_OF_LONG + 2];
+	int normalized_len = snprintf(buf, sizeof(buf), is_percentage ? ZEND_LONG_FMT "%%" : ZEND_LONG_FMT, new_freq);
+	entry->value = zend_string_init(buf, (size_t) normalized_len, true);
+	GC_MAKE_PERSISTENT_LOCAL(entry->value);
+
 	return SUCCESS;
 }
 

ext/session/tests/rfc1867_freq_leading_whitespace.phpt

@@ -0,0 +1,17 @@
+--TEST--
+session rfc1867 freq: leading whitespace is tolerated and stripped from the stored value
+--INI--
+session.upload_progress.freq="  5%"
+error_log=
+--EXTENSIONS--
+session
+--SKIPIF--
+<?php
+include('skipif.inc');
+?>
+--FILE--
+<?php
+var_dump(ini_get("session.upload_progress.freq"));
+?>
+--EXPECT--
+string(2) "5%"

ext/session/tests/rfc1867_freq_trailing_percent_space.phpt

@@ -0,0 +1,17 @@
+--TEST--
+session rfc1867 freq: trailing whitespace after the "%" suffix is tolerated
+--INI--
+session.upload_progress.freq="5% "
+error_log=
+--EXTENSIONS--
+session
+--SKIPIF--
+<?php
+include('skipif.inc');
+?>
+--FILE--
+<?php
+var_dump(ini_get("session.upload_progress.freq"));
+?>
+--EXPECT--
+string(2) "5%"

ext/session/tests/rfc1867_invalid_settings_3.phpt

@@ -0,0 +1,18 @@
+--TEST--
+session rfc1867 invalid settings 3: non-numeric freq is rejected instead of silently truncated
+--INI--
+session.upload_progress.freq=5 apples
+error_log=
+--EXTENSIONS--
+session
+--SKIPIF--
+<?php
+include('skipif.inc');
+?>
+--FILE--
+<?php
+var_dump(ini_get("session.upload_progress.freq"));
+?>
+--EXPECTF--
+Warning: PHP Startup: session.upload_progress.freq must be of type int in Unknown on line 0
+string(%d) "1%"

ext/session/tests/rfc1867_invalid_settings_4.phpt

@@ -0,0 +1,18 @@
+--TEST--
+session rfc1867 invalid settings 4: non-numeric freq in percentage mode is rejected instead of silently truncated
+--INI--
+session.upload_progress.freq=5 apples%
+error_log=
+--EXTENSIONS--
+session
+--SKIPIF--
+<?php
+include('skipif.inc');
+?>
+--FILE--
+<?php
+var_dump(ini_get("session.upload_progress.freq"));
+?>
+--EXPECTF--
+Warning: PHP Startup: session.upload_progress.freq must be of type int in Unknown on line 0
+string(%d) "1%"

ext/session/tests/rfc1867_invalid_settings_5.phpt

@@ -0,0 +1,18 @@
+--TEST--
+session rfc1867 invalid settings 5: whitespace-only freq is rejected
+--INI--
+session.upload_progress.freq="   "
+error_log=
+--EXTENSIONS--
+session
+--SKIPIF--
+<?php
+include('skipif.inc');
+?>
+--FILE--
+<?php
+var_dump(ini_get("session.upload_progress.freq"));
+?>
+--EXPECTF--
+Warning: PHP Startup: session.upload_progress.freq must be of type int in Unknown on line 0
+string(%d) "1%"

ext/session/tests/rfc1867_invalid_settings_6.phpt

@@ -0,0 +1,18 @@
+--TEST--
+session rfc1867 invalid settings 6: whitespace between the number and the "%" suffix is rejected
+--INI--
+session.upload_progress.freq="5 %"
+error_log=
+--EXTENSIONS--
+session
+--SKIPIF--
+<?php
+include('skipif.inc');
+?>
+--FILE--
+<?php
+var_dump(ini_get("session.upload_progress.freq"));
+?>
+--EXPECTF--
+Warning: PHP Startup: session.upload_progress.freq must be of type int in Unknown on line 0
+string(%d) "1%"