From 06dcd1c7625a1fa71b435d0155622a6d97393a49 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thomas=20M=C3=BCller?= Date: Wed, 16 Sep 2026 13:41:19 +0200 Subject: [PATCH 1/2] fix: release the stream when a bitmap preview cannot be decoded [10.16] MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Bitmap::getThumbnail() opened the file and closed it only on the success path. The catch around getResizedPreview() returned without closing, so every failed decode leaked one file descriptor for the lifetime of the process. A preview pre-generation run or a cron preview job over a directory of files ImageMagick has no coder for exhausts the descriptors one file at a time. The open was also unchecked. A storage that cannot open the file returns false rather than throwing, and stream_get_contents(false) cannot report that: on the PHP 7.4 this branch runs on it warns and hands on false, so the real cause is only ever logged as "ImageMagick says: Zero size image string passed", behind an unrelated PHP warning. (On PHP 8, which master runs, the same call raises a TypeError - an \Error, so it escapes the \Exception handler directly underneath and surfaces as a 500. That difference is why the wording here and in the changelog deviates from #41835.) Closing moves into a finally block, and a false return from fopen() is handled explicitly. 10.16 backport of #41835. (cherry picked from commit 0d0a3064e73cccc682eebbdda673b09a1dbe32d5) Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com> --- changelog/unreleased/41835 | 12 ++++ lib/private/Preview/Bitmap.php | 13 +++- tests/lib/Preview/BitmapStreamTest.php | 86 ++++++++++++++++++++++++++ 3 files changed, 109 insertions(+), 2 deletions(-) create mode 100644 changelog/unreleased/41835 create mode 100644 tests/lib/Preview/BitmapStreamTest.php 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..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)); + } +} From 890e83a97d076ec5462a54037c597c7cfe3284d5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thomas=20M=C3=BCller?= <323649642+oc-tmueller@users.noreply.github.com> Date: Wed, 16 Sep 2026 14:50:02 +0200 Subject: [PATCH 2/2] test: make the backported stream tests detect their defects on PHP 7.4 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both cherry-picked test cases were written against PHP 8 and did not hold on the only version this branch supports. testReturnsFalseWhenTheFileCannotBeOpened asserted the return value, which cannot distinguish anything on 7.4: stream_get_contents(false) merely warns and hands on false, the sanitizer coerces it and Imagick rejects the empty string, so the unpatched code already returns false. The case passed with the fix reverted. What the guard actually removes on 7.4 is the noise - a warning from stream_get_contents(), and an "ImageMagick says:" line blaming ImageMagick for a file it never saw - so it now asserts that no warning is emitted, and is renamed accordingly. The handler honours error_reporting(), so diagnostics the code under test silenced with @ (the sanitizer's own loadXML warning, among any future ones) cannot fail the case; the un-suppressed warning alone detects the regression. testClosesTheStreamWhenDecodingThrows fed an XML payload, which ImageMagick sniffs as SVG. It throws here only because neither owncloudci/php:7.4 nor :8.3 registers an SVG delegate; on a build with librsvg or the internal MSVG renderer the lenient parser returns a blank canvas instead, the decode succeeds and the case fails. Replaced with content no coder claims at all, verified to be reported as format '' rather than format 'SVG' on both images. Confirmed both now fail without the fix - 2 failures, the second listing the warning verbatim - with the success-path case still passing as the control. Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com> --- tests/lib/Preview/BitmapStreamTest.php | 41 +++++++++++++++++++++----- 1 file changed, 34 insertions(+), 7 deletions(-) diff --git a/tests/lib/Preview/BitmapStreamTest.php b/tests/lib/Preview/BitmapStreamTest.php index 1ed03f9b176c..a5a02741a817 100644 --- a/tests/lib/Preview/BitmapStreamTest.php +++ b/tests/lib/Preview/BitmapStreamTest.php @@ -51,7 +51,10 @@ private function makeFile(string $content): array { * descriptors one file at a time. */ public function testClosesTheStreamWhenDecodingThrows(): void { - list($file, $stream) = $this->makeFile('x'); + // 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); @@ -70,17 +73,41 @@ public function testClosesTheStreamOnSuccess(): void { } /** - * 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. + * 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 testReturnsFalseWhenTheFileCannotBeOpened(): void { + 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'); - $this->assertFalse((new Photoshop())->getThumbnail($file, 32, 32, false)); + $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'); } }