Fix GH-12504: Corrupted session written when there's a fatal error in autoloader

For details and reasoning, see [1] and following.

[1] #12504 (comment)

Closes GH-13207.

Author: ndossche

NEWS

@@ -35,6 +35,10 @@ PHP                                                                        NEWS
   . Fixed bug GH-13138 (Randomizer::pickArrayKeys() does not detect broken
     engines). (timwolla)
 
+- Session:
+  . Fixed bug GH-12504 (Corrupted session written when there's a fatal error
+    in autoloader). (nielsdos)
+
 - Streams:
   . Fixed bug GH-13071 (Copying large files using mmap-able source streams may
     exhaust available memory and fail). (nielsdos)

ext/session/session.c

@@ -246,18 +246,28 @@ static zend_string *php_session_encode(void) /* {{{ */
 }
 /* }}} */
 
+static ZEND_COLD void php_session_cancel_decode(void)
+{
+	php_session_destroy();
+	php_session_track_init();
+	php_error_docref(NULL, E_WARNING, "Failed to decode session object. Session has been destroyed");
+}
+
 static zend_result php_session_decode(zend_string *data) /* {{{ */
 {
 	if (!PS(serializer)) {
 		php_error_docref(NULL, E_WARNING, "Unknown session.serialize_handler. Failed to decode session object");
 		return FAILURE;
 	}
-	if (PS(serializer)->decode(ZSTR_VAL(data), ZSTR_LEN(data)) == FAILURE) {
-		php_session_destroy();
-		php_session_track_init();
-		php_error_docref(NULL, E_WARNING, "Failed to decode session object. Session has been destroyed");
-		return FAILURE;
-	}
+	zend_try {
+		if (PS(serializer)->decode(ZSTR_VAL(data), ZSTR_LEN(data)) == FAILURE) {
+			php_session_cancel_decode();
+			return FAILURE;
+		}
+	} zend_catch {
+		php_session_cancel_decode();
+		zend_bailout();
+	} zend_end_try();
 	return SUCCESS;
 }
 /* }}} */

ext/session/tests/gh12504.phpt

@@ -0,0 +1,62 @@
+--TEST--
+GH-12504 (Corrupted session written when there's a fatal error in autoloader)
+--EXTENSIONS--
+session
+--FILE--
+<?php
+
+class TestSessionHandler implements SessionHandlerInterface
+{
+	public function close(): bool
+	{
+		return true;
+	}
+	public function destroy(string $id): bool
+	{
+		return true;
+	}
+	public function gc(int $max_lifetime): int|false
+	{
+		return 0;
+	}
+	public function open(string $path, string $name): bool
+	{
+		return true;
+	}
+	public function read(string $id): string|false
+	{
+		// Return a session object that has 3 variables
+		return 'before|i:1234;test|O:4:"Test":0:{}after|i:5678;';
+	}
+	public function write($id, $data): bool
+	{
+		echo 'Write session:' . PHP_EOL;
+		echo $data . PHP_EOL;
+		return true;
+	}
+}
+
+register_shutdown_function(function() {
+	echo "In shutdown function\n";
+	var_dump($_SESSION);
+});
+
+session_set_save_handler(new TestSessionHandler());
+
+// Autoload class that's in session
+spl_autoload_register(function() {
+	// Easiest way to reproduce the issue is to dynamically define a class with a bogus definition
+	eval('class Test {public int $var = null;}');
+	return true;
+});
+
+session_start();
+
+?>
+--EXPECTF--
+Fatal error: Default value for property of type int may not be null. Use the nullable type ?int to allow null default value in %s on line %d
+
+Warning: Unknown: Failed to decode session object. Session has been destroyed in Unknown on line 0
+In shutdown function
+array(0) {
+}