ext/session: session_start() options arguments type checks.

close phpGH-17388

Author: devnexen

NEWS

@@ -70,6 +70,11 @@ PHP                                                                        NEWS
   . Added ReflectionConstant::getExtension() and ::getExtensionName().
     (DanielEScherzer)
 
+- Session:
+  . session_start() throws a ValueError on option argument if not a hashmap
+    or a TypeError if read_and_close value is not compatible with int.
+    (David Carlier)
+
 - SOAP:
   . Fixed bug #49169 (SoapServer calls wrong function, although "SOAP action"
     header is correct). (nielsdos)

UPGRADING

@@ -129,6 +129,12 @@ PHP 8.5 UPGRADE NOTES
   . posix_fpathconf checks invalid file descriptors and sets
     last_error to EBADF and raises an E_WARNING message.
 
+- Session:
+  . session_start is stricter in regard of the option argument.
+    it throws a ValueError if the whole is not a hashmap or
+    a TypeError if read_on_close value is not a valid type
+    compatible with int.
+
 - Sockets:
   . socket_create_listen, socket_bind and socket_sendto throw a
     ValueError if the port is lower than 0 or greater than 65535,

ext/session/session.c

@@ -2634,9 +2634,8 @@ PHP_FUNCTION(session_start)
 {
 	zval *options = NULL;
 	zval *value;
-	zend_ulong num_idx;
 	zend_string *str_idx;
-	zend_long read_and_close = 0;
+	bool read_and_close = false;
 
 	if (zend_parse_parameters(ZEND_NUM_ARGS(), "|a", &options) == FAILURE) {
 		RETURN_THROWS();
@@ -2659,32 +2658,44 @@ PHP_FUNCTION(session_start)
 
 	/* set options */
 	if (options) {
-		ZEND_HASH_FOREACH_KEY_VAL(Z_ARRVAL_P(options), num_idx, str_idx, value) {
-			if (str_idx) {
-				switch(Z_TYPE_P(value)) {
-					case IS_STRING:
-					case IS_TRUE:
-					case IS_FALSE:
-					case IS_LONG:
-						if (zend_string_equals_literal(str_idx, "read_and_close")) {
-							read_and_close = zval_get_long(value);
+		ZEND_HASH_FOREACH_STR_KEY_VAL(Z_ARRVAL_P(options), str_idx, value) {
+			if (UNEXPECTED(!str_idx)) {
+				zend_argument_value_error(1, "must be of type array with keys as string");
+				RETURN_THROWS();
+			}
+			switch(Z_TYPE_P(value)) {
+				case IS_STRING:
+				case IS_TRUE:
+				case IS_FALSE:
+				case IS_LONG:
+					if (zend_string_equals_literal(str_idx, "read_and_close")) {
+						zend_long tmp;
+						if (Z_TYPE_P(value) != IS_STRING) {
+							tmp = zval_get_long(value);
 						} else {
-							zend_string *tmp_val;
-							zend_string *val = zval_get_tmp_string(value, &tmp_val);
-							if (php_session_start_set_ini(str_idx, val) == FAILURE) {
-								php_error_docref(NULL, E_WARNING, "Setting option \"%s\" failed", ZSTR_VAL(str_idx));
+							if (is_numeric_string(Z_STRVAL_P(value), Z_STRLEN_P(value), &tmp, NULL, false) != IS_LONG) {
+								zend_type_error("%s(): Option \"%s\" value must be of type compatible with int, \"%s\" given",
+										get_active_function_name(), ZSTR_VAL(str_idx), Z_STRVAL_P(value)
+									       );
+								RETURN_THROWS();
 							}
-							zend_tmp_string_release(tmp_val);
 						}
-						break;
-					default:
-						zend_type_error("%s(): Option \"%s\" must be of type string|int|bool, %s given",
+						read_and_close = (tmp > 0);
+					} else {
+						zend_string *tmp_val;
+						zend_string *val = zval_get_tmp_string(value, &tmp_val);
+						if (php_session_start_set_ini(str_idx, val) == FAILURE) {
+							php_error_docref(NULL, E_WARNING, "Setting option \"%s\" failed", ZSTR_VAL(str_idx));
+						}
+						zend_tmp_string_release(tmp_val);
+					}
+					break;
+				default:
+					zend_type_error("%s(): Option \"%s\" must be of type string|int|bool, %s given",
 							get_active_function_name(), ZSTR_VAL(str_idx), zend_zval_value_name(value)
-						);
-						RETURN_THROWS();
-				}
+						       );
+					RETURN_THROWS();
 			}
-			(void) num_idx;
 		} ZEND_HASH_FOREACH_END();
 	}
 

ext/session/tests/session_start_error.phpt

@@ -15,13 +15,22 @@ try {
     echo $exception->getMessage() . "\n";
 }
 
+$read_and_close = "false";
+
+try {
+    session_start([$read_and_close]);
+} catch (ValueError $exception) {
+    echo $exception::class, ': ', $exception->getMessage() . "\n";
+}
+
 var_dump(session_start(['option' => false]));
 
 ob_end_flush();
 
 ?>
 --EXPECTF--
 session_start(): Option "option" must be of type string|int|bool, stdClass given
+ValueError: session_start(): Argument #1 ($options) must be of type array with keys as string
 
 Warning: session_start(): Setting option "option" failed in %s on line %d
 bool(true)

ext/session/tests/session_start_read_and_close.phpt

@@ -19,14 +19,18 @@ foreach ($valuesEnablingReadAndClose as $value) {
 }
 
 foreach ($valuesDisablingReadAndClose as $value) {
-    session_start(["read_and_close" => $value]);
+    try {
+    	session_start(["read_and_close" => $value]);
+    } catch (TypeError $e) {
+    	echo $e::class, ': ', $e->getMessage(), PHP_EOL;
+    }
     var_dump(session_status() === PHP_SESSION_ACTIVE);
     session_write_close();
 }
 
 try {
     session_start(["read_and_close" => 1.0]);
-} catch (Throwable $e) {
+} catch (TypeError $e) {
     echo $e::class, ': ', $e->getMessage(), PHP_EOL;
 }
 
@@ -38,12 +42,16 @@ bool(true)
 bool(true)
 bool(true)
 bool(true)
+bool(false)
+
+Notice: session_start(): Ignoring session_start() because a session is already active (started from %s on line %d) in %s on line %d
 bool(true)
 bool(true)
+TypeError: session_start(): Option "read_and_close" value must be of type compatible with int, "false" given
+bool(false)
 bool(true)
 bool(true)
-bool(true)
-bool(true)
-bool(true)
+TypeError: session_start(): Option "read_and_close" value must be of type compatible with int, "no" given
+bool(false)
 bool(true)
 TypeError: session_start(): Option "read_and_close" must be of type string|int|bool, float given