Validate uncompressed size up front in ZipArchiveEntry update mode#128319
Validate uncompressed size up front in ZipArchiveEntry update mode#128319Copilot wants to merge 3 commits into
Conversation
…127834) Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/d2fb03b5-3b19-489d-9856-bb2c42a4d7e4 Co-authored-by: alinpahontu2912 <56953855+alinpahontu2912@users.noreply.github.com>
|
Tagging subscribers to this area: @karelz, @dotnet/area-system-io-compression |
|
Hey @copilot, I think the fix should be different. We shouldn’t rely on the MemoryStream hitting its max capacity to fail. Instead, we should validate the uncompressedSize up front in ZipArchiveEntry and ensure it falls within the valid range (0 to Array.MaxLength). If it doesn’t, we should throw immediately when opening the entry in update mode. |
Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/0fbb8331-f2e4-4bd0-acb1-a58139224285 Co-authored-by: alinpahontu2912 <56953855+alinpahontu2912@users.noreply.github.com>
Switched to up-front validation in ff62bbb. |
There was a problem hiding this comment.
Pull request overview
This PR tightens validation for ZipArchiveEntry when opening entries in Update mode by rejecting entries whose declared uncompressed size can’t be represented by the in-memory MemoryStream buffer used for Update-mode editing, and adds a regression test to cover the exception behavior for both sync and async open paths.
Changes:
- Add an Update-mode openability check that fails early when
_uncompressedSizeexceedsArray.MaxLength, returningInvalidDataExceptioninstead of allowing an(int)cast to wrap and triggerMemoryStream(int)ArgumentOutOfRangeException. - Update comments in sync/async
GetUncompressedData*to reflect the new upstream validation. - Add a regression test that forces
_uncompressedSize > Array.MaxLengthand assertsOpen()/OpenAsync()throwInvalidDataException.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/libraries/System.IO.Compression/src/System/IO/Compression/ZipArchiveEntry.cs | Adds upfront Update-mode validation for _uncompressedSize and updates the GetUncompressedData comment accordingly. |
| src/libraries/System.IO.Compression/src/System/IO/Compression/ZipArchiveEntry.Async.cs | Updates the async GetUncompressedDataAsync comment to match the new validation behavior. |
| src/libraries/System.IO.Compression/tests/ZipArchive/zip_InvalidParametersAndStrangeFiles.cs | Adds a regression test ensuring Update-mode Open/OpenAsync throws InvalidDataException for oversized declared uncompressed sizes. |
| if ((ulong)_uncompressedSize > (ulong)Array.MaxLength) | ||
| { | ||
| message = SR.EntryTooLarge; | ||
| return false; |
| // The uncompressed data is loaded into a MemoryStream, which is backed by a | ||
| // single byte[] and therefore cannot grow beyond Array.MaxLength. Reject | ||
| // up front rather than failing later from the MemoryStream constructor with | ||
| // a misleading argument-out-of-range exception (caused by the unchecked | ||
| // (int) cast in GetUncompressedData wrapping a long > int.MaxValue to a | ||
| // negative value). | ||
| if ((ulong)_uncompressedSize > (ulong)Array.MaxLength) |
Fixes #127834
ZipArchiveEntry.OpenthrowsArgumentOutOfRangeExceptionfrom theMemoryStreamconstructor whenever an entry's declared_uncompressedSizeexceedsArray.MaxLength: the unchecked(int)cast inGetUncompressedDatawraps negative, andMemoryStream(int capacity)validatescapacityis in[0, Array.MaxLength]. The accompanying comment claimed this was safe ("MemoryStreamwill just grow") — true on .NET Framework, no longer true on .NET Core.Description
ZipArchiveEntry.IsOpenableFinalVerifications: validate_uncompressedSizeup front when opening an entry in update mode. If_uncompressedSizefalls outside[0, Array.MaxLength]— the only range that can be loaded into aMemoryStream(which is backed by a singlebyte[]) — fail immediately withInvalidDataException(SR.EntryTooLarge). This is the shared verification helper called from bothThrowIfNotOpenable(sync) andThrowIfNotOpenableAsync(async), so it coversOpenInUpdateModeandOpenInUpdateModeAsyncwith a single check, rather than relying onMemoryStreamto fail later from the constructor or fromEnsureCapacityduringCopyTo.ZipArchiveEntry.GetUncompressedData(sync + async): simplified the capacity hint back tonew MemoryStream((int)_uncompressedSize). The(int)cast is now safe because_uncompressedSizeis validated upstream. The stale "MemoryStreamwill just grow" comment is replaced with one that points at the upstream validation.zip_InvalidParametersAndStrangeFiles.cs: reflectively forces_uncompressedSize > Array.MaxLengthand asserts that bothOpenandOpenAsyncthrowInvalidDataExceptionimmediately in update mode (rather than the previouscapacity '-2147483549'ArgumentOutOfRangeExceptionfromMemoryStream).Scope / non-goals
Entries whose declared uncompressed size exceeds
Array.MaxLengthcannot be loaded in update mode —MemoryStreamis backed by a singlebyte[]and cannot grow beyondArray.MaxLength. Such entries are now rejected up front with a descriptiveInvalidDataException("Entries larger than 4GB are not supported in Update mode.")instead of surfacing as a misleadingArgumentOutOfRangeExceptionabout a negativecapacityargument from theMemoryStreamconstructor before any I/O happens. Lifting the underlying ceiling requires replacing the in-memory update buffer with a segmented or file-backed stream and is out of scope.Note
This pull request was authored by GitHub Copilot.