fix: release the stream when a bitmap preview cannot be decoded - #41835
Merged
Merged
Conversation
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) raises a TypeError - an \Error, so it escapes the \Exception handler directly underneath and surfaces as a 500 instead of the missing preview every other failure here degrades to. Closing moves into a finally block, and a false return from fopen() is handled explicitly. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com>
This comment was marked as resolved.
This comment was marked as resolved.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com>
phil-davis
approved these changes
Sep 16, 2026
This was referenced Sep 16, 2026
phil-davis
pushed a commit
that referenced
this pull request
Sep 16, 2026
…6] (#41837) * fix: release the stream when a bitmap preview cannot be decoded [10.16] 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 0d0a306) Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com> * test: make the backported stream tests detect their defects on PHP 7.4 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) <noreply@anthropic.com> Signed-off-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com> --------- Signed-off-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Co-authored-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Bitmap::getThumbnail()opens the file and closes it only on the success path — thecatcharoundgetResizedPreview()returns before thefclose(). Every failed decode therefore leaks one file descriptor for the lifetime of the process.Flagged as out of scope in #41827 ("that's a resource leak, not a security defect, and deserves its own focused PR"), so this is that PR. It targets
masterand is independent of the #41827/#41834 chain.Why it matters more than it looks
A failed decode is not an edge case. Any content ImageMagick has no coder for lands in that
catch, and #41834 makes throwing a designed outcome — if a build does not register a provider's coder, the pin throws rather than falling back to content sniffing. Sooccpreview pre-generation, or a cron preview job, over a directory of.heic/.psdfiles leaks a handle per file untilEMFILE.Second defect, same three lines
$file->fopen('r')was unchecked. A storage that cannot open the file returnsfalse, andstream_get_contents(false)raises aTypeError— an\Error, so it escapes thecatch (\Exception)directly underneath and surfaces as a 500 instead of the missing preview every other failure here degrades to. Confirmed, not theorised:What changed
fclose()moves into afinally, so it runs on both paths.falsereturn fromfopen()is handled explicitly and logged.Tests
New
tests/lib/Preview/BitmapStreamTest.php, 3 cases. It asserts the contract directly viais_resource($stream)on the caller's own handle rather than counting descriptors, so it is portable rather than Linux-only.Confirmed RED before the fix on
owncloudci/php:8.3— 1 failure (the stream must be closed on the failure path) and 1 error (theTypeErrorabove); the success-path case passed unfixed, as the control. GREEN after: 3 tests, 5 assertions.Verification
tests/lib/Preview/onowncloudci/php:8.3: 55 tests, 152 assertions, 0 failures.make test-php-style: 0 of 2435 files need fixing.php -lclean under PHP 7.4, so this backports to the 10.x line without syntax changes.Note on sequencing
#41827 modifies these same lines. Whichever lands second needs a trivial rebase — the conflict is confined to the
try/catch/fcloseblock.🤖 Generated with Claude Code