From e75609751872cd908a7ea9c27c4cc6118bf2bd79 Mon Sep 17 00:00:00 2001 From: Marc Bennewitz Date: Wed, 8 Jun 2016 21:42:26 +0200 Subject: [PATCH 1/2] filesystem: fixed race condition in clearByNamespace, clearByPrefix, clearExpired and flush --- src/Storage/Adapter/Filesystem.php | 76 +++++-- test/Storage/Adapter/FilesystemTest.php | 200 ++++++++++++++++-- .../TestAsset/FilesystemDelayedUnlink.php | 8 +- 3 files changed, 252 insertions(+), 32 deletions(-) diff --git a/src/Storage/Adapter/Filesystem.php b/src/Storage/Adapter/Filesystem.php index 7a645e6c5..6b1896cb2 100644 --- a/src/Storage/Adapter/Filesystem.php +++ b/src/Storage/Adapter/Filesystem.php @@ -110,7 +110,14 @@ public function flush() $clearFolder($pathname); rmdir($pathname); } else { + // remove the file by ignoring errors if the file doesn't exist afterwards + // to fix a possible race condition if onother process removed the faile already. + ErrorHandler::start(); unlink($pathname); + $err = ErrorHandler::stop(); + if ($err && file_exists($pathname)) { + ErrorHandler::addError($err->getSeverity(), $err->getMessage(), $err->getFile(), $err->getLine()); + } } } }; @@ -140,7 +147,7 @@ public function clearExpired() $namespace = $options->getNamespace(); $prefix = ($namespace === '') ? '' : $namespace . $options->getNamespaceSeparator(); - $flags = GlobIterator::SKIP_DOTS | GlobIterator::CURRENT_AS_FILEINFO; + $flags = GlobIterator::SKIP_DOTS | GlobIterator::CURRENT_AS_PATHNAME; $path = $options->getCacheDir() . str_repeat(DIRECTORY_SEPARATOR . $prefix . '*', $options->getDirLevel()) . DIRECTORY_SEPARATOR . $prefix . '*.dat'; @@ -149,15 +156,30 @@ public function clearExpired() $ttl = $options->getTtl(); ErrorHandler::start(); - foreach ($glob as $entry) { - $mtime = $entry->getMTime(); - if ($time >= $mtime + $ttl) { - $pathname = $entry->getPathname(); + foreach ($glob as $pathname) { + // get last modification time of the file but ignore if the file is missing + // to fix a possible race condition if onother process removed the faile already. + ErrorHandler::start(); + $mtime = filemtime($pathname); + $err = ErrorHandler::stop(); + if ($err && file_exists($pathname)) { + ErrorHandler::addError($err->getSeverity(), $err->getMessage(), $err->getFile(), $err->getLine()); + } elseif ($time >= $mtime + $ttl) { + // remove the file by ignoring errors if the file doesn't exist afterwards + // to fix a possible race condition if onother process removed the faile already. + ErrorHandler::start(); unlink($pathname); - - $tagPathname = substr($pathname, 0, -4) . '.tag'; - if (file_exists($tagPathname)) { + $err = ErrorHandler::stop(); + if ($err && file_exists($pathname)) { + ErrorHandler::addError($err->getSeverity(), $err->getMessage(), $err->getFile(), $err->getLine()); + } else { + $tagPathname = substr($pathname, 0, -4) . '.tag'; + ErrorHandler::start(); unlink($tagPathname); + $err = ErrorHandler::stop(); + if ($err && file_exists($pathname)) { + ErrorHandler::addError($err->getSeverity(), $err->getMessage(), $err->getFile(), $err->getLine()); + } } } } @@ -202,11 +224,24 @@ public function clearByNamespace($namespace) ErrorHandler::start(); foreach ($glob as $pathname) { + // remove the file by ignoring errors if the file doesn't exist afterwards + // to fix a possible race condition if onother process removed the faile already. + ErrorHandler::start(); unlink($pathname); + $err = ErrorHandler::stop(); + if ($err && file_exists($pathname)) { + ErrorHandler::addError($err->getSeverity(), $err->getMessage(), $err->getFile(), $err->getLine()); + } } - $error = ErrorHandler::stop(); - if ($error) { - throw new Exception\RuntimeException("Failed to remove files of '{$path}'", 0, $error); + $err = ErrorHandler::stop(); + if ($err) { + $result = false; + return $this->triggerException( + __FUNCTION__, + new ArrayObject(), + $result, + new Exception\RuntimeException("Failed to clear items of namespace '{$namespace}'", 0, $err) + ); } return true; @@ -240,11 +275,24 @@ public function clearByPrefix($prefix) ErrorHandler::start(); foreach ($glob as $pathname) { + // remove the file by ignoring errors if the file doesn't exist afterwards + // to fix a possible race condition if onother process removed the faile already. + ErrorHandler::start(); unlink($pathname); + $err = ErrorHandler::stop(); + if ($err && file_exists($pathname)) { + ErrorHandler::addError($err->getSeverity(), $err->getMessage(), $err->getFile(), $err->getLine()); + } } - $error = ErrorHandler::stop(); - if ($error) { - throw new Exception\RuntimeException("Failed to remove files of '{$path}'", 0, $error); + $err = ErrorHandler::stop(); + if ($err) { + $result = false; + return $this->triggerException( + __FUNCTION__, + new ArrayObject(), + $result, + new Exception\RuntimeException("Failed to remove files of '{$path}'", 0, $err) + ); } return true; diff --git a/test/Storage/Adapter/FilesystemTest.php b/test/Storage/Adapter/FilesystemTest.php index b9a4cfc29..7b755fafc 100644 --- a/test/Storage/Adapter/FilesystemTest.php +++ b/test/Storage/Adapter/FilesystemTest.php @@ -338,15 +338,18 @@ public function testClearByPrefixWithUnexpectedDirectory() /** * @runInSeparateProcess */ - public function testClearByTagsWithoutLocking() + public function testRaceConditionInClearByTags() { if (!function_exists('pcntl_fork') || !function_exists('posix_kill')) { $this->markTestSkipped('Missing pcntl_fork and/or posix_kill'); } + // delay unlink() by global variable $unlinkDelay + global $unlinkDelay; + require __DIR__ . '/TestAsset/FilesystemDelayedUnlink.php'; + // create cache items $this->_storage->getOptions()->setDirLevel(0); - $this->_storage->getOptions()->setFileLocking(false); $this->_storage->setItems([ 'a_key' => 'a_value', 'b_key' => 'b_value', @@ -362,14 +365,15 @@ public function testClearByTagsWithoutLocking() // The parent process // Slow down unlink function and start removing items. // Finally test if the item not matching the tag was removed by the child process. - require __DIR__ . '/TestAsset/FilesystemDelayedUnlink.php'; + $unlinkDelay = 5000; + $this->_storage->clearByTags(['a_tag'], true); - $this->assertFalse($this->_storage->hasItem('other')); + $this->assertFalse($this->_storage->hasItem('other'), 'Child process does not run as expected'); } else { // The child process: // Wait to make sure the parent process has started determining files to unlink. // Than remove one of the items the parent process should remove and another item for testing. - usleep(10000); + usleep(1000); $this->_storage->removeItems(['b_key', 'other']); posix_kill(posix_getpid(), SIGTERM); } @@ -378,22 +382,138 @@ public function testClearByTagsWithoutLocking() /** * @runInSeparateProcess */ - public function testClearByTagsWithLocking() + public function testRaceConditionInClearByNamespace() + { + if (!function_exists('pcntl_fork') || !function_exists('posix_kill')) { + $this->markTestSkipped('Missing pcntl_fork and/or posix_kill'); + } + + // delay unlink() by global variable $unlinkDelay + global $unlinkDelay; + require __DIR__ . '/TestAsset/FilesystemDelayedUnlink.php'; + + // create cache items + $this->_storage->getOptions()->setDirLevel(0); + $this->_storage->getOptions()->setNamespace('ns-other'); + $this->_storage->setItems([ + 'other' => 'other', + ]); + $this->_storage->getOptions()->setNamespace('ns-4-clear'); + $this->_storage->setItems([ + 'a_key' => 'a_value', + 'b_key' => 'b_value', + ]); + + $pidChild = pcntl_fork(); + if ($pidChild == -1) { + $this->fail('pcntl_fork() failed'); + } elseif ($pidChild) { + // The parent process + // Slow down unlink function and start removing items. + // Finally test if the item not matching the tag was removed by the child process. + $unlinkDelay = 5000; + + $this->_storage->getOptions()->setNamespace('ns-4-clear'); + $this->_storage->clearByNamespace('ns-4-clear'); + + $this->assertFalse($this->_storage->hasItem('a_key')); + $this->assertFalse($this->_storage->hasItem('b_key')); + + $this->_storage->getOptions()->setNamespace('ns-other'); + $this->assertFalse($this->_storage->hasItem('other'), 'Child process does not run as expected'); + } else { + // The child process: + // Wait to make sure the parent process has started determining files to unlink. + // Than remove one of the items the parent process should remove and another item for testing. + usleep(1000); + + $this->_storage->getOptions()->setNamespace('ns-4-clear'); + $this->assertTrue($this->_storage->removeItem('b_key')); + + $this->_storage->getOptions()->setNamespace('ns-other'); + $this->assertTrue($this->_storage->removeItem('other')); + + posix_kill(posix_getpid(), SIGTERM); + } + } + + /** + * @runInSeparateProcess + */ + public function testRaceConditionInClearByPrefix() + { + if (!function_exists('pcntl_fork') || !function_exists('posix_kill')) { + $this->markTestSkipped('Missing pcntl_fork and/or posix_kill'); + } + + // delay unlink() by global variable $unlinkDelay + global $unlinkDelay; + require __DIR__ . '/TestAsset/FilesystemDelayedUnlink.php'; + + // create cache items + $this->_storage->getOptions()->setDirLevel(0); + $this->_storage->getOptions()->setNamespace('ns'); + $this->_storage->setItems([ + 'prefix_a_key' => 'a_value', + 'prefix_b_key' => 'b_value', + 'other' => 'other', + ]); + + $pidChild = pcntl_fork(); + if ($pidChild == -1) { + $this->fail('pcntl_fork() failed'); + } elseif ($pidChild) { + // The parent process + // Slow down unlink function and start removing items. + // Finally test if the item not matching the tag was removed by the child process. + $unlinkDelay = 5000; + + $this->_storage->clearByPrefix('prefix_'); + + $this->assertFalse($this->_storage->hasItem('prefix_a_key')); + $this->assertFalse($this->_storage->hasItem('prefix_b_key')); + + $this->assertFalse($this->_storage->hasItem('other'), 'Child process does not run as expected'); + } else { + // The child process: + // Wait to make sure the parent process has started determining files to unlink. + // Than remove one of the items the parent process should remove and another item for testing. + usleep(1000); + + $this->assertTrue($this->_storage->removeItem('prefix_b_key')); + $this->assertTrue($this->_storage->removeItem('other')); + + posix_kill(posix_getpid(), SIGTERM); + } + } + + /** + * @runInSeparateProcess + */ + public function testRaceConditionInClearExpired() { if (!function_exists('pcntl_fork') || !function_exists('posix_kill')) { $this->markTestSkipped('Missing pcntl_fork and/or posix_kill'); } + // delay unlink() by global variable $unlinkDelay + global $unlinkDelay; + require __DIR__ . '/TestAsset/FilesystemDelayedUnlink.php'; + // create cache items $this->_storage->getOptions()->setDirLevel(0); - $this->_storage->getOptions()->setFileLocking(true); + $this->_storage->getOptions()->setTtl(2); $this->_storage->setItems([ 'a_key' => 'a_value', 'b_key' => 'b_value', 'other' => 'other', ]); - $this->_storage->setTags('a_key', ['a_tag']); - $this->_storage->setTags('b_key', ['a_tag']); + + // wait TTL seconds and touch item other so this item will not be deleted by clearExpired + // and can be used for testing the child process + $this->waitForFullSecond(); + sleep(2); + $this->_storage->touchItem('other'); $pidChild = pcntl_fork(); if ($pidChild == -1) { @@ -402,15 +522,67 @@ public function testClearByTagsWithLocking() // The parent process // Slow down unlink function and start removing items. // Finally test if the item not matching the tag was removed by the child process. - require __DIR__ . '/TestAsset/FilesystemDelayedUnlink.php'; - $this->_storage->clearByTags(['a_tag'], true); - $this->assertFalse($this->_storage->hasItem('other')); + $unlinkDelay = 5000; + + $this->_storage->clearExpired(); + + $this->assertFalse($this->_storage->hasItem('a_key')); + $this->assertFalse($this->_storage->hasItem('b_key')); + + $this->assertFalse($this->_storage->hasItem('other'), 'Child process does not run as expected'); } else { // The child process: // Wait to make sure the parent process has started determining files to unlink. // Than remove one of the items the parent process should remove and another item for testing. - usleep(10000); - $this->_storage->removeItems(['b_key', 'other']); + usleep(1000); + + $this->assertTrue($this->_storage->removeItem('b_key')); + $this->assertTrue($this->_storage->removeItem('other')); + + posix_kill(posix_getpid(), SIGTERM); + } + } + + /** + * @runInSeparateProcess + */ + public function testRaceConditionInFlush() + { + if (!function_exists('pcntl_fork') || !function_exists('posix_kill')) { + $this->markTestSkipped('Missing pcntl_fork and/or posix_kill'); + } + + // delay unlink() by global variable $unlinkDelay + global $unlinkDelay; + require __DIR__ . '/TestAsset/FilesystemDelayedUnlink.php'; + + // create cache items + $this->_storage->getOptions()->setDirLevel(0); + $this->_storage->setItems([ + 'a_key' => 'a_value', + 'b_key' => 'b_value', + ]); + + $pidChild = pcntl_fork(); + if ($pidChild == -1) { + $this->fail('pcntl_fork() failed'); + } elseif ($pidChild) { + // The parent process + // Slow down unlink function and start removing items. + $unlinkDelay = 5000; + + $this->_storage->flush(); + + $this->assertFalse($this->_storage->hasItem('a_key')); + $this->assertFalse($this->_storage->hasItem('b_key')); + } else { + // The child process: + // Wait to make sure the parent process has started determining files to unlink. + // Than remove one of the items the parent process should remove. + usleep(1000); + + $this->assertTrue($this->_storage->removeItem('b_key')); + posix_kill(posix_getpid(), SIGTERM); } } diff --git a/test/Storage/Adapter/TestAsset/FilesystemDelayedUnlink.php b/test/Storage/Adapter/TestAsset/FilesystemDelayedUnlink.php index 75da536a4..64d5cd1e6 100644 --- a/test/Storage/Adapter/TestAsset/FilesystemDelayedUnlink.php +++ b/test/Storage/Adapter/TestAsset/FilesystemDelayedUnlink.php @@ -4,10 +4,10 @@ function unlink($path, $context = null) { - usleep(50000); - if ($context) { - return \unlink($path, $context); + global $unlinkDelay; + if (isset($unlinkDelay) && $unlinkDelay > 0) { + usleep($unlinkDelay); } - return \unlink($path); + return $context ? \unlink($path, $context) : \unlink($path); } From 03ba74c69cc58ec6b22b7c252f48077ecab969de Mon Sep 17 00:00:00 2001 From: Marc Bennewitz Date: Sat, 25 Jun 2016 23:17:22 +0200 Subject: [PATCH 2/2] added #102 to CHANGELOG.md --- CHANGELOG.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 371efc085..c5855dad0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,7 +18,8 @@ All notable changes to this project will be documented in this file, in reverse ### Fixed -- Nothing. +- [#102](https://github.com/zendframework/zend-cache/pull/102) + filesystem: fixes a lot of possible race conditions ## 2.7.1 - 2016-05-12