diff --git a/changelog/unreleased/41835 b/changelog/unreleased/41835 new file mode 100644 index 000000000000..d60f03a150fb --- /dev/null +++ b/changelog/unreleased/41835 @@ -0,0 +1,11 @@ +Bugfix: Release the file handle when a bitmap preview cannot be decoded + +Bitmap previews closed the file they had opened only when decoding succeeded, so +every file that could not be decoded leaked a file handle for the lifetime of +the process. Generating previews for a directory of files that ImageMagick has +no decoder for could therefore exhaust the available file handles. + +Bitmap previews for a file that cannot be opened at all now report no preview +instead of failing the whole request. + +https://github.com/owncloud/core/pull/41835 diff --git a/lib/private/Preview/Bitmap.php b/lib/private/Preview/Bitmap.php index b7f783c5cb38..ac7b3ea08f11 100644 --- a/lib/private/Preview/Bitmap.php +++ b/lib/private/Preview/Bitmap.php @@ -45,6 +45,12 @@ public function getThumbnail(File $file, $maxX, $maxY, $scalingUp) { return false; } $stream = $file->fopen('r'); + if ($stream === false) { + // stream_get_contents() below would raise a TypeError, which is an \Error and + // so would escape the handler underneath rather than degrade to no preview + Util::writeLog('core', 'Could not open ' . $file->getPath() . ' for a preview', Util::ERROR); + return false; + } // Creates \Imagick object from bitmap or vector file try { @@ -52,10 +58,12 @@ public function getThumbnail(File $file, $maxX, $maxY, $scalingUp) { } catch (\Exception $e) { Util::writeLog('core', 'ImageMagick says: ' . $e->getmessage(), Util::ERROR); return false; + } finally { + // also on the failure path: any content ImageMagick has no coder for lands + // here, so leaking the handle would be routine rather than exceptional + \fclose($stream); } - \fclose($stream); - //new bitmap image object $image = new \OC_Image(); $image->loadFromData((string)$bp); diff --git a/tests/lib/Preview/BitmapStreamTest.php b/tests/lib/Preview/BitmapStreamTest.php new file mode 100644 index 000000000000..1ed03f9b176c --- /dev/null +++ b/tests/lib/Preview/BitmapStreamTest.php @@ -0,0 +1,86 @@ + + * + * @copyright Copyright (c) 2026, ownCloud GmbH + * @license AGPL-3.0 + * + * This code is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License, version 3, + * as published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Affero General Public License for more details. + * + * You should have received a copy of the GNU Affero General Public License, version 3, + * along with this program. If not, see + * + */ + +namespace Test\Preview; + +use OC\Preview\Photoshop; +use OCP\Files\File; +use Test\TestCase; + +/** + * @requires extension imagick + */ +class BitmapStreamTest extends TestCase { + /** + * @return array{0: File, 1: resource} + */ + private function makeFile(string $content): array { + $stream = \fopen('php://memory', 'rb+'); + \fwrite($stream, $content); + \rewind($stream); + $file = $this->createMock(File::class); + $file->method('fopen')->willReturn($stream); + $file->method('getSize')->willReturn(\strlen($content)); + $file->method('getPath')->willReturn('/test/bitmap-stream'); + return [$file, $stream]; + } + + /** + * getResizedPreview() throwing is a routine outcome rather than an exceptional one - + * any content ImageMagick has no coder for reaches it - so the handle has to be + * released on that path too. Otherwise a preview pre-generation run, or a cron + * preview job, over a directory of undecodable files exhausts the process's + * descriptors one file at a time. + */ + public function testClosesTheStreamWhenDecodingThrows(): void { + list($file, $stream) = $this->makeFile('x'); + + $result = (new Photoshop())->getThumbnail($file, 32, 32, false); + + $this->assertFalse($result, 'undecodable content must not produce a preview'); + $this->assertFalse(\is_resource($stream), 'the stream must be closed on the failure path'); + } + + public function testClosesTheStreamOnSuccess(): void { + $png = \file_get_contents(\OC::$SERVERROOT . '/tests/data/testimage.png'); + list($file, $stream) = $this->makeFile($png); + + $result = (new Photoshop())->getThumbnail($file, 32, 32, false); + + $this->assertNotFalse($result, 'a PNG should still decode'); + $this->assertFalse(\is_resource($stream), 'the stream must be closed on the success path'); + } + + /** + * A storage that cannot open the file returns false rather than throwing, and + * stream_get_contents(false) raises a TypeError - an \Error, so it would escape the + * \Exception handler in getThumbnail() and surface as a 500 instead of a missing + * preview. + */ + public function testReturnsFalseWhenTheFileCannotBeOpened(): void { + $file = $this->createMock(File::class); + $file->method('fopen')->willReturn(false); + $file->method('getSize')->willReturn(1024); + $file->method('getPath')->willReturn('/test/unopenable'); + + $this->assertFalse((new Photoshop())->getThumbnail($file, 32, 32, false)); + } +}