MDL-84010 core_files: sync_external_file can cause infinite recursion

sammarshallou · Dec 16, 2024 · e599ccc · e599ccc
1 parent 0888a6d
commit e599ccc
Show file tree

Hide file tree

Showing 2 changed files with 104 additions and 8 deletions.
diff --git a/lib/filestorage/tests/stored_file_test.php b/lib/filestorage/tests/stored_file_test.php
@@ -117,4 +117,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
@@ -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;
         }