Skip to content

Commit 2b4ab6f

Browse files
nikaiwnikaiw
committed
fix(files): skip directory symlinks that loop back onto the scanned path
A directory symlink whose target is one of its own ancestors, e.g. `device -> ..` in a copied /sys tree or docker-ce's `bundles/latest -> .`, makes the scanner re-enter the same tree until the 4000 character path limit: .../4:0:0:0/bsg/4:0:0:0/device/bsg/4:0:0:0/device/... (~140 levels) Each level adds a filecache row, so scanning such a storage grows oc_filecache without bound and the background scan never finishes. In effect this is a denial of service: anything able to write symlinks to the backing storage (a build job, rsync, sftp) can grow the database until the disk is full. On one instance two stale build trees produced 30M bogus rows and a 185 GB database, and at their peak filled the node's disk, taking the server down. The bug has been open since 2017 (#6395, see also #20197 and #23022). The previous attempt, #21723, compared the logical path to the resolved one, which cannot match when an ancestor is itself a symlink, and was closed unmerged. Skip a child symlink in Local::getDirectoryContent() when its resolved target equals the listed directory or one of its ancestors, comparing resolved paths on both sides. This also catches indirect cycles (a -> b, b -> a). The loop above now stops where it closes, with a single log line instead of a runaway descent: Skipping looping directory symlink '.../4:0:0:0/bsg/4:0:0:0/device' -> '.../4:0:0:0' Only listing is affected; files behind such links stay readable and writable (see testDisallowSymlinksInsideDatadir). Signed-off-by: NK <nicolas.devillers@airbus.com>
1 parent 76dc4c7 commit 2b4ab6f

3 files changed

Lines changed: 199 additions & 0 deletions

File tree

‎lib/private/Files/Storage/Local.php‎

Lines changed: 67 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -539,6 +539,73 @@ public function hasUpdated(string $path, int $time): bool {
539539
}
540540
}
541541

