Corrupted session written when there's a fatal error in autoloader

[ssigwart]

Description

Create two files in the same directory:

index.php

<?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;
	}
}

session_set_save_handler(new TestSessionHandler());

// Autoload class that's in session
spl_autoload_register(function() {
	require_once(__DIR__ . '/Test.class.php');
	return true;
});

// If you use this line, the session still fails to load, but the session will not be written back, which is ideal
// require_once(__DIR__ . '/Test.class.php');

header('Content-type: text/plain');
session_start();
print_r($_SESSION);

Test.class.php

<?php

class Test
{
	// The `= null` results in a fatal error
	public int $var = null;
}

When running index.php, it will result in a fatal error. However, it will also write a session with all variables before the invalid Test class set to zero and no variables after the $_SESSION['test']. You can see that in the output:

<br />
<b>Fatal error</b>:  Default value for property of type int may not be null. Use the nullable type ?int to allow null default value in <b>/path/to/dir/Test.class.php</b> on line <b>5</b><br />
Write session:
before|i:0;

If you uncomment the second require_once so that the autoloader doesn't need to run, it still results in the fatal error as expected, but it does not write back to the session.

I know that avoiding the fatal error is a workaround, but if this can be fixed, it could prevent corrupting user sessions if bad code gets deployed to production.

PHP Version

PHP 8.0.30

Operating System

No response

[SakiTakamachi]

Active support for php8.0.x has ended. Regards.

[ssigwart]

I confirmed that this is also an issue with PHP 8.1.24 and 8.2.11 too. In the test script above, I updated the type hints on the session handler to get rid of the associated warnings for 8.1 and 8.2.

[SakiTakamachi]

Thank you for confirmation. Can you provide a new repro code?

[SakiTakamachi]

Oops, it was edited.

[SakiTakamachi]

I understand that if there is an output after the error, this if passes (i.e. fci_cache is null).

php-src/Zend/zend_execute_API.c

Line 782 in 2fffb83

if (!fci_cache || !fci_cache->function_handler) {

I don't know much about this, so maybe someone else can give you a better answer.

[ssigwart]

Thanks for looking, @SakiTakamachi.

[ssigwart]

I took a look through the code and here's what I found. For existing contributors to the project, they probably already know how the code works, but maybe I'll try to figure out how to debug this myself some day.

• php_session_start calls php_session_initialize.
• Eventually php_session_decode is called.
• PS_SERIALIZER_DECODE_FUNC(php) is the default serializer. The code is below.

php-src/ext/session/session.c

Lines 984 to 1030 in 4ae483a

	PS_SERIALIZER_DECODE_FUNC(php) /* {{{ */
	{
	const char *p, *q;
	const char *endptr = val + vallen;
	ptrdiff_t namelen;
	zend_string *name;
	zend_result retval = SUCCESS;
	php_unserialize_data_t var_hash;
	zval *current, rv;
	PHP_VAR_UNSERIALIZE_INIT(var_hash);
	p = val;
	while (p < endptr) {
	q = p;
	while (*q != PS_DELIMITER) {
	if (++q >= endptr) {
	retval = FAILURE;
	goto break_outer_loop;
	}
	}
	namelen = q - p;
	name = zend_string_init(p, namelen, 0);
	q++;
	current = var_tmp_var(&var_hash);
	if (php_var_unserialize(current, (const unsigned char **)&q, (const unsigned char *)endptr, &var_hash)) {
	ZVAL_PTR(&rv, current);
	php_set_session_var(name, &rv, &var_hash);
	} else {
	zend_string_release_ex(name, 0);
	retval = FAILURE;
	goto break_outer_loop;
	}
	zend_string_release_ex(name, 0);
	p = q;
	}
	break_outer_loop:
	php_session_normalize_vars();
	PHP_VAR_UNSERIALIZE_DESTROY(var_hash);
	return retval;
	}

I think this is were the issue is. I notice that it loops through the serialized string and calls php_var_unserialize. That seems to be where the fatal error happens for the bad class. That loop explains why the variables before it are in the new serialized session, but not those after.

php_var_unserialize calls php_var_unserialize_internal, which calls zend_lookup_class_ex. That calls zend_autoload, which is set to spl_perform_autoload. That's probably where the class file is loaded that results in the exception. Based on the error message, the fatal error comes from zend_compile_prop_decl.

I also see the following code, which I think explains why the variables that are set are zeroed out.

php-src/ext/standard/var_unserializer.re

Lines 859 to 875 in 4ae483a

	result = php_var_unserialize_internal(UNSERIALIZE_PASSTHRU);
	if (!result) {
	/* If the unserialization failed, mark all elements that have been added to var_hash
	* as NULL. This will forbid their use by other unserialize() calls in the same
	* unserialization context. */
	var_entries *e = orig_var_entries;
	zend_long s = orig_used_slots;
	while (e) {
	for (; s < e->used_slots; s++) {
	e->data[s] = NULL;
	}
	e = e->next;
	s = 0;
	}
	}

The one final thing I notices is that if I add the following to the test code above, it shows that the session is active when the fatal error happens.

register_shutdown_function(function() {
	echo 'Exiting. Session is ' . (session_status() === PHP_SESSION_ACTIVE ? 'active' : 'not active') . '.' . PHP_EOL;
});

My guess is that php_session_initialize set the session to active in the below.

php-src/ext/session/session.c

Line 400 in 4ae483a

PS(session_status) = php_session_active;

Perhaps that needs to be set back to disabled if the following line fails:

php-src/ext/session/session.c

Line 477 in 4ae483a

php_session_decode(val);

[nielsdos]

Normally, if the serializer function returns FAILURE, then the session will be destroyed.
The failure path of php_var_unserialize_internal isn't even taken, because otherwise it would correctly destroy the session. What happens is that the compiler performs a bailout, directly jumping out of the deserialization loop in PS_SERIALIZER_DECODE_FUNC(php). You can check that by seeing that neither branches of if (php_var_unserialize(...)) are taken. All the php_set_session_var calls already happened, so you end up with a partial session.
This is fixable by using zend_try to catch the bailout, cancel the session, and reperforming the bailout. Something like this:

diff --git a/ext/session/session.c b/ext/session/session.c
index 114df24b46..311abe84e6 100644
--- a/ext/session/session.c
+++ b/ext/session/session.c
@@ -242,18 +242,29 @@ static zend_string *php_session_encode(void) /* {{{ */
 }
 /* }}} */
 
