From d91e2c15dbd501231e37efb338418d6b9014bc0c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Petr=20S=CC=8Ckoda?= Date: Thu, 18 Apr 2013 21:55:31 +0200 Subject: [PATCH 1/2] MDL-36959 rework adding of content files to the file pool This patch includes refreshing of borked files in file pool and basic prevention of race conditions. It also helps with diagnosing of file pool permission problems, detects coding errors and some other type of problems including sha1 collision jackpot. --- lang/en/error.php | 1 + lib/filestorage/file_storage.php | 149 +++++++++++++++++++++++-------- 2 files changed, 111 insertions(+), 39 deletions(-) diff --git a/lang/en/error.php b/lang/en/error.php index 66fc80a0da3..d98425b8645 100644 --- a/lang/en/error.php +++ b/lang/en/error.php @@ -472,6 +472,7 @@ $string['sslonlyaccess'] = 'For security reasons only https connections are allo $string['statscatchupmode'] = 'Statistics is currently in catchup mode. So far {$a->daysdone} day(s) have been processed and {$a->dayspending} are pending. Check back soon!'; $string['statsdisable'] = 'Statistics are not enabled.'; $string['statsnodata'] = 'There is no available data for that combination of course and time period'; +$string['storedfilecannotcreatefile'] = 'Can not create local file pool file, please verify permissions in dataroot and available disk space.'; $string['storedfilecannotcreatefiledirs'] = 'Can not create local file pool directories, please verify permissions in dataroot.'; $string['storedfilecannotread'] = 'Can not read file, either file does not exist or there are permission problems'; $string['storedfilenotcreated'] = 'Can not create file "{$a->contextid}/{$a->component}/{$a->filearea}/{$a->itemid}/{$a->filepath}/{$a->filename}"'; diff --git a/lib/filestorage/file_storage.php b/lib/filestorage/file_storage.php index ee29b75f6a1..e1dd7d6bedd 100644 --- a/lib/filestorage/file_storage.php +++ b/lib/filestorage/file_storage.php @@ -1558,40 +1558,90 @@ class file_storage { throw new file_exception('storedfilecannotread', '', $pathname); } - if (is_null($contenthash)) { - $contenthash = sha1_file($pathname); + $filesize = filesize($pathname); + if ($filesize === false) { + throw new file_exception('storedfilecannotread', '', $pathname); } - $filesize = filesize($pathname); + if (is_null($contenthash)) { + $contenthash = sha1_file($pathname); + } else if (debugging('', DEBUG_DEVELOPER)) { + $filehash = sha1_file($pathname); + if ($filehash === false) { + throw new file_exception('storedfilecannotread', '', $pathname); + } + if ($filehash !== $contenthash) { + // Hopefully this never happens, if yes we need to fix calling code. + debugging("Invalid contenthash submitted for file $pathname"); + $contenthash = $filehash; + } + } + if ($contenthash === false) { + throw new file_exception('storedfilecannotread', '', $pathname); + } + + if ($filesize > 0 and $contenthash === sha1('')) { + // Did the file change or is sha1_file() borked for this file? + clearstatcache(); + $contenthash = sha1_file($pathname); + $filesize = filesize($pathname); + + if ($contenthash === false or $filesize === false) { + throw new file_exception('storedfilecannotread', '', $pathname); + } + if ($filesize > 0 and $contenthash === sha1('')) { + // This is very weird... + throw new file_exception('storedfilecannotread', '', $pathname); + } + } $hashpath = $this->path_from_hash($contenthash); $hashfile = "$hashpath/$contenthash"; + $newfile = true; + if (file_exists($hashfile)) { - if (filesize($hashfile) !== $filesize) { + if (filesize($hashfile) === $filesize) { + return array($contenthash, $filesize, false); + } + if (sha1_file($hashfile) === $contenthash) { + // Jackpot! We have a sha1 collision. + mkdir("$this->filedir/jackpot/", $this->dirpermissions, true); + copy($hashfile, "$this->filedir/jackpot/{$contenthash}_1"); + copy($hashfile, "$this->filedir/jackpot/{$contenthash}_2"); throw new file_pool_content_exception($contenthash); } + debugging("Replacing invalid content file $contenthash"); + unlink($hashfile); $newfile = false; - - } else { - if (!is_dir($hashpath)) { - if (!mkdir($hashpath, $this->dirpermissions, true)) { - throw new file_exception('storedfilecannotcreatefiledirs'); // permission trouble - } - } - $newfile = true; - - if (!copy($pathname, $hashfile)) { - throw new file_exception('storedfilecannotread', '', $pathname); - } - - if (filesize($hashfile) !== $filesize) { - @unlink($hashfile); - throw new file_pool_content_exception($contenthash); - } - chmod($hashfile, $this->filepermissions); // fix permissions if needed } + if (!is_dir($hashpath)) { + if (!mkdir($hashpath, $this->dirpermissions, true)) { + // Permission trouble. + throw new file_exception('storedfilecannotcreatefiledirs'); + } + } + + // Let's try to prevent some race conditions. + + $prev = ignore_user_abort(true); + @unlink($hashfile.'.tmp'); + if (!copy($pathname, $hashfile.'.tmp')) { + // Borked permissions or out of disk space. + ignore_user_abort($prev); + throw new file_exception('storedfilecannotcreatefile'); + } + if (filesize($hashfile.'.tmp') !== $filesize) { + // This should not happen. + unlink($hashfile.'.tmp'); + ignore_user_abort($prev); + throw new file_exception('storedfilecannotcreatefile'); + } + rename($hashfile.'.tmp', $hashfile); + chmod($hashfile, $this->filepermissions); // Fix permissions if needed. + @unlink($hashfile.'.tmp'); // Just in case anything fails in a weird way. + ignore_user_abort($prev); return array($contenthash, $filesize, $newfile); } @@ -1609,30 +1659,51 @@ class file_storage { $hashpath = $this->path_from_hash($contenthash); $hashfile = "$hashpath/$contenthash"; + $newfile = true; if (file_exists($hashfile)) { - if (filesize($hashfile) !== $filesize) { + if (filesize($hashfile) === $filesize) { + return array($contenthash, $filesize, false); + } + if (sha1_file($hashfile) === $contenthash) { + // Jackpot! We have a sha1 collision. + mkdir("$this->filedir/jackpot/", $this->dirpermissions, true); + copy($hashfile, "$this->filedir/jackpot/{$contenthash}_1"); + file_put_contents("$this->filedir/jackpot/{$contenthash}_2", $content); throw new file_pool_content_exception($contenthash); } + debugging("Replacing invalid content file $contenthash"); + unlink($hashfile); $newfile = false; - - } else { - if (!is_dir($hashpath)) { - if (!mkdir($hashpath, $this->dirpermissions, true)) { - throw new file_exception('storedfilecannotcreatefiledirs'); // permission trouble - } - } - $newfile = true; - - file_put_contents($hashfile, $content); - - if (filesize($hashfile) !== $filesize) { - @unlink($hashfile); - throw new file_pool_content_exception($contenthash); - } - chmod($hashfile, $this->filepermissions); // fix permissions if needed } + if (!is_dir($hashpath)) { + if (!mkdir($hashpath, $this->dirpermissions, true)) { + // Permission trouble. + throw new file_exception('storedfilecannotcreatefiledirs'); + } + } + + // Hopefully this works around most potential race conditions. + + $prev = ignore_user_abort(true); + $newsize = file_put_contents($hashfile.'.tmp', $content, LOCK_EX); + if ($newsize === false) { + // Borked permissions most likely. + ignore_user_abort($prev); + throw new file_exception('storedfilecannotcreatefile'); + } + if (filesize($hashfile.'.tmp') !== $filesize) { + // Out of disk space? + unlink($hashfile.'.tmp'); + ignore_user_abort($prev); + throw new file_exception('storedfilecannotcreatefile'); + } + rename($hashfile.'.tmp', $hashfile); + chmod($hashfile, $this->filepermissions); // Fix permissions if needed. + @unlink($hashfile.'.tmp'); // Just in case anything fails in a weird way. + ignore_user_abort($prev); + return array($contenthash, $filesize, $newfile); } From fd4592bbbcbd65a69311addd06541f38206973ed Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Petr=20S=CC=8Ckoda?= Date: Sat, 20 Apr 2013 21:47:41 +0200 Subject: [PATCH 2/2] MDL-36959 test reworked adding of files to sha1 content pool --- lib/filestorage/tests/file_storage_test.php | 179 +++++++++++++++++++- 1 file changed, 174 insertions(+), 5 deletions(-) diff --git a/lib/filestorage/tests/file_storage_test.php b/lib/filestorage/tests/file_storage_test.php index b895e578bb5..709e886fc82 100644 --- a/lib/filestorage/tests/file_storage_test.php +++ b/lib/filestorage/tests/file_storage_test.php @@ -29,13 +29,168 @@ defined('MOODLE_INTERNAL') || die(); global $CFG; require_once($CFG->libdir . '/filelib.php'); require_once($CFG->dirroot . '/repository/lib.php'); +require_once($CFG->libdir . '/filestorage/stored_file.php'); class filestoragelib_testcase extends advanced_testcase { + /** + * Files can be created from strings. + */ + public function test_create_file_from_string() { + global $DB; + + $this->resetAfterTest(true); + + $this->assertEquals(0, $DB->count_records('files', array())); + + $content = 'abcd'; + $syscontext = context_system::instance(); + $filerecord = array( + 'contextid' => $syscontext->id, + 'component' => 'core', + 'filearea' => 'unittest', + 'itemid' => 0, + 'filepath' => '/images/', + 'filename' => 'testfile.txt', + ); + $pathhash = sha1('/'.$filerecord['contextid'].'/'.$filerecord['component'].'/'.$filerecord['filearea'].'/'.$filerecord['itemid'].$filerecord['filepath'].$filerecord['filename']); + + $fs = get_file_storage(); + $file = $fs->create_file_from_string($filerecord, $content); + + $this->assertInstanceOf('stored_file', $file); + $this->assertSame(sha1($content), $file->get_contenthash()); + $this->assertSame($pathhash, $file->get_pathnamehash()); + + $this->assertTrue($DB->record_exists('files', array('pathnamehash'=>$pathhash))); + + $location = test_stored_file_inspection::get_pretected_pathname($file); + + $this->assertFileExists($location); + + + // Verify the dir placeholder files are created. + $this->assertEquals(3, $DB->count_records('files', array())); + $this->assertTrue($DB->record_exists('files', array('pathnamehash'=>sha1('/'.$filerecord['contextid'].'/'.$filerecord['component'].'/'.$filerecord['filearea'].'/'.$filerecord['itemid'].'/.')))); + $this->assertTrue($DB->record_exists('files', array('pathnamehash'=>sha1('/'.$filerecord['contextid'].'/'.$filerecord['component'].'/'.$filerecord['filearea'].'/'.$filerecord['itemid'].$filerecord['filepath'].'.')))); + + + // Tests that missing content file is recreated. + + unlink($location); + $this->assertFileNotExists($location); + + $filerecord['filename'] = 'testfile2.txt'; + $file2 = $fs->create_file_from_string($filerecord, $content); + $this->assertInstanceOf('stored_file', $file2); + $this->assertSame($file->get_contenthash(), $file2->get_contenthash()); + $this->assertFileExists($location); + + $this->assertEquals(4, $DB->count_records('files', array())); + + + // Test that borked content file is recreated. + + $this->assertSame(2, file_put_contents($location, 'xx')); + + $filerecord['filename'] = 'testfile3.txt'; + $file3 = $fs->create_file_from_string($filerecord, $content); + $this->assertInstanceOf('stored_file', $file3); + $this->assertSame($file->get_contenthash(), $file3->get_contenthash()); + $this->assertFileExists($location); + + $this->assertSame($content, file_get_contents($location)); + $this->assertDebuggingCalled(); + + $this->assertEquals(5, $DB->count_records('files', array())); + } + /** * Local files can be added to the filepool */ public function test_create_file_from_pathname() { + global $CFG, $DB; + + $this->resetAfterTest(true); + + $filecount = $DB->count_records('files', array()); + $this->assertEquals(0, $filecount); + + $filepath = $CFG->dirroot.'/lib/filestorage/tests/fixtures/testimage.jpg'; + $syscontext = context_system::instance(); + $filerecord = array( + 'contextid' => $syscontext->id, + 'component' => 'core', + 'filearea' => 'unittest', + 'itemid' => 0, + 'filepath' => '/images/', + 'filename' => 'testimage.jpg', + ); + $pathhash = sha1('/'.$filerecord['contextid'].'/'.$filerecord['component'].'/'.$filerecord['filearea'].'/'.$filerecord['itemid'].$filerecord['filepath'].$filerecord['filename']); + + $fs = get_file_storage(); + $file = $fs->create_file_from_pathname($filerecord, $filepath); + + $this->assertInstanceOf('stored_file', $file); + $this->assertSame(sha1_file($filepath), $file->get_contenthash()); + + $this->assertTrue($DB->record_exists('files', array('pathnamehash'=>$pathhash))); + + $location = test_stored_file_inspection::get_pretected_pathname($file); + + $this->assertFileExists($location); + + + // Verify the dir placeholder files are created. + $this->assertEquals(3, $DB->count_records('files', array())); + $this->assertTrue($DB->record_exists('files', array('pathnamehash'=>sha1('/'.$filerecord['contextid'].'/'.$filerecord['component'].'/'.$filerecord['filearea'].'/'.$filerecord['itemid'].'/.')))); + $this->assertTrue($DB->record_exists('files', array('pathnamehash'=>sha1('/'.$filerecord['contextid'].'/'.$filerecord['component'].'/'.$filerecord['filearea'].'/'.$filerecord['itemid'].$filerecord['filepath'].'.')))); + + + // Tests that missing content file is recreated. + + unlink($location); + $this->assertFileNotExists($location); + + $filerecord['filename'] = 'testfile2.jpg'; + $file2 = $fs->create_file_from_pathname($filerecord, $filepath); + $this->assertInstanceOf('stored_file', $file2); + $this->assertSame($file->get_contenthash(), $file2->get_contenthash()); + $this->assertFileExists($location); + + $this->assertEquals(4, $DB->count_records('files', array())); + + + // Test that borked content file is recreated. + + $this->assertSame(2, file_put_contents($location, 'xx')); + + $filerecord['filename'] = 'testfile3.jpg'; + $file3 = $fs->create_file_from_pathname($filerecord, $filepath); + $this->assertInstanceOf('stored_file', $file3); + $this->assertSame($file->get_contenthash(), $file3->get_contenthash()); + $this->assertFileExists($location); + + $this->assertSame(file_get_contents($filepath), file_get_contents($location)); + $this->assertDebuggingCalled(); + + $this->assertEquals(5, $DB->count_records('files', array())); + + // Test invalid file creation. + + $filerecord['filename'] = 'testfile4.jpg'; + try { + $fs->create_file_from_pathname($filerecord, $filepath.'nonexistent'); + $this->fail('Exception expected when trying to add non-existent stored file.'); + } catch (Exception $e) { + $this->assertInstanceOf('file_exception', $e); + } + } + + /** + * Tests get get file. + */ + public function test_get_file() { global $CFG; $this->resetAfterTest(false); @@ -50,19 +205,28 @@ class filestoragelib_testcase extends advanced_testcase { 'filepath' => '/images/', 'filename' => 'testimage.jpg', ); + $pathhash = sha1('/'.$filerecord['contextid'].'/'.$filerecord['component'].'/'.$filerecord['filearea'].'/'.$filerecord['itemid'].$filerecord['filepath'].$filerecord['filename']); $fs = get_file_storage(); - $fs->create_file_from_pathname($filerecord, $filepath); + $file = $fs->create_file_from_pathname($filerecord, $filepath); - $this->assertTrue($fs->file_exists($syscontext->id, 'core', 'unittest', 0, '/images/', 'testimage.jpg')); + $this->assertInstanceOf('stored_file', $file); + $this->assertEquals($syscontext->id, $file->get_contextid()); + $this->assertEquals('core', $file->get_component()); + $this->assertEquals('unittest', $file->get_filearea()); + $this->assertEquals(0, $file->get_itemid()); + $this->assertEquals('/images/', $file->get_filepath()); + $this->assertEquals('testimage.jpg', $file->get_filename()); + $this->assertEquals(filesize($filepath), $file->get_filesize()); + $this->assertEquals($pathhash, $file->get_pathnamehash()); - return $fs->get_file($syscontext->id, 'core', 'unittest', 0, '/images/', 'testimage.jpg'); + return $file; } /** * Local images can be added to the filepool and their preview can be obtained * - * @depends test_create_file_from_pathname + * @depends test_get_file */ public function test_get_file_preview(stored_file $file) { global $CFG; @@ -1421,5 +1585,10 @@ class filestoragelib_testcase extends advanced_testcase { $this->setExpectedException('coding_exception'); $fs->get_unused_filename($contextid, $component, $filearea, $itemid, $filepath, ''); } - +} + +class test_stored_file_inspection extends stored_file { + public static function get_pretected_pathname(stored_file $file) { + return $file->get_pathname_by_contenthash(); + } }