Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 16 additions & 6 deletions ext/session/session.c
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

makes sense (but will be leaving the final say to girgias) but did you measure roughly the before/after change performance impact ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In isolation I measured the overhead of doing a sigsetjmp and it was around 7ns on my system.

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;
}
/* }}} */
Expand Down
62 changes: 62 additions & 0 deletions ext/session/tests/gh12504.phpt
Original file line number Diff line number Diff line change
@@ -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) {
}