542+
/**
543+
* Skips directory symlinks that resolve to the listed directory or one of
544+
* its ancestors: following such a link sends the scanner back into the tree
545+
* it is already walking, until the path length limit. Only the listing is
546+
* filtered; resolving a path still follows the link.
547+
*/
548+
#[\Override]
549+
public function getDirectoryContent(string $directory): \Traversable {
550+
$ancestors = null;
551+
foreach (parent::getDirectoryContent($directory) as $metadata) {
552+
if ($metadata['mimetype'] === FileInfo::MIMETYPE_FOLDER) {
553+
try {
554+
$childSource = $this->getSourcePath(rtrim($directory, '/') . '/' . $metadata['name']);
555+
} catch (ForbiddenException) {
556+
// Retargeted outside the datadir since listed; drop it like getMetaData() would.
557+
continue;
558+
}
559+
if (is_link($childSource)) {
560+
$childReal = realpath($childSource);
561+
if ($childReal !== false) {
562+
// Built lazily, only once a symlinked directory shows up.
563+
$ancestors ??= $this->getAncestorRealPaths($directory);
564+
if (isset($ancestors[rtrim($childReal, '/')])) {
565+
Server::get(LoggerInterface::class)->warning(
566+
"Skipping looping directory symlink '$childSource' -> '$childReal'",
567+
['app' => 'core']
568+
);
569+
continue;
570+
}
571+
}
572+
}
573+
}
574+
yield $metadata;
575+
}
576+
}
577+
578+
/**
579+
* Resolved paths of $directory and each of its ancestors up to the storage
580+
* root, as a set. A directory symlink resolving to any of them closes a loop.
581+
*/
582+
private function getAncestorRealPaths(string $directory): array {
583+
$root = rtrim($this->realDataDir, '/');
584+
$paths = [];
585+
try {
586+
$current = $this->getSourcePath(rtrim($directory, '/'));
587+
} catch (ForbiddenException) {
588+
// No resolvable ancestor chain: filter nothing.
589+
return $paths;
590+
}
591+
while (true) {
592+
$real = realpath($current);
593+
if ($real !== false) {
594+
$real = rtrim($real, '/');
595+
$paths[$real] = true;
596+
if ($real === $root) {
597+
break;
598+
}
599+
}
600+
$parent = dirname($current);
601+
if ($parent === $current || strlen($parent) < strlen($root)) {
602+
break;
603+
}
604+
$current = $parent;
605+
}
606+
return $paths;
607+
}
608+
542609
/**
543610
* Get the source path (on disk) of a given path
544611
*

‎tests/lib/Files/Cache/ScannerTest.php‎

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -452,4 +452,40 @@ public function testNoETagUnscannedSubFolder(): void {
452452
$newFolderEntry2 = $this->cache->get('folder/sub');
453453
$this->assertNotEquals($newFolderEntry2->getEtag(), $oldFolderEntry2->getEtag());
454454
}
455+
public function testScanSkipsSelfReferencingSymlink(): void {
456+
$root = rtrim($this->storage->getSourcePath(''), '/');
457+
mkdir($root . '/dir');
458+
file_put_contents($root . '/dir/real.txt', 'data');
459+
symlink($root . '/dir', $root . '/dir/self');
460+
461+
$this->scanner->scan('');
462+
463+
$this->assertTrue($this->cache->inCache('dir'));
464+
$this->assertTrue($this->cache->inCache('dir/real.txt'));
465+
// the loop itself is skipped, at every level
466+
$this->assertFalse($this->cache->inCache('dir/self'));
467+
$this->assertFalse($this->cache->inCache('dir/self/real.txt'));
468+
$this->assertFalse($this->cache->inCache('dir/self/self'));
469+
}
470+
471+
public function testScanSkipsIndirectSymlinkCycle(): void {
472+
$root = rtrim($this->storage->getSourcePath(''), '/');
473+
mkdir($root . '/x');
474+
mkdir($root . '/y');
475+
symlink($root . '/y', $root . '/x/toy');
476+
symlink($root . '/x', $root . '/y/tox');
477+
478+
$this->scanner->scan('');
479+
480+
$this->assertTrue($this->cache->inCache('x'));
481+
$this->assertTrue($this->cache->inCache('y'));
482+
// a link to a sibling is legitimate and stays visible...
483+
$this->assertTrue($this->cache->inCache('x/toy'));
484+
$this->assertTrue($this->cache->inCache('y/tox'));
485+
// ...but the walk stops where the cycle closes: entering x/toy lands
486+
// in y, whose link back to x would re-enter the path being walked
487+
$this->assertFalse($this->cache->inCache('x/toy/tox'));
488+
$this->assertFalse($this->cache->inCache('y/tox/toy'));
489+
}
490+
455491
}

‎tests/lib/Files/Storage/LocalTest.php‎

Lines changed: 96 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -280,4 +280,100 @@ public function testFopenRecoversFromStaleRealpathCache(string $staleName, strin
280280
$this->assertSame('abc', stream_get_contents($handle));
281281
fclose($handle);
282282
}
283+
private function collectNames(\Traversable $content): array {
284+
$names = [];
285+
foreach ($content as $metadata) {
286+
$names[] = $metadata['name'];
287+
}
288+
sort($names);
289+
return $names;
290+
}
291+
292+
public function testGetDirectoryContentSkipsSelfReferencingSymlink(): void {
293+
mkdir($this->tmpDir . 'dir');
294+
mkdir($this->tmpDir . 'dir/real');
295+
symlink($this->tmpDir . 'dir', $this->tmpDir . 'dir/self');
296+
297+
$storage = new Local(['datadir' => $this->tmpDir]);
298+
299+
$this->assertEquals(['real'], $this->collectNames($storage->getDirectoryContent('dir')));
300+
}
301+
302+
public function testGetDirectoryContentSkipsParentSymlink(): void {
303+
mkdir($this->tmpDir . 'a');
304+
mkdir($this->tmpDir . 'a/b');
305+
mkdir($this->tmpDir . 'a/b/real');
306+
symlink($this->tmpDir . 'a', $this->tmpDir . 'a/b/up');
307+
308+
$storage = new Local(['datadir' => $this->tmpDir]);
309+
310+
$this->assertEquals(['real'], $this->collectNames($storage->getDirectoryContent('a/b')));
311+
}
312+
313+
/**
314+
* a -> b while b -> a: neither target is an ancestor of its own parent, so a
315+
* parent-only check walks the pair forever.
316+
*/
317+
public function testGetDirectoryContentSkipsIndirectCycle(): void {
318+
mkdir($this->tmpDir . 'x');
319+
mkdir($this->tmpDir . 'y');
320+
symlink($this->tmpDir . 'y', $this->tmpDir . 'x/toy');
321+
symlink($this->tmpDir . 'x', $this->tmpDir . 'y/tox');
322+
323+
$storage = new Local(['datadir' => $this->tmpDir]);
324+
325+
// Entering x/toy lands in y; y's link back to x closes the cycle.
326+
$this->assertEquals([], $this->collectNames($storage->getDirectoryContent('x/toy')));
327+
}
328+
329+
/**
330+
* The loops we care about sit under ancestors that are themselves symlinks,
331+
* so the logical path and the resolved path are on different branches.
332+
* Comparing a logical path against a resolved one misses exactly this.
333+
*/
334+
public function testGetDirectoryContentSkipsLoopReachedThroughASymlinkedAncestor(): void {
335+
mkdir($this->tmpDir . 'real');
336+
mkdir($this->tmpDir . 'real/leaf');
337+
symlink($this->tmpDir . 'real', $this->tmpDir . 'alias');
338+
symlink($this->tmpDir . 'real', $this->tmpDir . 'real/leaf/back');
339+
340+
$storage = new Local(['datadir' => $this->tmpDir]);
341+
342+
// Reached as alias/leaf, whose resolved parent is real/leaf: 'back'
343+
// resolves to 'real', an ancestor, even though the logical path says alias/.
344+
$this->assertEquals([], $this->collectNames($storage->getDirectoryContent('alias/leaf')));
345+
}
346+
347+
public function testGetDirectoryContentKeepsNonLoopingSymlinks(): void {
348+
mkdir($this->tmpDir . 'a');
349+
mkdir($this->tmpDir . 'other');
350+
mkdir($this->tmpDir . 'other/deep');
351+
file_put_contents($this->tmpDir . 'other/f.txt', 'x');
352+
symlink($this->tmpDir . 'other', $this->tmpDir . 'a/sibling');
353+
symlink($this->tmpDir . 'other/deep', $this->tmpDir . 'a/deeper');
354+
symlink($this->tmpDir . 'other/f.txt', $this->tmpDir . 'a/afile');
355+
356+
$storage = new Local(['datadir' => $this->tmpDir]);
357+
358+
$this->assertEquals(
359+
['afile', 'deeper', 'sibling'],
360+
$this->collectNames($storage->getDirectoryContent('a'))
361+
);
362+
}
363+
364+
/**
365+
* Enumeration is filtered, resolution is not: a file behind a directory
366+
* symlink stays reachable. Guards the behaviour asserted by
367+
* testDisallowSymlinksInsideDatadir.
368+
*/
369+
public function testLoopingSymlinkStillResolvesForFileAccess(): void {
370+
mkdir($this->tmpDir . 'dir');
371+
symlink($this->tmpDir . 'dir', $this->tmpDir . 'dir/self');
372+
373+
$storage = new Local(['datadir' => $this->tmpDir]);
374+
$storage->file_put_contents('dir/self/foo', 'bar');
375+
376+
$this->assertEquals('bar', $storage->file_get_contents('dir/foo'));
377+
}
378+
283379
}

0 commit comments

Comments
 (0)