Skip to content

Commit e352661

Browse files
yatsukhnenkoyatsukhnenko
authored
Merge pull request phpredis#1312 from SkydiveMarius/session-locking
Session locking: Fix regenerate ID bug (PHP5, proxy handler)
2 parents 5213ee5 + 0a799af commit e352661

File tree

4 files changed

+290
-2
lines changed

4 files changed

+290
-2
lines changed

‎redis_session.c

Lines changed: 81 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -62,7 +62,7 @@
6262
#define IS_REDIS_OK(r, len) (r != NULL && len == 3 && !memcmp(r, "+OK", 3))
6363

6464
ps_module ps_mod_redis = {
65-
PS_MOD(redis)
65+
PS_MOD_SID(redis)
6666
};
6767
ps_module ps_mod_redis_cluster = {
6868
PS_MOD(rediscluster)
@@ -566,6 +566,73 @@ redis_session_key(redis_pool_member *rpm, const char *key, int key_len, int *ses
566566
return session;
567567
}
568568

569+
/* {{{ PS_CREATE_SID_FUNC
570+
*/
571+
PS_CREATE_SID_FUNC(redis)
572+
{
573+
int retries = 3;
574+
redis_pool *pool = PS_GET_MOD_DATA();
575+
576+
if (!pool) {
577+
#if (PHP_MAJOR_VERSION < 7)
578+
return php_session_create_id(NULL, newlen TSRMLS_CC);
579+
#else
580+
return php_session_create_id(NULL TSRMLS_CC);
581+
#endif
582+
}
583+
584+
while (retries-- > 0) {
585+
#if (PHP_MAJOR_VERSION < 7)
586+
char* sid = php_session_create_id((void **) &pool, newlen TSRMLS_CC);
587+
redis_pool_member *rpm = redis_pool_get_sock(pool, sid TSRMLS_CC);
588+
#else
589+
zend_string* sid = php_session_create_id((void **) &pool TSRMLS_CC);
590+
redis_pool_member *rpm = redis_pool_get_sock(pool, ZSTR_VAL(sid) TSRMLS_CC);
591+
#endif
592+
RedisSock *redis_sock = rpm?rpm->redis_sock:NULL;
593+
594+
if (!rpm || !redis_sock) {
595+
php_error_docref(NULL TSRMLS_CC, E_NOTICE,
596+
"Redis not available while creating session_id");
597+
598+
#if (PHP_MAJOR_VERSION < 7)
599+
efree(sid);
600+
return php_session_create_id(NULL, newlen TSRMLS_CC);
601+
#else
602+
zend_string_release(sid);
603+
return php_session_create_id(NULL TSRMLS_CC);
604+
#endif
605+
}
606+
607+
int resp_len;
608+
#if (PHP_MAJOR_VERSION < 7)
609+
char *full_session_key = redis_session_key(rpm, sid, strlen(sid), &resp_len);
610+
#else
611+
char *full_session_key = redis_session_key(rpm, ZSTR_VAL(sid), ZSTR_LEN(sid), &resp_len);
612+
#endif
613+
char *full_session_key_nt = estrndup(full_session_key, resp_len);
614+
efree(full_session_key);
615+
pool->lock_status->session_key = full_session_key_nt;
616+
617+
if (lock_acquire(redis_sock, pool->lock_status TSRMLS_CC) == SUCCESS) {
618+
return sid;
619+
}
620+
621+
#if (PHP_MAJOR_VERSION < 7)
622+
efree(sid);
623+
#else
624+
zend_string_release(sid);
625+
#endif
626+
sid = NULL;
627+
}
628+
629+
php_error_docref(NULL TSRMLS_CC, E_NOTICE,
630+
"Acquiring session lock failed while creating session_id");
631+
632+
return NULL;
633+
}
634+
/* }}} */
635+
569636
/* {{{ PS_READ_FUNC
570637
*/
571638
PS_READ_FUNC(redis)
@@ -654,6 +721,19 @@ PS_WRITE_FUNC(redis)
654721
/* send SET command */
655722
#if (PHP_MAJOR_VERSION < 7)
656723
session = redis_session_key(rpm, key, strlen(key), &session_len);
724+
725+
/* We need to check for PHP5 if the session key changes (a bug with session_regenerate_id() is causing a missing PS_CREATE_SID call)*/
726+
int session_key_changed = strlen(pool->lock_status->session_key) != session_len || strncmp(pool->lock_status->session_key, session, session_len) != 0;
727+
if (session_key_changed) {
728+
efree(pool->lock_status->session_key);
729+
pool->lock_status->session_key = estrndup(session, session_len);
730+
}
731+
732+
if (session_key_changed && lock_acquire(redis_sock, pool->lock_status TSRMLS_CC) != SUCCESS) {
733+
efree(session);
734+
return FAILURE;
735+
}
736+
657737
cmd_len = REDIS_SPPRINTF(&cmd, "SETEX", "sds", session, session_len,
658738
INI_INT("session.gc_maxlifetime"), val, vallen);
659739
#else

‎redis_session.h

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ PS_READ_FUNC(redis);
99
PS_WRITE_FUNC(redis);
1010
PS_DESTROY_FUNC(redis);
1111
PS_GC_FUNC(redis);
12+
PS_CREATE_SID_FUNC(redis);
1213

1314
PS_OPEN_FUNC(rediscluster);
1415
PS_CLOSE_FUNC(rediscluster);

‎tests/RedisTest.php

Lines changed: 125 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5274,7 +5274,8 @@ public function testSession_lockWaitTime()
52745274
$end = microtime(true);
52755275
$elapsedTime = $end - $start;
52765276

5277-
$this->assertTrue($elapsedTime > 2.5 && $elapsedTime < 3.5);
5277+
$this->assertTrue($elapsedTime > 2.5);
5278+
$this->assertTrue($elapsedTime < 3.5);
52785279
$this->assertTrue($sessionSuccessful);
52795280
}
52805281

