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] 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); }