ext/session: improve parsing of session.cookie_lifetime (#21704)

When session.cookie_lifetime was set to a value larger than maxcookie,
OnUpdateCookieLifetime returned SUCCESS without updating the internal
long value, causing ini_get() string and PS(cookie_lifetime) to go
out of sync.

We now properly parse the string value of the ini setting and fail when it is not an integer string or is not within the expected range.

Author: jorgsowa

ext/session/session.c

@@ -704,12 +704,20 @@ static PHP_INI_MH(OnUpdateCookieLifetime)
 #else
 	const zend_long maxcookie = ZEND_LONG_MAX / 2 - 1;
 #endif
-	zend_long v = (zend_long)atol(ZSTR_VAL(new_value));
-	if (v < 0) {
-		php_error_docref(NULL, E_WARNING, "CookieLifetime cannot be negative");
+	zend_long lval = 0;
+	int oflow = 0;
+	uint8_t type = is_numeric_string_ex(ZSTR_VAL(new_value), ZSTR_LEN(new_value), &lval, NULL, false, &oflow, NULL);
+	if (UNEXPECTED(type != IS_LONG)) {
+		if (oflow != 0) {
+			php_error_docref(NULL, E_WARNING, "session.cookie_lifetime must be between 0 and " ZEND_LONG_FMT, maxcookie);
+		} else {
+			php_error_docref(NULL, E_WARNING, "session.cookie_lifetime must be of type int");
+		}
+		return FAILURE;
+	}
+	if (lval < 0 || lval > maxcookie) {
+		php_error_docref(NULL, E_WARNING, "session.cookie_lifetime must be between 0 and " ZEND_LONG_FMT, maxcookie);
 		return FAILURE;
-	} else if (v > maxcookie) {
-		return SUCCESS;
 	}
 
 	return OnUpdateLongGEZero(entry, new_value, mh_arg1, mh_arg2, mh_arg3, stage);

ext/session/tests/gh16290.phpt

@@ -6,8 +6,11 @@ session
 <?php include('skipif.inc'); ?>
 --FILE--
 <?php
+ob_start();
 session_set_cookie_params(PHP_INT_MAX, '/', null, false, true);
 echo "DONE";
+ob_end_flush();
 ?>
---EXPECT--
+--EXPECTF--
+Warning: session_set_cookie_params(): session.cookie_lifetime must be between 0 and %d in %s on line %d
 DONE

ext/session/tests/session_cookie_lifetime_invalid.phpt

@@ -0,0 +1,55 @@
+--TEST--
+session.cookie_lifetime rejects invalid values
+--EXTENSIONS--
+session
+--SKIPIF--
+<?php include('skipif.inc'); ?>
+--FILE--
+<?php
+
+ob_start();
+
+ini_set("session.cookie_lifetime", 100);
+
+// Float strings are rejected
+ini_set("session.cookie_lifetime", "1.5");
+var_dump(ini_get("session.cookie_lifetime"));
+
+// Non-numeric strings are rejected
+ini_set("session.cookie_lifetime", "abc");
+var_dump(ini_get("session.cookie_lifetime"));
+
+// Negative values are rejected
+ini_set("session.cookie_lifetime", -1);
+var_dump(ini_get("session.cookie_lifetime"));
+
+// Negative overflow strings are rejected
+ini_set("session.cookie_lifetime", "-99999999999999999999");
+var_dump(ini_get("session.cookie_lifetime"));
+
+// Overflow values are rejected
+ini_set("session.cookie_lifetime", PHP_INT_MAX);
+var_dump(ini_get("session.cookie_lifetime"));
+
+// Valid values still work after rejection
+ini_set("session.cookie_lifetime", 200);
+var_dump(ini_get("session.cookie_lifetime"));
+
+ob_end_flush();
+?>
+--EXPECTF--
+Warning: ini_set(): session.cookie_lifetime must be of type int in %s on line %d
+string(3) "100"
+
+Warning: ini_set(): session.cookie_lifetime must be of type int in %s on line %d
+string(3) "100"
+
+Warning: ini_set(): session.cookie_lifetime must be between 0 and %d in %s on line %d
+string(3) "100"
+
+Warning: ini_set(): session.cookie_lifetime must be between 0 and %d in %s on line %d
+string(3) "100"
+
+Warning: ini_set(): session.cookie_lifetime must be between 0 and %d in %s on line %d
+string(3) "100"
+string(3) "200"

ext/session/tests/session_get_cookie_params_basic.phpt

@@ -22,7 +22,7 @@ echo "*** Testing session_get_cookie_params() : basic functionality ***\n";
 var_dump(session_get_cookie_params());
 var_dump(session_set_cookie_params(3600, "/path", "blah", FALSE, FALSE));
 var_dump(session_get_cookie_params());
-var_dump(session_set_cookie_params(1234567890, "/guff", "foo", TRUE, TRUE));
+var_dump(session_set_cookie_params(1000000000, "/guff", "foo", TRUE, TRUE));
 var_dump(session_get_cookie_params());
 var_dump(session_set_cookie_params([
   "lifetime" => 123,
@@ -40,7 +40,7 @@ var_dump(session_get_cookie_params());
 echo "Done";
 ob_end_flush();
 ?>
---EXPECTF--
+--EXPECT--
 *** Testing session_get_cookie_params() : basic functionality ***
 array(7) {
   ["lifetime"]=>
@@ -78,7 +78,7 @@ array(7) {
 bool(true)
 array(7) {
   ["lifetime"]=>
-  int(%d)
+  int(1000000000)
   ["path"]=>
   string(5) "/guff"
   ["domain"]=>

ext/session/tests/session_set_cookie_params_basic.phpt

@@ -15,7 +15,7 @@ var_dump(session_set_cookie_params(3600));
 var_dump(session_start());
 var_dump(session_set_cookie_params(1800));
 var_dump(session_destroy());
-var_dump(session_set_cookie_params(1234567890));
+var_dump(session_set_cookie_params(1000000000));
 
 echo "Done";
 ob_end_flush();

ext/session/tests/session_set_cookie_params_variation1.phpt

@@ -24,7 +24,7 @@ var_dump(ini_get("session.cookie_lifetime"));
 var_dump(session_destroy());
 
 var_dump(ini_get("session.cookie_lifetime"));
-var_dump(session_set_cookie_params(1234567890));
+var_dump(session_set_cookie_params(1000000000));
 var_dump(ini_get("session.cookie_lifetime"));
 
 echo "Done";
@@ -44,5 +44,5 @@ string(4) "3600"
 bool(true)
 string(4) "3600"
 bool(true)
-string(10) "1234567890"
+string(10) "1000000000"
 Done

ext/session/tests/session_set_cookie_params_variation8.phpt

@@ -25,7 +25,7 @@ bool(true)
 string(1) "0"
 string(1) "0"
 
-Warning: session_set_cookie_params(): CookieLifetime cannot be negative in %s on line %d
+Warning: session_set_cookie_params(): session.cookie_lifetime must be between 0 and %d in %s on line %d
 bool(false)
 string(1) "0"
 Done