@@ -5288,6 +5289,110 @@ public function testMultipleConnect() {
52885289
}
52895290
}
52905291

5292+
public function testSession_regenerateSessionId_noLock_noDestroy() {
5293+
$this->setSessionHandler();
5294+
$sessionId = $this->generateSessionId();
5295+
$this->startSessionProcess($sessionId, 0, false, 300, true, null, -1, 1, 'bar');
5296+
5297+
$newSessionId = $this->regenerateSessionId($sessionId);
5298+
5299+
$this->assertTrue($newSessionId !== $sessionId);
5300+
$this->assertEquals('bar', $this->getSessionData($newSessionId));
5301+
}
5302+
5303+
public function testSession_regenerateSessionId_noLock_withDestroy() {
5304+
$this->setSessionHandler();
5305+
$sessionId = $this->generateSessionId();
5306+
$this->startSessionProcess($sessionId, 0, false, 300, true, null, -1, 1, 'bar');
5307+
5308+
$newSessionId = $this->regenerateSessionId($sessionId, false, true);
5309+
5310+
$this->assertTrue($newSessionId !== $sessionId);
5311+
$this->assertEquals('bar', $this->getSessionData($newSessionId));
5312+
}
5313+
5314+
public function testSession_regenerateSessionId_withLock_noDestroy() {
5315+
$this->setSessionHandler();
5316+
$sessionId = $this->generateSessionId();
5317+
$this->startSessionProcess($sessionId, 0, false, 300, true, null, -1, 1, 'bar');
5318+
5319+
$newSessionId = $this->regenerateSessionId($sessionId, true);
5320+
5321+
$this->assertTrue($newSessionId !== $sessionId);
5322+
$this->assertEquals('bar', $this->getSessionData($newSessionId));
5323+
}
5324+
5325+
public function testSession_regenerateSessionId_withLock_withDestroy() {
5326+
$this->setSessionHandler();
5327+
$sessionId = $this->generateSessionId();
5328+
$this->startSessionProcess($sessionId, 0, false, 300, true, null, -1, 1, 'bar');
5329+
5330+
$newSessionId = $this->regenerateSessionId($sessionId, true, true);
5331+
5332+
$this->assertTrue($newSessionId !== $sessionId);
5333+
$this->assertEquals('bar', $this->getSessionData($newSessionId));
5334+
}
5335+
5336+
public function testSession_regenerateSessionId_noLock_noDestroy_withProxy() {
5337+
if (!interface_exists('SessionHandlerInterface')) {
5338+
$this->markTestSkipped('session handler interface not available in PHP < 5.4');
5339+
}
5340+
5341+
$this->setSessionHandler();
5342+
$sessionId = $this->generateSessionId();
5343+
$this->startSessionProcess($sessionId, 0, false, 300, true, null, -1, 1, 'bar');
5344+
5345+
$newSessionId = $this->regenerateSessionId($sessionId, false, false, true);
5346+
5347+
$this->assertTrue($newSessionId !== $sessionId);
5348+
$this->assertEquals('bar', $this->getSessionData($newSessionId));
5349+
}
5350+
5351+
public function testSession_regenerateSessionId_noLock_withDestroy_withProxy() {
5352+
if (!interface_exists('SessionHandlerInterface')) {
5353+
$this->markTestSkipped('session handler interface not available in PHP < 5.4');
5354+
}
5355+
5356+
$this->setSessionHandler();
5357+
$sessionId = $this->generateSessionId();
5358+
$this->startSessionProcess($sessionId, 0, false, 300, true, null, -1, 1, 'bar');
5359+
5360+
$newSessionId = $this->regenerateSessionId($sessionId, false, true, true);
5361+
5362+
$this->assertTrue($newSessionId !== $sessionId);
5363+
$this->assertEquals('bar', $this->getSessionData($newSessionId));
5364+
}
5365+
5366+
public function testSession_regenerateSessionId_withLock_noDestroy_withProxy() {
5367+
if (!interface_exists('SessionHandlerInterface')) {
5368+
$this->markTestSkipped('session handler interface not available in PHP < 5.4');
5369+
}
5370+
5371+
$this->setSessionHandler();
5372+
$sessionId = $this->generateSessionId();
5373+
$this->startSessionProcess($sessionId, 0, false, 300, true, null, -1, 1, 'bar');
5374+
5375+
$newSessionId = $this->regenerateSessionId($sessionId, true, false, true);
5376+
5377+
$this->assertTrue($newSessionId !== $sessionId);
5378+
$this->assertEquals('bar', $this->getSessionData($newSessionId));
5379+
}
5380+
5381+
public function testSession_regenerateSessionId_withLock_withDestroy_withProxy() {
5382+
if (!interface_exists('SessionHandlerInterface')) {
5383+
$this->markTestSkipped('session handler interface not available in PHP < 5.4');
5384+
}
5385+
5386+
$this->setSessionHandler();
5387+
$sessionId = $this->generateSessionId();
5388+
$this->startSessionProcess($sessionId, 0, false, 300, true, null, -1, 1, 'bar');
5389+
5390+
$newSessionId = $this->regenerateSessionId($sessionId, true, true, true);
5391+
5392+
$this->assertTrue($newSessionId !== $sessionId);
5393+
$this->assertEquals('bar', $this->getSessionData($newSessionId));
5394+
}
5395+
52915396
private function setSessionHandler()
52925397
{
52935398
$host = $this->getHost() ?: 'localhost';
@@ -5358,5 +5463,24 @@ private function getSessionData($sessionId)
53585463

53595464
return $output[0];
53605465
}
5466+
5467+
/**
5468+
* @param string $sessionId
5469+
* @param bool $locking
5470+
* @param bool $destroyPrevious
5471+
* @param bool $sessionProxy
5472+
*
5473+
* @return string
5474+
*/
5475+
private function regenerateSessionId($sessionId, $locking = false, $destroyPrevious = false, $sessionProxy = false)
5476+
{
5477+
$args = array_map('escapeshellarg', array($sessionId, $locking, $destroyPrevious, $sessionProxy));
5478+
5479+
$command = 'php --no-php-ini --define extension=igbinary.so --define extension=' . __DIR__ . '/../modules/redis.so ' . __DIR__ . '/regenerateSessionId.php ' . escapeshellarg($this->getHost()) . ' ' . implode(' ', $args);
5480+
5481+
exec($command, $output);
5482+
5483+
return $output[0];
5484+
}
53615485
}
53625486
?>