+static void php_session_cancel_decode(void)
+{
+	php_session_destroy();
+	php_session_track_init();
+	// XXX: debatable if the warning should be here...
+	php_error_docref(NULL, E_WARNING, "Failed to decode session object. Session has been destroyed");
+}
+
 static int 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;
 }
 /* }}} */

Results in:

Fatal error: Default value for property of type int may not be null. Use the nullable type ?int to allow null default > value in /run/media/niels/MoreData/php-8.1/Test.class.php on line 6



Warning: Unknown: Failed to decode session object. Session has been destroyed in Unknown on line 0

Which is more desirable. If you don't want the warning, you move the php_err_docref call.

The sad part is that this isn't exactly a light-weight solution, the jump buffers used in zend_try are heavyweight objects and this will thus incur a cost on every session decode.
Another trick you could use is not writing the session back if a bailout ever happened during execution, but that may cause unforeseen problems because many things could cause bailouts...

[ssigwart]

Thanks, @nielsdos. Is that something you want to submit a PR for or should I add that try/catch?

[nielsdos]

I'm looking for some feedback about the general idea from other contributors before PR'ing that, because it's a bit ad hoc and may have a performance penalty. So I'm holding off for submitting a PR for the moment.

[ssigwart]

Sounds good. Thanks!

[nielsdos]

I've experimented with different kinds of fixes.

I also found that since the session is partially decoded, $_SESSION contains UNDEF zvals which breaks operations done on them. For example adding the following code will cause an assertion failure:

register_shutdown_function(function() {
	var_dump($_SESSION["before"] . 'x');
});

• Using zend_try like in the above example will work. As I said, there's an unfortunate performance penalty then though, although "penalty" might be relative...
• Setting PS(session_status) = php_session_none; before the PS(serializer)->decode call and restoring the status afterwards half-works and is super cheap. It works because the restoring of the status won't happen because of the fatal error. No corrupted session is written, but the shutdown function snippet I posted in this comment will still cause an assertion failure on the concat operation.
• Working on a copy of the session data would work. As the copy would usually be the empty array it's possible to optimize for this. I wonder what the cost would be vs the zend_try solution, it's possible the zend_try solution is cheaper. Also, in this case we'd still write an empty session, which is better than a broken session, but not perfect.

So it seems like the zend_try solution might be the most logical one.
May I ask your opinion @Girgias ?

[Girgias]

Using zend_try() probably makes the most sense. If this causes too much of a performance regression, people will complain fast enough...

[nielsdos]

Using zend_try() probably makes the most sense. If this causes too much of a performance regression, people will complain fast enough...

Although I might be making a fuzz for nothing, seems like it takes about 7 ns to do the setjump on my system...
I'll go that route and make a PR.