From 633a907bf974d303a2ee19eef070efdc6150ca2c Mon Sep 17 00:00:00 2001 From: Alwin Garside Date: Fri, 26 Oct 2018 00:06:40 +0200 Subject: [PATCH 1/3] [5.7] Fix filesystem locking hangs in PackageManifest::build() (#25898) - Filesystem.php 1. Created a new `Filesystem::replace()` method that atomically replaces a file if it already exists. - PackageManifest.php 1. The Filesystem::get() call in PackageManifest::getManifest() no longer has to use an exclusive lock because the packages.php manifest file will always be replaced atomically. 2. Use the new Filesystem::replace() method in PackageManifest::write() --- src/Illuminate/Filesystem/Filesystem.php | 25 +++++++++ src/Illuminate/Foundation/PackageManifest.php | 7 ++- tests/Filesystem/FilesystemTest.php | 51 +++++++++++++++++++ 3 files changed, 79 insertions(+), 4 deletions(-) diff --git a/src/Illuminate/Filesystem/Filesystem.php b/src/Illuminate/Filesystem/Filesystem.php index b458500e81ae..b02b161f2b75 100644 --- a/src/Illuminate/Filesystem/Filesystem.php +++ b/src/Illuminate/Filesystem/Filesystem.php @@ -122,6 +122,31 @@ public function put($path, $contents, $lock = false) return file_put_contents($path, $contents, $lock ? LOCK_EX : 0); } + /** + * Write the contents of a file, replacing it atomically if it already exists. + * + * This will replace the target file permissions. It also resolves symlinks to replace the symlink's target file. + * + * @param string $path + * @param string $content + */ + public function replace($path, $content) + { + // Just in case path already exists and is a symlink, we want to make sure we get the real path. + clearstatcache(true, $path); + $path = realpath($path) ?: $path; + + // Write out the contents to a temp file, so we then can rename the file atomically. + $tempPath = tempnam(dirname($path), basename($path)); + + // Fix permissions of tempPath because `tempnam()` creates it with permissions set to 0600. + chmod($tempPath, 0777 - umask()); + + file_put_contents($tempPath, $content); + + rename($tempPath, $path); + } + /** * Prepend to a file. * diff --git a/src/Illuminate/Foundation/PackageManifest.php b/src/Illuminate/Foundation/PackageManifest.php index 5a86e5925e37..a091372a0ad2 100644 --- a/src/Illuminate/Foundation/PackageManifest.php +++ b/src/Illuminate/Foundation/PackageManifest.php @@ -97,7 +97,7 @@ protected function getManifest() $this->build(); } - $this->files->get($this->manifestPath, true); + $this->files->get($this->manifestPath); return $this->manifest = file_exists($this->manifestPath) ? $this->files->getRequire($this->manifestPath) : []; @@ -168,9 +168,8 @@ protected function write(array $manifest) throw new Exception('The '.dirname($this->manifestPath).' directory must be present and writable.'); } - $this->files->put( - $this->manifestPath, 'files->replace( + $this->manifestPath, 'assertStringEqualsFile($this->tempDir.'/file.txt', 'Hello World'); } + public function testReplaceStoresFiles() + { + $tempFile = "{$this->tempDir}/file.txt"; + $symlinkDir = "{$this->tempDir}/symlink_dir"; + $symlink = "{$symlinkDir}/symlink.txt"; + + mkdir($symlinkDir); + symlink($tempFile, $symlink); + + // Prevent changes to symlink_dir + chmod($symlinkDir, 0555); + + // Test with a weird non-standard umask. + $umask = 0131; + $originalUmask = umask($umask); + + $filesystem = new Filesystem; + + // Test replacing non-existent file. + $filesystem->replace($tempFile, 'Hello World'); + $this->assertStringEqualsFile($tempFile, 'Hello World'); + $this->assertEquals($umask, 0777 - $this->getFilePermissions($tempFile)); + + // Test replacing existing file. + $filesystem->replace($tempFile, 'Something Else'); + $this->assertStringEqualsFile($tempFile, 'Something Else'); + $this->assertEquals($umask, 0777 - $this->getFilePermissions($tempFile)); + + // Test replacing symlinked file. + $filesystem->replace($symlink, 'Yet Something Else Again'); + $this->assertStringEqualsFile($tempFile, 'Yet Something Else Again'); + $this->assertEquals($umask, 0777 - $this->getFilePermissions($tempFile)); + + umask($originalUmask); + + // Reset changes to symlink_dir + chmod($symlinkDir, 0777 - $originalUmask); + } + public function testSetChmod() { file_put_contents($this->tempDir.'/file.txt', 'Hello World'); @@ -496,4 +535,16 @@ public function testHash() $filesystem = new Filesystem; $this->assertEquals('acbd18db4cc2f85cedef654fccc4a4d8', $filesystem->hash($this->tempDir.'/foo.txt')); } + + /** + * @param string $file + * @return int + */ + private function getFilePermissions($file) + { + $filePerms = fileperms($file); + $filePerms = substr(sprintf('%o', $filePerms), -3); + + return (int) base_convert($filePerms, 8, 10); + } } From 68a85d55c74206b9db6bb36cdb6cdac45c0f0895 Mon Sep 17 00:00:00 2001 From: Alwin Garside Date: Fri, 26 Oct 2018 22:23:08 +0200 Subject: [PATCH 2/3] Fix syntax --- tests/Filesystem/FilesystemTest.php | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/Filesystem/FilesystemTest.php b/tests/Filesystem/FilesystemTest.php index 4bd6ad8f0f30..a0f7edfafc29 100755 --- a/tests/Filesystem/FilesystemTest.php +++ b/tests/Filesystem/FilesystemTest.php @@ -44,9 +44,9 @@ public function testPutStoresFiles() public function testReplaceStoresFiles() { - $tempFile = "{$this->tempDir}/file.txt"; + $tempFile = "{$this->tempDir}/file.txt"; $symlinkDir = "{$this->tempDir}/symlink_dir"; - $symlink = "{$symlinkDir}/symlink.txt"; + $symlink = "{$symlinkDir}/symlink.txt"; mkdir($symlinkDir); symlink($tempFile, $symlink); @@ -55,7 +55,7 @@ public function testReplaceStoresFiles() chmod($symlinkDir, 0555); // Test with a weird non-standard umask. - $umask = 0131; + $umask = 0131; $originalUmask = umask($umask); $filesystem = new Filesystem; From 8bb1ff9fa876bf2e8222725a25a8aa279634bbd8 Mon Sep 17 00:00:00 2001 From: Alwin Garside Date: Sun, 28 Oct 2018 08:26:25 +0100 Subject: [PATCH 3/3] formatting --- src/Illuminate/Filesystem/Filesystem.php | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/src/Illuminate/Filesystem/Filesystem.php b/src/Illuminate/Filesystem/Filesystem.php index b02b161f2b75..a4a3448c3a24 100644 --- a/src/Illuminate/Filesystem/Filesystem.php +++ b/src/Illuminate/Filesystem/Filesystem.php @@ -127,16 +127,17 @@ public function put($path, $contents, $lock = false) * * This will replace the target file permissions. It also resolves symlinks to replace the symlink's target file. * - * @param string $path - * @param string $content + * @param string $path + * @param string $content + * @return void */ public function replace($path, $content) { - // Just in case path already exists and is a symlink, we want to make sure we get the real path. + // If the path already exists and is a symlink, make sure we get the real path... clearstatcache(true, $path); + $path = realpath($path) ?: $path; - // Write out the contents to a temp file, so we then can rename the file atomically. $tempPath = tempnam(dirname($path), basename($path)); // Fix permissions of tempPath because `tempnam()` creates it with permissions set to 0600.