Uh oh!
There was an error while loading. Please reload this page.
Fix unzipping 4GB+ zip files#77181
Uh oh!
There was an error while loading. Please reload this page.
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -199,30 +199,55 @@ private static bool TryGetZip64BlockFromGenericExtraField(ZipGenericExtraField e | ||
| if (extraField.Size < sizeof(long)) | ||
| return true; | ||
| // Advancing the stream (by reading from it) is possible only when: | ||
| // 1. There is an explicit ask to do that (valid files, corresponding boolean flag(s) set to true). | ||
| // 2. When the size indicates that all the information is available ("slightly invalid files"). | ||
| bool readAllFields = extraField.Size >= sizeof(long) + sizeof(long) + sizeof(long) + sizeof(int); | ||
| if (readUncompressedSize) | ||
| { | ||
| zip64Block._uncompressedSize = reader.ReadInt64(); | ||
| } | ||
| else if (readAllFields) | ||
| { | ||
| _ = reader.ReadInt64(); | ||
| } | ||
| if (ms.Position > extraField.Size - sizeof(long)) | ||
| return true; | ||
| if (readCompressedSize) | ||
| { | ||
| zip64Block._compressedSize = reader.ReadInt64(); | ||
| } | ||
| else if (readAllFields) | ||
| { | ||
| _ = reader.ReadInt64(); | ||
| } | ||
| if (ms.Position > extraField.Size - sizeof(long)) | ||
| return true; | ||
jozkee marked this conversation as resolved.
Uh oh!There was an error while loading. Please reload this page. | ||
| if (readLocalHeaderOffset) | ||
| { | ||
| zip64Block._localHeaderOffset = reader.ReadInt64(); | ||
| } | ||
| else if (readAllFields) | ||
| { | ||
| _ = reader.ReadInt64(); | ||
| } | ||
| if (ms.Position > extraField.Size - sizeof(int)) | ||
| return true; | ||
| if (readStartDiskNumber) | ||
| { | ||
| zip64Block._startDiskNumber = reader.ReadInt32(); | ||
| } | ||
| else if (readAllFields) | ||
| { | ||
| _ = reader.ReadInt32(); | ||
| } | ||
| // original values are unsigned, so implies value is too big to fit in signed integer | ||
| if (zip64Block._uncompressedSize < 0) throw new InvalidDataException(SR.FieldTooBigUncompressedSize); | ||
Member There was a problem hiding this comment. These validations only occur if extraField.Size >= 28 since you are now returning if there's less than that. Shouldn't we validate when MemberAuthor There was a problem hiding this comment. This is how it always was, I would prefer not to do it now as I intend to backport it to 7.0 and trying to make it as defensive/safe as possible. Member There was a problem hiding this comment. The reason I was asking is because it wasn't always like that. It was changed to how it is now on #68106, which is the PR that introduced the regression. | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,48 @@ | ||
| // Licensed to the .NET Foundation under one or more agreements. | ||
| // The .NET Foundation licenses this file to you under the MIT license. | ||
| using System.Collections.Generic; | ||
| using System.Linq; | ||
| using Xunit; | ||
| namespace System.IO.Compression.Tests | ||
| { | ||
| [Collection(nameof(DisableParallelization))] | ||
| public class zip_LargeFiles : ZipFileTestBase | ||
| { | ||
| [ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsSpeedOptimized), nameof(PlatformDetection.Is64BitProcess))] // don't run it on slower runtimes | ||
| [OuterLoop("It requires almost 12 GB of free disk space")] | ||
| public static void UnzipOver4GBZipFile() | ||
adamsitnik marked this conversation as resolved.
Uh oh!There was an error while loading. Please reload this page. | ||
| { | ||
| byte[] buffer = GC.AllocateUninitializedArray<byte>(1_000_000_000); // 1 GB | ||
Member There was a problem hiding this comment. Technically, this is not 1Gb, but since you are creating 6 files, the test is still valid. I would rather create two tests, one that succeeds at the limit (say 3.999 Gb) without the fix and one that fails and then verify that both pass with your fix. Since this is urgent for a backport, we can defer that. MemberAuthor There was a problem hiding this comment.
You mean that
My typical workflow is to add test that fails first, then make it pass. We have plenty of tests that verify < 4 GB archives so I don't see the value in adding it (also it would require us to use even more disk space and take longer to execute). In the long term we could add more tests, especially such that verify that Member There was a problem hiding this comment.
Yes.
I meant to verify on the edge cases of the bug. In this case that would be 4Gb - 1, 4Gb, and 4Gb + 1; rather than a 6Gb case. But yes, that would multiply test execution time which is already quite large with this new test. Member There was a problem hiding this comment. Is there a reasonable chance this will OOM? If so, consider catching that and skipping the test, rather than failing it. MemberAuthor There was a problem hiding this comment.
It should be executed only in Outerloop runs for the 64 bit "fast runtimes" and not in parallel with other tests so I don't think it's needed (at least for now). runtime/src/libraries/Common/tests/TestUtilities/System/PlatformDetection.cs Lines 105 to 106 in 98a89c9 | ||
| string zipArchivePath = Path.Combine(Path.GetTempPath(), "over4GB.zip"); | ||
| DirectoryInfo tempDir = Directory.CreateDirectory(Path.Combine(Path.GetTempPath(), "over4GB")); | ||
| try | ||
| { | ||
| for (byte i = 0; i < 6; i++) | ||
| { | ||
| File.WriteAllBytes(Path.Combine(tempDir.FullName, $"{i}.test"), buffer); | ||
| } | ||
| ZipFile.CreateFromDirectory(tempDir.FullName, zipArchivePath, CompressionLevel.NoCompression, includeBaseDirectory: false); | ||
| using ZipArchive zipArchive = ZipFile.OpenRead(zipArchivePath); | ||
| foreach (ZipArchiveEntry entry in zipArchive.Entries) | ||
| { | ||
| using Stream entryStream = entry.Open(); | ||
| Assert.True(entryStream.CanRead); | ||
| Assert.Equal(buffer.Length, entryStream.Length); | ||
| } | ||
| } | ||
| finally | ||
| { | ||
| File.Delete(zipArchivePath); | ||
| tempDir.Delete(recursive: true); | ||
| } | ||
| } | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For my own education: is there a perf improvement when using
_ = ReadInt64();to discard the value, compared to just not using_? Or is the idea here to explicitly signal that we are ignoring the value on purpose?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It shouldn't have a runtime effect, it's self documenting, both for humans and also for linters/analyzers that might otherwise flag an unused method result.