diff --git a/changelog/unreleased/41835 b/changelog/unreleased/41835 new file mode 100644 index 000000000000..c98de24f308c --- /dev/null +++ b/changelog/unreleased/41835 @@ -0,0 +1,12 @@ +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. + +A file that cannot be opened at all is now reported as having no preview right +away, instead of travelling on until ImageMagick rejects the empty content and an +unrelated warning plus a misleading decoder error have been logged. + +https://github.com/owncloud/core/pull/41835 diff --git a/lib/private/Preview/Bitmap.php b/lib/private/Preview/Bitmap.php index e004f93c2cd1..038d65603ec5 100644 --- a/lib/private/Preview/Bitmap.php +++ b/lib/private/Preview/Bitmap.php @@ -45,6 +45,13 @@ public function getThumbnail(File $file, $maxX, $maxY, $scalingUp) { return false; } $stream = $file->fopen('r'); + if ($stream === false) { + // stream_get_contents() below cannot report this: on PHP 7.4 it warns and hands + // on false, so the failure is only noticed as a misleading decoder error later + // (and on PHP 8 it raises a TypeError, which escapes the handler underneath) + 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 +59,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($bp); diff --git a/tests/lib/Preview/BitmapStreamTest.php b/tests/lib/Preview/BitmapStreamTest.php new file mode 100644 index 000000000000..a5a02741a817 --- /dev/null +++ b/tests/lib/Preview/BitmapStreamTest.php @@ -0,0 +1,113 @@ + + * + * @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 { + // content no coder claims at all. An XML payload would be sniffed as SVG and only + // fail where that delegate is missing, so it would decode to a blank canvas - and + // fail this test - on a build with librsvg or the internal MSVG renderer enabled + list($file, $stream) = $this->makeFile('not-an-image-' . \str_repeat("\x00\xff", 16)); + + $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. On PHP 7.4 + * stream_get_contents(false) only warns and hands on false, which the sanitizer + * coerces and Imagick then rejects, so the return value alone cannot tell this apart + * from an undecodable file - it is false either way. The observable difference is the + * noise: unhandled, the attempt warns from stream_get_contents() before blaming + * ImageMagick for a file it never saw. (The sanitizer warns too, but behind @, so + * only the first is asserted on here.) + * + * (On PHP 8 the same call raises a TypeError instead - an \Error, so it escapes the + * \Exception handler in getThumbnail() and surfaces as a 500. That is the failure this + * guard prevents there, and why master's version of this test asserts the return.) + */ + public function testReportsNoPreviewWithoutWarningsWhenTheFileCannotBeOpened(): void { + $file = $this->createMock(File::class); + $file->method('fopen')->willReturn(false); + $file->method('getSize')->willReturn(1024); + $file->method('getPath')->willReturn('/test/unopenable'); + + $warnings = []; + \set_error_handler(function ($number, $string) use (&$warnings) { + // ignore what the code under test deliberately silenced with @, so that + // suppressed diagnostics from anywhere else in the path cannot fail this + if (!($number & \error_reporting())) { + return false; + } + $warnings[] = $string; + return true; + }); + try { + $result = (new Photoshop())->getThumbnail($file, 32, 32, false); + } finally { + \restore_error_handler(); + } + + $this->assertFalse($result, 'an unopenable file must not produce a preview'); + $this->assertSame([], $warnings, 'the failure must be handled, not warned about'); + } +}