Simplify ImageCropper crop: drop the leaking MemoryStream round-trip#1525
Draft
imnasnainaec wants to merge 1 commit into
Draft
Simplify ImageCropper crop: drop the leaking MemoryStream round-trip#1525imnasnainaec wants to merge 1 commit into
imnasnainaec wants to merge 1 commit into
Conversation
The MemoryStream fix for #1275 could not dispose the stream (GDI+ decodes lazily, so the stream had to outlive the returned Bitmap), which CodeQL flagged as a leak and which could accumulate orphaned streams across repeated crops. Drop the JPEG re-encode round-trip entirely. Its only effect was restoring the returned bitmap's RawFormat to Jpeg, and nothing reads that: PalasoImage.Save picks the format from the file extension. GetCroppedImage now copies the crop into a fresh Bitmap (new Bitmap(...)), which also severs the lazy reference the file-backed clone keeps to originalImage -- otherwise the caller's temp file stays locked when the crop is re-saved (as the Image setter does), the same defense SaveImageSafely already uses. Removes the now-dead _originalFormat field and its NRE-guard test. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
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
1 participant
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.
Simplifies the memory-leak concern raised on #1521 (CodeQL: MemoryStream created but not disposed; Devin: orphaned streams may accumulate across repeated crops).
What changed
GetCroppedImageno longer re-encodes the crop through aMemoryStream. That round-trip existed only to restore the returned bitmap'sRawFormatto JPEG, but nothing reads it —PalasoImage.Savepicks the format from the file extension, andSaveImageSafelyalready copies vianew Bitmap(Image)before saving.GetCroppedImagenow returns a stand-alonenew Bitmap(cropped):MemoryStream).Cloneof the file-backedoriginalImagekeeps a lazy link to its source, which left the caller's temp file locked when the crop was later re-saved (theImagesetter does exactly this). Copying into a freshBitmapbreaks that link, the same defensePalasoImage.SaveImageSafelyuses.Also removes the now-dead
_originalFormatfield and its NRE-guard test.Behavior change
The returned bitmap's
RawFormatis nowMemoryBmprather thanJpeg. This is not observed anywhere: save format is chosen by extension. Tests updated to reflect the new contract.Tests
All
ImageCropperTestspass locally (net8.0-windows), includingSetImage_ReCropPreviouslyCroppedJpeg_DoesNotThrow, the #1275 regression test.🤖 Generated with Claude Code
This change is