Block premature access to session storage

[znerol]

Description

• session_start() unconditionally calls SessionHandler::read() - even in cases where the session id has been newly generated just a couple of lines earlier.
• session_write_close() unconditionally calls SessionHandler::write() even if the session id has been newly generated for the current request and the session is empty.
• session_regenerate_id(true) unconditionally calls SessionHandler::destroy() even if the session id has been newly generated for the current request.

I think there is some room for optimization if the session module could suppress calls into the session handler in certain situations. I guess its best expressed as phpt what I'm having in mind.

---

Incoming requests without a session (cookie) shouldn't be accessing storage at all:

--TEST--
Request without session
--EXTENSIONS--
session
--INI--
session.use_strict_mode=1
--FILE--
<?php
error_reporting(E_ALL);
ob_start();

class handler implements SessionHandlerInterface, SessionUpdateTimestampHandlerInterface {
    function open($save_path, $session_name): bool
    {
        print "OPEN: $session_name\n";
        return true;
    }

    function close(): bool
    {
        print "CLOSE\n";
        return true;
    }

    function read($key): string|false
    {
        print "READ: $key\n";
        return "";
    }

    function write($key, $val): bool
    {
        print "WRITE: $key, $val\n";
        return true;
    }

    function destroy($key): bool
    {
        print "DESTROY: $key\n";
        return true;
    }

    function updateTimestamp(string $id, string $data): bool {
        print "UPDATE_TIMESTAMP: $id, $data\n";
        return true;
    }

    function validateId(string $id): bool {
        print "VALIDATE_ID: $id\n";
        return false;
    }

    function gc($max_lifetime): int { return 0; }
}

session_set_save_handler(new handler());

session_start();
session_write_close();
?>
--EXPECT--
OPEN: PHPSESSID
CLOSE

This is failing with the following diff:

Running selected tests.
TEST 1/1 [ext/session/tests/user_session_module/request-without-session.phpt]
========DIFF========
     OPEN: PHPSESSID
002+ READ: 78d7fe4a7170fb5a60934db9784d7d63
003+ WRITE: 78d7fe4a7170fb5a60934db9784d7d63,
     CLOSE
========DONE========

---

When logging in the initial empty session record shouldn't attempted to be destroyed. Also there is no point in attempting to read a session record with the newly generated id:

--TEST--
Request with login
--EXTENSIONS--
session
--INI--
session.use_strict_mode=1
--FILE--
<?php
error_reporting(E_ALL);
ob_start();

class handler implements SessionHandlerInterface, SessionUpdateTimestampHandlerInterface {
    function open($save_path, $session_name): bool
    {
        print "OPEN: $session_name\n";
        return true;
    }

    function close(): bool
    {
        print "CLOSE\n";
        return true;
    }

    function read($key): string|false
    {
        print "READ: $key\n";
        return "user_id|i:42;";
    }

    function write($key, $val): bool
    {
        print "WRITE: $key, $val\n";
        return true;
    }

    function destroy($key): bool
    {
        print "DESTROY: $key\n";
        return true;
    }

    function updateTimestamp(string $id, string $data): bool {
        print "UPDATE_TIMESTAMP: $id, $data\n";
        return true;
    }

    function validateId(string $id): bool {
        print "VALIDATE_ID: $id\n";
        return false;
    }

    function gc($max_lifetime): int { return 0; }
}

session_set_save_handler(new handler());

session_start();
session_regenerate_id(TRUE);
$_SESSION['user_id']=42;
session_write_close();
?>
--EXPECTF--
OPEN: PHPSESSID
CLOSE
OPEN: PHPSESSID
VALIDATE_ID: %s
WRITE: %s %s
CLOSE

This is failing with the following diff:

Running selected tests.
TEST 1/1 [ext/session/tests/user_session_module/request-with-login.phpt]
========DIFF========
     OPEN: PHPSESSID
002+ READ: 9f2306bd1721271b20061dfe45d044a1
003+ DESTROY: 9f2306bd1721271b20061dfe45d044a1
     CLOSE
     OPEN: PHPSESSID
     VALIDATE_ID: a7efc5fb4eca6fd6e44799a6e50e7960
007+ READ: a7efc5fb4eca6fd6e44799a6e50e7960
     WRITE: %s %s
     CLOSE
========DONE========

---

When logging out the resulting empty session record shouldn't attempted to be read and written:

--TEST--
Request with logout
--EXTENSIONS--
session
--INI--
session.use_strict_mode=1
--FILE--
<?php
error_reporting(E_ALL);
ob_start();

class handler implements SessionHandlerInterface, SessionUpdateTimestampHandlerInterface {
    function open($save_path, $session_name): bool
    {
        print "OPEN: $session_name\n";
        return true;
    }

    function close(): bool
    {
        print "CLOSE\n";
        return true;
    }

    function read($key): string|false
    {
        print "READ: $key\n";
        return "user_id|i:42;";
    }

    function write($key, $val): bool
    {
        print "WRITE: $key, $val\n";
        return true;
    }

    function destroy($key): bool
    {
        print "DESTROY: $key\n";
        return true;
    }

    function updateTimestamp(string $id, string $data): bool {
        print "UPDATE_TIMESTAMP: $id, $data\n";
        return true;
    }

    function validateId(string $id): bool {
        print "VALIDATE_ID: $id\n";
        return $GLOBALS['validId'];
    }

    function gc($max_lifetime): int { return 0; }
}

session_set_save_handler(new handler());

$GLOBALS['validId'] = true;
session_id(session_create_id());
session_start();
$_SESSION = [];
$GLOBALS['validId'] = false;
session_regenerate_id(TRUE);
session_write_close();
?>
--EXPECTF--
OPEN: PHPSESSID
VALIDATE_ID: %s
READ: %s
DESTROY: %s
CLOSE
OPEN: PHPSESSID
VALIDATE_ID: %s
CLOSE

This is failing with the following diff:

Running selected tests.
TEST 1/1 [ext/session/tests/user_session_module/request-with-logout.phpt]
========DIFF========
--
     CLOSE
     OPEN: PHPSESSID
     VALIDATE_ID: %s
008+ READ: 483617bb2349fe2345fcb67b391e7b1d
009+ WRITE: 483617bb2349fe2345fcb67b391e7b1d,
     CLOSE
========DONE========

[andypost]

Thank you raising it. +1 a bit more context from Drupal https://www.drupal.org/project/drupal/issues/3570824#comment-16451703

[iluuu1994]

/cc @Girgias