fix: release the stream when a bitmap preview cannot be decoded [10.16]

[oc-tmueller]

10.16 backport of #41835.

Description

Bitmap::getThumbnail() opens the file and closes it only on the success path — the catch around getResizedPreview() returns before the fclose(). Every failed decode therefore leaks one file descriptor for the lifetime of the process.

A failed decode is not an edge case: any content ImageMagick has no coder for lands in that catch. So occ preview pre-generation, or a cron preview job, over a directory of .heic/.psd files leaks a handle per file until EMFILE.

fclose() moves into a finally, so it runs on both paths.

Second defect, same three lines — and it behaves differently on 7.4

$file->fopen('r') was unchecked. This is where the backport genuinely diverges from #41835, so the wording of the commit message, the changelog and one test was adapted rather than cherry-picked verbatim.

On PHP 8, which master runs, stream_get_contents(false) raises a TypeError — an \Error, so it escapes the catch (\Exception) directly underneath and surfaces as a 500. On PHP 7.4, the only version this branch supports, the same call merely warns and hands on false. Probed directly on both images:

PHP 7.4.33  WARNING: stream_get_contents() expects parameter 1 to be resource, bool given
            returned: false
PHP 8.3.31  threw TypeError: stream_get_contents(): Argument #1 ($stream) must be of type resource, false given

Tracing the unfixed 10.16 path for a storage that returns false, the false is coerced by the sanitizer and Imagick rejects the empty string:

PHP WARNING: stream_get_contents() expects parameter 1 to be resource, bool given
caught \Exception -> logs 'ImageMagick says: Zero size image string passed' and returns false

So on 10.16 this is not a 500 — it is a spurious warning plus a log line blaming ImageMagick for a file that was never opened. Handling the false explicitly replaces both with one accurate message, and keeps the line correct if it is ever run on PHP 8. (DOMDocument::loadXML() warns as well, but behind @, and OC\Log\ErrorHandler::onError() drops @-suppressed diagnostics, so only one warning reaches owncloud.log outside debug mode — which is why the changelog says "an unrelated warning" where #41835's says nothing about warnings at all.)

Tests

tests/lib/Preview/BitmapStreamTest.php, 3 cases. It asserts the contract via is_resource($stream) on the caller's own handle rather than counting descriptors, so it is portable rather than Linux-only.

The second commit reworks two of the three cases, because as cherry-picked they did not hold on 7.4:

• The unopenable-file case could not fail. It asserted the return value, but unpatched 10.16 already returns false there (per the trace above) — verified by reverting the fix and watching it pass. What the guard actually removes on 7.4 is the warning, so it now asserts that none is emitted, and is renamed to match. The handler honours error_reporting(), so @-suppressed diagnostics from anywhere in the path cannot fail it; the un-suppressed warning alone detects the regression.
• The undecodable-content payload was delegate-dependent. <?xml …?><notanimage> is sniffed as SVG and throws only because neither owncloudci/php:7.4 nor :8.3 registers an SVG delegate (no decode delegate … 'SVG'). On a build with librsvg or the internal MSVG renderer, that lenient parser returns a blank canvas, the decode succeeds and the case fails. Replaced with content no coder claims at all, confirmed to be reported as format '' rather than 'SVG' on both images.

Confirmed RED before the fix on owncloudci/php:7.4: 2 failures — the unclosed stream, and the warning recorded verbatim — with testClosesTheStreamOnSuccess passing unfixed as the control. GREEN after: 3 tests, 6 assertions.

Verification

All on owncloudci/php:7.4, PHP 7.4.33, imagick 3.8.1 / ImageMagick 6.9.11-60:

• tests/lib/Preview/BitmapStreamTest.php: 3 tests, 6 assertions, 0 failures.
• tests/lib/Preview/: 41 tests, 0 failures, 18 skipped — against a pristine-branch baseline of 38 tests / 18 skipped. Exactly the 3 new tests, no new skips. (All 18 skips are the pre-existing No SVG provider present; this image registers no SVG coder. Assertion totals drift by a couple between runs because tests/lib/Preview/Provider.php feeds random_int() dimensions.)
• make test-php-style: php-cs-fixer 0 of 2410 files, phpcs clean, OCPSinceChecker OK.
• make test-php-phan: exit 0 over 1440 files, nothing reported. Worth running separately from master, since this branch pins phan ^5.4 (5.5.2) where master pins ^6.0.7.
• phpstan (1.12.34, the version this branch locks): 311 findings both with and without the change — identical counts, none in lib/private/Preview/Bitmap.php. That count is an artefact of my local lib/composer state; CI reports "No errors" over the same 1497 files.
• php -l clean on both changed files.

Diff vs master

Only the (string)$bp cast is absent — master gained it in #41449 (PHP 8.3 support), 10.16 keeps loadFromData($bp). It is context, not touched by this change; the 3-way cherry-pick preserved the 10.16 form.

Related

• fix: release the stream when a bitmap preview cannot be decoded #41835 (master, merged)
• fix: prevent arbitrary file write via unsanitized SVG/MVG bitmap previews [10.16] (OC10-164) #41828 — touches getResizedPreview() and the use block only, with no textual overlap with getThumbnail(), so it needs no rebase after this lands. The two are complementary: the \RuntimeException fix: prevent arbitrary file write via unsanitized SVG/MVG bitmap previews [10.16] (OC10-164) #41828 throws out of getResizedPreview() reaches the same catch, and is now covered by the finally.

The same class of leak still exists next door — Preview\Image::getThumbnail() returns early on rejected dimensions without reaching its fclose(), and SVG/TXT also feed an unchecked fopen() into stream_get_contents(). #41835 left those alone too; keeping this backport minimal, they deserve their own issue.

🤖 Generated with Claude Code