Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions changelog/unreleased/41835
Original file line number Diff line number Diff line change
@@ -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
12 changes: 10 additions & 2 deletions lib/private/Preview/Bitmap.php
Original file line number Diff line number Diff line change
Expand Up @@ -45,17 +45,25 @@ 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 {
$bp = $this->getResizedPreview($stream, $maxX, $maxY);
} 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);
Expand Down
86 changes: 86 additions & 0 deletions tests/lib/Preview/BitmapStreamTest.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,86 @@
<?php
/**
* @author Thomas Müller <thomas.mueller@tmit.eu>
*
* @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 <http://www.gnu.org/licenses/>
*
*/

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('<?xml version="1.0"?><notanimage>x</notanimage>');

$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));
}
}