From 9e37810c0827acdfcbb5f373bf20264fc038038f Mon Sep 17 00:00:00 2001 From: kenjis Date: Mon, 11 Dec 2023 17:21:22 +0900 Subject: [PATCH 1/7] style: break long lines --- system/Session/Handlers/RedisHandler.php | 30 +++++++++++++++++++----- 1 file changed, 24 insertions(+), 6 deletions(-) diff --git a/system/Session/Handlers/RedisHandler.php b/system/Session/Handlers/RedisHandler.php index b429792929b4..b516bcec7a1c 100644 --- a/system/Session/Handlers/RedisHandler.php +++ b/system/Session/Handlers/RedisHandler.php @@ -124,12 +124,20 @@ public function open($path, $name): bool $redis = new Redis(); - if (! $redis->connect($this->savePath['protocol'] . '://' . $this->savePath['host'], ($this->savePath['host'][0] === '/' ? 0 : $this->savePath['port']), $this->savePath['timeout'])) { + if ( + ! $redis->connect( + $this->savePath['protocol'] . '://' . $this->savePath['host'], + ($this->savePath['host'][0] === '/' ? 0 : $this->savePath['port']), + $this->savePath['timeout'] + ) + ) { $this->logger->error('Session: Unable to connect to Redis with the configured settings.'); } elseif (isset($this->savePath['password']) && ! $redis->auth($this->savePath['password'])) { $this->logger->error('Session: Unable to authenticate to Redis instance.'); } elseif (isset($this->savePath['database']) && ! $redis->select($this->savePath['database'])) { - $this->logger->error('Session: Unable to select Redis database with index ' . $this->savePath['database']); + $this->logger->error( + 'Session: Unable to select Redis database with index ' . $this->savePath['database'] + ); } else { $this->redis = $redis; @@ -251,7 +259,9 @@ public function destroy($id): bool { if (isset($this->redis, $this->lockKey)) { if (($result = $this->redis->del($this->keyPrefix . $id)) !== 1) { - $this->logger->debug('Session: Redis::del() expected to return 1, got ' . var_export($result, true) . ' instead.'); + $this->logger->debug( + 'Session: Redis::del() expected to return 1, got ' . var_export($result, true) . ' instead.' + ); } return $this->destroyCookie(); @@ -303,7 +313,9 @@ protected function lockSession(string $sessionID): bool } if (! $this->redis->setex($lockKey, 300, (string) Time::now()->getTimestamp())) { - $this->logger->error('Session: Error while trying to obtain lock for ' . $this->keyPrefix . $sessionID); + $this->logger->error( + 'Session: Error while trying to obtain lock for ' . $this->keyPrefix . $sessionID + ); return false; } @@ -313,13 +325,19 @@ protected function lockSession(string $sessionID): bool } while (++$attempt < 30); if ($attempt === 30) { - log_message('error', 'Session: Unable to obtain lock for ' . $this->keyPrefix . $sessionID . ' after 30 attempts, aborting.'); + log_message( + 'error', + 'Session: Unable to obtain lock for ' . $this->keyPrefix . $sessionID . ' after 30 attempts, aborting.' + ); return false; } if ($ttl === -1) { - log_message('debug', 'Session: Lock for ' . $this->keyPrefix . $sessionID . ' had no TTL, overriding.'); + log_message( + 'debug', + 'Session: Lock for ' . $this->keyPrefix . $sessionID . ' had no TTL, overriding.' + ); } $this->lock = true; From ce34999b9b2bf28d0aff0eedd82f091c1b9b9b1f Mon Sep 17 00:00:00 2001 From: kenjis Date: Mon, 11 Dec 2023 17:22:34 +0900 Subject: [PATCH 2/7] refactor: use $this->logger instead of log_message() --- system/Session/Handlers/RedisHandler.php | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/system/Session/Handlers/RedisHandler.php b/system/Session/Handlers/RedisHandler.php index b516bcec7a1c..34d33e9565ff 100644 --- a/system/Session/Handlers/RedisHandler.php +++ b/system/Session/Handlers/RedisHandler.php @@ -325,8 +325,7 @@ protected function lockSession(string $sessionID): bool } while (++$attempt < 30); if ($attempt === 30) { - log_message( - 'error', + $this->logger->error( 'Session: Unable to obtain lock for ' . $this->keyPrefix . $sessionID . ' after 30 attempts, aborting.' ); @@ -334,8 +333,7 @@ protected function lockSession(string $sessionID): bool } if ($ttl === -1) { - log_message( - 'debug', + $this->logger->debug( 'Session: Lock for ' . $this->keyPrefix . $sessionID . ' had no TTL, overriding.' ); } From 849b93a3bf0c8f06e077a3bf056b3a8b2fc981bc Mon Sep 17 00:00:00 2001 From: kenjis Date: Mon, 11 Dec 2023 18:06:41 +0900 Subject: [PATCH 3/7] fix: Redis Session race condition --- system/Session/Handlers/RedisHandler.php | 35 ++++++++++-------------- 1 file changed, 14 insertions(+), 21 deletions(-) diff --git a/system/Session/Handlers/RedisHandler.php b/system/Session/Handlers/RedisHandler.php index 34d33e9565ff..5f0392b78921 100644 --- a/system/Session/Handlers/RedisHandler.php +++ b/system/Session/Handlers/RedisHandler.php @@ -297,47 +297,40 @@ protected function lockSession(string $sessionID): bool // so we need to check here if the lock key is for the // correct session ID. if ($this->lockKey === $lockKey) { + // If there is the lock, make the ttl longer. return $this->redis->expire($this->lockKey, 300); } $attempt = 0; do { - $ttl = $this->redis->ttl($lockKey); - assert(is_int($ttl)); + $result = $this->redis->set( + $lockKey, + (string) Time::now()->getTimestamp(), + // NX -- Only set the key if it does not already exist. + // EX seconds -- Set the specified expire time, in seconds. + ['nx', 'ex' => 300] + ); - if ($ttl > 0) { - sleep(1); + if (! $result) { + usleep(100000); continue; } - if (! $this->redis->setex($lockKey, 300, (string) Time::now()->getTimestamp())) { - $this->logger->error( - 'Session: Error while trying to obtain lock for ' . $this->keyPrefix . $sessionID - ); - - return false; - } - $this->lockKey = $lockKey; break; - } while (++$attempt < 30); + } while (++$attempt < 300); - if ($attempt === 30) { + if ($attempt === 300) { $this->logger->error( - 'Session: Unable to obtain lock for ' . $this->keyPrefix . $sessionID . ' after 30 attempts, aborting.' + 'Session: Unable to obtain lock for ' . $this->keyPrefix . $sessionID + . ' after 300 attempts, aborting.' ); return false; } - if ($ttl === -1) { - $this->logger->debug( - 'Session: Lock for ' . $this->keyPrefix . $sessionID . ' had no TTL, overriding.' - ); - } - $this->lock = true; return true; From 63beda25040e82c18370b857fc5cb612e52501c8 Mon Sep 17 00:00:00 2001 From: kenjis Date: Thu, 22 Feb 2024 12:40:10 +0900 Subject: [PATCH 4/7] fix: close() should return false if releaseLock() failed The return value (usually true on success, false on failure) https://www.php.net/manual/en/sessionhandlerinterface.close.php#refsect1-sessionhandlerinterface.close-returnvalues --- system/Session/Handlers/RedisHandler.php | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/system/Session/Handlers/RedisHandler.php b/system/Session/Handlers/RedisHandler.php index 5f0392b78921..5e175de4a9f3 100644 --- a/system/Session/Handlers/RedisHandler.php +++ b/system/Session/Handlers/RedisHandler.php @@ -231,7 +231,9 @@ public function close(): bool if (($pingReply === true) || ($pingReply === '+PONG')) { if (isset($this->lockKey)) { - $this->releaseLock(); + if (! $this->releaseLock()) { + return false; + } } if (! $this->redis->close()) { From 7707ddf7dd841a361b58c6c06906e98790095cdc Mon Sep 17 00:00:00 2001 From: kenjis Date: Thu, 22 Feb 2024 12:46:17 +0900 Subject: [PATCH 5/7] fix: read() should return false if lockSession() failed --- system/Session/Handlers/RedisHandler.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/system/Session/Handlers/RedisHandler.php b/system/Session/Handlers/RedisHandler.php index 5e175de4a9f3..657fe7ddd770 100644 --- a/system/Session/Handlers/RedisHandler.php +++ b/system/Session/Handlers/RedisHandler.php @@ -176,7 +176,7 @@ public function read($id) return $data; } - return ''; + return false; } /** From 0617a7be1c917cd17fce3000057aece08427fe93 Mon Sep 17 00:00:00 2001 From: kenjis Date: Thu, 22 Feb 2024 12:55:17 +0900 Subject: [PATCH 6/7] refactor: by rector --- system/Session/Handlers/RedisHandler.php | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/system/Session/Handlers/RedisHandler.php b/system/Session/Handlers/RedisHandler.php index 657fe7ddd770..e9bcbe3ff8d6 100644 --- a/system/Session/Handlers/RedisHandler.php +++ b/system/Session/Handlers/RedisHandler.php @@ -230,10 +230,8 @@ public function close(): bool $pingReply = $this->redis->ping(); if (($pingReply === true) || ($pingReply === '+PONG')) { - if (isset($this->lockKey)) { - if (! $this->releaseLock()) { - return false; - } + if (isset($this->lockKey) && ! $this->releaseLock()) { + return false; } if (! $this->redis->close()) { From 2fa55d435bd2717eab18770d2062d00cf83b1c57 Mon Sep 17 00:00:00 2001 From: kenjis Date: Fri, 23 Feb 2024 09:51:41 +0900 Subject: [PATCH 7/7] docs: add @throws --- system/Session/Handlers/RedisHandler.php | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/system/Session/Handlers/RedisHandler.php b/system/Session/Handlers/RedisHandler.php index e9bcbe3ff8d6..5a81e6a4da00 100644 --- a/system/Session/Handlers/RedisHandler.php +++ b/system/Session/Handlers/RedisHandler.php @@ -115,6 +115,8 @@ protected function setSavePath(): void * * @param string $path The path where to store/retrieve the session * @param string $name The session name + * + * @throws RedisException */ public function open($path, $name): bool { @@ -154,6 +156,8 @@ public function open($path, $name): bool * * @return false|string Returns an encoded string of the read data. * If nothing was read, it must return false. + * + * @throws RedisException */ #[ReturnTypeWillChange] public function read($id) @@ -184,6 +188,8 @@ public function read($id) * * @param string $id The session ID * @param string $data The encoded session data + * + * @throws RedisException */ public function write($id, $data): bool { @@ -254,6 +260,8 @@ public function close(): bool * Destroys a session * * @param string $id The session ID being destroyed + * + * @throws RedisException */ public function destroy($id): bool { @@ -288,6 +296,8 @@ public function gc($max_lifetime) * Acquires an emulated lock. * * @param string $sessionID Session ID + * + * @throws RedisException */ protected function lockSession(string $sessionID): bool { @@ -338,6 +348,8 @@ protected function lockSession(string $sessionID): bool /** * Releases a previously acquired lock + * + * @throws RedisException */ protected function releaseLock(): bool {