diff --git a/lib/filestorage/tests/stored_file_test.php b/lib/filestorage/tests/stored_file_test.php index ff2af34fb8887..3128b3f3f63cd 100644 --- a/lib/filestorage/tests/stored_file_test.php +++ b/lib/filestorage/tests/stored_file_test.php @@ -121,4 +121,82 @@ public function test_get_psr_stream(): void { $stream->close(); } + /** + * If the data gets into an incorrect state where a file references itself, this should not + * get into endless recursion (stack overflow) but should throw an exception. + */ + public function test_sync_externalfile_with_recursive_reference(): void { + global $DB; + + $this->resetAfterTest(); + + $fs = get_file_storage(); + $filerecord = [ + 'contextid' => \context_system::instance()->id, + 'component' => 'core', + 'filearea' => 'unittest', + 'itemid' => 0, + 'filepath' => '/', + 'filename' => 'hello.txt', + ]; + $expectedstr = 'hello world'; + $file = $fs->create_file_from_string($filerecord, $expectedstr); + + $referenceid = $DB->get_field('repository_instances', 'id', ['typeid' => FILE_INTERNAL]); + $referencestr = \file_storage::pack_reference($filerecord); + $copyrecord = [ + 'contextid' => \context_system::instance()->id, + 'component' => 'core', + 'filearea' => 'unittest', + 'itemid' => 1, + 'filepath' => '/', + 'filename' => 'hello.txt', + ]; + $copy = $fs->create_file_from_reference($copyrecord, $referenceid, $referencestr); + + // Hack the original file so that it has the reference id to itself from the copy. + $DB->set_field('files', 'referencefileid', $copy->get_referencefileid(), ['id' => $file->get_id()]); + + // Now sync the original file. + $hackedfile = $fs->get_file_by_id($file->get_id()); + + try { + $hackedfile->sync_external_file(); + $this->fail('Should not work because this is a recursive reference'); + } catch (\moodle_exception $e) { + $this->assertStringContainsString('File references itself: ' . $file->get_id(), $e->getMessage()); + } + + // Create another file that references the copy. + $reference2str = \file_storage::pack_reference($copyrecord); + $copy2record = [ + 'contextid' => \context_system::instance()->id, + 'component' => 'core', + 'filearea' => 'unittest', + 'itemid' => 2, + 'filepath' => '/', + 'filename' => 'hello.txt', + ]; + $copy2 = $fs->create_file_from_reference($copy2record, $referenceid, $reference2str); + + // Now we change the original file to reference this second one - 2 levels of redirection. + $DB->set_field('files', 'referencefileid', $copy2->get_referencefileid(), ['id' => $file->get_id()]); + + // Again try to sync the original file. + $hackedfile = $fs->get_file_by_id($file->get_id()); + + try { + $hackedfile->sync_external_file(); + $this->fail('Should not work because this is a recursive reference'); + } catch (\moodle_exception $e) { + $this->assertStringContainsString('File references itself: ' . $file->get_id(), $e->getMessage()); + } + + // Put the hacked file back how it started so the situation is valid. + $DB->set_field('files', 'referencefileid', 0, ['id' => $file->get_id()]); + $copy2->sync_external_file(); + $copy->sync_external_file(); + $file->sync_external_file(); + } + } diff --git a/repository/lib.php b/repository/lib.php index 249b79eea460a..f767a08d29576 100644 --- a/repository/lib.php +++ b/repository/lib.php @@ -536,6 +536,9 @@ abstract class repository implements cacheable_object { /** @var bool true if the super construct is called, otherwise false. */ public $super_called; + /** @var array List of file ids currently being synced, to avoid endless recursion */ + protected static $syncfileids = []; + /** * Constructor * @@ -2759,14 +2762,29 @@ public function sync_reference(stored_file $file) { if ($file->get_referencelastsync()) { return false; } - $fs = get_file_storage(); - $params = file_storage::unpack_reference($file->get_reference(), true); - if (!is_array($params) || !($storedfile = $fs->get_file($params['contextid'], - $params['component'], $params['filearea'], $params['itemid'], $params['filepath'], - $params['filename']))) { - $file->set_missingsource(); - } else { - $file->set_synchronized($storedfile->get_contenthash(), $storedfile->get_filesize(), 0, $storedfile->get_timemodified()); + + if (in_array($file->get_id(), self::$syncfileids)) { + throw new \coding_exception('File references itself: ' . $file->get_id()); + } + try { + array_push(self::$syncfileids, $file->get_id()); + + $fs = get_file_storage(); + $params = file_storage::unpack_reference($file->get_reference(), true); + if (!is_array($params) || !($storedfile = $fs->get_file($params['contextid'], + $params['component'], $params['filearea'], $params['itemid'], $params['filepath'], + $params['filename']))) { + $file->set_missingsource(); + } else { + $file->set_synchronized( + $storedfile->get_contenthash(), + $storedfile->get_filesize(), + 0, + $storedfile->get_timemodified(), + ); + } + } finally { + array_pop(self::$syncfileids); } return true; }