‎tests/regenerateSessionId.php

Lines changed: 83 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,83 @@
1+
<?php
2+
error_reporting(E_ERROR | E_WARNING);
3+
4+
$redisHost = $argv[1];
5+
$sessionId = $argv[2];
6+
$locking = !!$argv[3];
7+
$destroyPrevious = !!$argv[4];
8+
$sessionProxy = !!$argv[5];
9+
10+
if (empty($redisHost)) {
11+
$redisHost = 'localhost';
12+
}
13+
14+
ini_set('session.save_handler', 'redis');
15+
ini_set('session.save_path', 'tcp://' . $redisHost . ':6379');
16+
17+
if ($locking) {
18+
ini_set('redis.session.locking_enabled', true);
19+
}
20+
21+
if (interface_exists('SessionHandlerInterface')) {
22+
class TestHandler implements SessionHandlerInterface
23+
{
24+
/**
25+
* @var SessionHandler
26+
*/
27+
private $handler;
28+
29+
public function __construct()
30+
{
31+
$this->handler = new SessionHandler();
32+
}
33+
34+
public function close()
35+
{
36+
return $this->handler->close();
37+
}
38+
39+
public function destroy($session_id)
40+
{
41+
return $this->handler->destroy($session_id);
42+
}
43+
44+
public function gc($maxlifetime)
45+
{
46+
return $this->handler->gc($maxlifetime);
47+
}
48+
49+
public function open($save_path, $name)
50+
{
51+
return $this->handler->open($save_path, $name);
52+
}
53+
54+
public function read($session_id)
55+
{
56+
return $this->handler->read($session_id);
57+
}
58+
59+
public function write($session_id, $session_data)
60+
{
61+
return $this->handler->write($session_id, $session_data);
62+
}
63+
}
64+
}
65+
66+
if ($sessionProxy) {
67+
$handler = new TestHandler();
68+
session_set_save_handler($handler);
69+
}
70+
71+
session_id($sessionId);
72+
if (!session_start()) {
73+
$result = "FAILED: session_start()";
74+
}
75+
elseif (!session_regenerate_id($destroyPrevious)) {
76+
$result = "FAILED: session_regenerate_id()";
77+
}
78+
else {
79+
$result = session_id();
80+
}
81+
session_write_close();
82+
echo $result;
83+

0 commit comments

Comments
 (0)