From: Eric StJohn Date: Mon, 30 Sep 2019 20:04:17 +0000 (-0700) Subject: Revert "Find zip file end of central directory backwards up to max possible size... X-Git-Tag: submit/tizen/20210909.063632~11031^2~387 X-Git-Url: http://review.tizen.org/git/?a=commitdiff_plain;h=c3252b3cbf960c970d2d922beeaf269be23088fd;p=platform%2Fupstream%2Fdotnet%2Fruntime.git Revert "Find zip file end of central directory backwards up to max possible size (dotnet/corefx#41007)" (dotnet/corefx#41449) This reverts commit dotnet/corefx@9d9c06f82eb4675a2aa56f07fe55a78039147784. Commit migrated from https://github.com/dotnet/corefx/commit/a9ebd33cbaef9b9eba9a0dbae178cc2814d62db4 --- diff --git a/src/libraries/System.IO.Compression/src/System/IO/Compression/ZipArchive.cs b/src/libraries/System.IO.Compression/src/System/IO/Compression/ZipArchive.cs index a714e04..a62b274 100644 --- a/src/libraries/System.IO.Compression/src/System/IO/Compression/ZipArchive.cs +++ b/src/libraries/System.IO.Compression/src/System/IO/Compression/ZipArchive.cs @@ -525,15 +525,9 @@ namespace System.IO.Compression { try { - // This seeks backwards almost to the beginning of the EOCD, one byte after where the signature would be - // located if the EOCD had the minimum possible size (no file zip comment) + // this seeks to the start of the end of central directory record _archiveStream.Seek(-ZipEndOfCentralDirectoryBlock.SizeOfBlockWithoutSignature, SeekOrigin.End); - - // If the EOCD has the minimum possible size (no zip file comment), then exactly the previous 4 bytes will contain the signature - // But if the EOCD has max possible size, the signature should be found somewhere in the previous 64K + 4 bytes - if (!ZipHelper.SeekBackwardsToSignature(_archiveStream, - ZipEndOfCentralDirectoryBlock.SignatureConstant, - ZipEndOfCentralDirectoryBlock.ZipFileCommentMaxLength + ZipEndOfCentralDirectoryBlock.SignatureSize)) + if (!ZipHelper.SeekBackwardsToSignature(_archiveStream, ZipEndOfCentralDirectoryBlock.SignatureConstant)) throw new InvalidDataException(SR.EOCDNotFound); long eocdStart = _archiveStream.Position; @@ -549,17 +543,56 @@ namespace System.IO.Compression _numberOfThisDisk = eocd.NumberOfThisDisk; _centralDirectoryStart = eocd.OffsetOfStartOfCentralDirectoryWithRespectToTheStartingDiskNumber; - if (eocd.NumberOfEntriesInTheCentralDirectory != eocd.NumberOfEntriesInTheCentralDirectoryOnThisDisk) throw new InvalidDataException(SR.SplitSpanned); - _expectedNumberOfEntries = eocd.NumberOfEntriesInTheCentralDirectory; // only bother saving the comment if we are in update mode if (_mode == ZipArchiveMode.Update) _archiveComment = eocd.ArchiveComment; - TryReadZip64EndOfCentralDirectory(eocd, eocdStart); + // only bother looking for zip64 EOCD stuff if we suspect it is needed because some value is FFFFFFFFF + // because these are the only two values we need, we only worry about these + // if we don't find the zip64 EOCD, we just give up and try to use the original values + if (eocd.NumberOfThisDisk == ZipHelper.Mask16Bit || + eocd.OffsetOfStartOfCentralDirectoryWithRespectToTheStartingDiskNumber == ZipHelper.Mask32Bit || + eocd.NumberOfEntriesInTheCentralDirectory == ZipHelper.Mask16Bit) + { + // we need to look for zip 64 EOCD stuff + // seek to the zip 64 EOCD locator + _archiveStream.Seek(eocdStart - Zip64EndOfCentralDirectoryLocator.SizeOfBlockWithoutSignature, SeekOrigin.Begin); + // if we don't find it, assume it doesn't exist and use data from normal eocd + if (ZipHelper.SeekBackwardsToSignature(_archiveStream, Zip64EndOfCentralDirectoryLocator.SignatureConstant)) + { + // use locator to get to Zip64EOCD + Zip64EndOfCentralDirectoryLocator locator; + bool zip64eocdLocatorProper = Zip64EndOfCentralDirectoryLocator.TryReadBlock(_archiveReader, out locator); + Debug.Assert(zip64eocdLocatorProper); // we just found this using the signature finder, so it should be okay + + if (locator.OffsetOfZip64EOCD > long.MaxValue) + throw new InvalidDataException(SR.FieldTooBigOffsetToZip64EOCD); + long zip64EOCDOffset = (long)locator.OffsetOfZip64EOCD; + + _archiveStream.Seek(zip64EOCDOffset, SeekOrigin.Begin); + + // read Zip64EOCD + Zip64EndOfCentralDirectoryRecord record; + if (!Zip64EndOfCentralDirectoryRecord.TryReadBlock(_archiveReader, out record)) + throw new InvalidDataException(SR.Zip64EOCDNotWhereExpected); + + _numberOfThisDisk = record.NumberOfThisDisk; + + if (record.NumberOfEntriesTotal > long.MaxValue) + throw new InvalidDataException(SR.FieldTooBigNumEntries); + if (record.OffsetOfCentralDirectory > long.MaxValue) + throw new InvalidDataException(SR.FieldTooBigOffsetToCD); + if (record.NumberOfEntriesTotal != record.NumberOfEntriesOnThisDisk) + throw new InvalidDataException(SR.SplitSpanned); + + _expectedNumberOfEntries = (long)record.NumberOfEntriesTotal; + _centralDirectoryStart = (long)record.OffsetOfCentralDirectory; + } + } if (_centralDirectoryStart > _archiveStream.Length) { @@ -576,63 +609,6 @@ namespace System.IO.Compression } } - // Tries to find the Zip64 End of Central Directory Locator, then the Zip64 End of Central Directory, assuming the - // End of Central Directory block has already been found, as well as the location in the stream where the EOCD starts. - private void TryReadZip64EndOfCentralDirectory(ZipEndOfCentralDirectoryBlock eocd, long eocdStart) - { - // Only bother looking for the Zip64-EOCD stuff if we suspect it is needed because some value is FFFFFFFFF - // because these are the only two values we need, we only worry about these - // if we don't find the Zip64-EOCD, we just give up and try to use the original values - if (eocd.NumberOfThisDisk == ZipHelper.Mask16Bit || - eocd.OffsetOfStartOfCentralDirectoryWithRespectToTheStartingDiskNumber == ZipHelper.Mask32Bit || - eocd.NumberOfEntriesInTheCentralDirectory == ZipHelper.Mask16Bit) - { - // Read Zip64 End of Central Directory Locator - - // This seeks forwards almost to the beginning of the Zip64-EOCDL, one byte after where the signature would be located - _archiveStream.Seek(eocdStart - Zip64EndOfCentralDirectoryLocator.SizeOfBlockWithoutSignature, SeekOrigin.Begin); - - // Exactly the previous 4 bytes should contain the Zip64-EOCDL signature - // if we don't find it, assume it doesn't exist and use data from normal EOCD - if (ZipHelper.SeekBackwardsToSignature(_archiveStream, - Zip64EndOfCentralDirectoryLocator.SignatureConstant, - Zip64EndOfCentralDirectoryLocator.SignatureSize)) - { - // use locator to get to Zip64-EOCD - Zip64EndOfCentralDirectoryLocator locator; - bool zip64eocdLocatorProper = Zip64EndOfCentralDirectoryLocator.TryReadBlock(_archiveReader, out locator); - Debug.Assert(zip64eocdLocatorProper); // we just found this using the signature finder, so it should be okay - - if (locator.OffsetOfZip64EOCD > long.MaxValue) - throw new InvalidDataException(SR.FieldTooBigOffsetToZip64EOCD); - - long zip64EOCDOffset = (long)locator.OffsetOfZip64EOCD; - - _archiveStream.Seek(zip64EOCDOffset, SeekOrigin.Begin); - - // Read Zip64 End of Central Directory Record - - Zip64EndOfCentralDirectoryRecord record; - if (!Zip64EndOfCentralDirectoryRecord.TryReadBlock(_archiveReader, out record)) - throw new InvalidDataException(SR.Zip64EOCDNotWhereExpected); - - _numberOfThisDisk = record.NumberOfThisDisk; - - if (record.NumberOfEntriesTotal > long.MaxValue) - throw new InvalidDataException(SR.FieldTooBigNumEntries); - - if (record.OffsetOfCentralDirectory > long.MaxValue) - throw new InvalidDataException(SR.FieldTooBigOffsetToCD); - - if (record.NumberOfEntriesTotal != record.NumberOfEntriesOnThisDisk) - throw new InvalidDataException(SR.SplitSpanned); - - _expectedNumberOfEntries = (long)record.NumberOfEntriesTotal; - _centralDirectoryStart = (long)record.OffsetOfCentralDirectory; - } - } - } - private void WriteFile() { // if we are in create mode, we always set readEntries to true in Init diff --git a/src/libraries/System.IO.Compression/src/System/IO/Compression/ZipBlocks.cs b/src/libraries/System.IO.Compression/src/System/IO/Compression/ZipBlocks.cs index 20fb1e7..bad189c 100644 --- a/src/libraries/System.IO.Compression/src/System/IO/Compression/ZipBlocks.cs +++ b/src/libraries/System.IO.Compression/src/System/IO/Compression/ZipBlocks.cs @@ -289,8 +289,6 @@ namespace System.IO.Compression internal struct Zip64EndOfCentralDirectoryLocator { public const uint SignatureConstant = 0x07064B50; - public const int SignatureSize = sizeof(uint); - public const int SizeOfBlockWithoutSignature = 16; public uint NumberOfDiskWithZip64EOCD; @@ -645,16 +643,7 @@ namespace System.IO.Compression internal struct ZipEndOfCentralDirectoryBlock { public const uint SignatureConstant = 0x06054B50; - public const int SignatureSize = sizeof(uint); - - // This is the minimum possible size, assuming the zip file comments variable section is empty public const int SizeOfBlockWithoutSignature = 18; - - // The end of central directory can have a variable size zip file comment at the end, but its max length can be 64K - // The Zip File Format Specification does not explicitly mention a max size for this field, but we are assuming this - // max size because that is the maximum value an ushort can hold. - public const int ZipFileCommentMaxLength = ushort.MaxValue; - public uint Signature; public ushort NumberOfThisDisk; public ushort NumberOfTheDiskWithTheStartOfTheCentralDirectory; @@ -684,7 +673,7 @@ namespace System.IO.Compression writer.Write(startOfCentralDirectoryTruncated); // Should be valid because of how we read archiveComment in TryReadBlock: - Debug.Assert((archiveComment == null) || (archiveComment.Length <= ZipFileCommentMaxLength)); + Debug.Assert((archiveComment == null) || (archiveComment.Length < ushort.MaxValue)); writer.Write(archiveComment != null ? (ushort)archiveComment.Length : (ushort)0); // zip file comment length if (archiveComment != null) diff --git a/src/libraries/System.IO.Compression/src/System/IO/Compression/ZipHelper.cs b/src/libraries/System.IO.Compression/src/System/IO/Compression/ZipHelper.cs index 22664cb..f8e3a3e 100644 --- a/src/libraries/System.IO.Compression/src/System/IO/Compression/ZipHelper.cs +++ b/src/libraries/System.IO.Compression/src/System/IO/Compression/ZipHelper.cs @@ -102,15 +102,11 @@ namespace System.IO.Compression return (uint)ret; } - // Assumes all bytes of signatureToFind are non zero, looks backwards from current position in stream, - // assumes maxBytesToRead is positive, ensures to not read beyond the provided max number of bytes, + // assumes all bytes of signatureToFind are non zero, looks backwards from current position in stream, // if the signature is found then returns true and positions stream at first byte of signature // if the signature is not found, returns false - internal static bool SeekBackwardsToSignature(Stream stream, uint signatureToFind, int maxBytesToRead) + internal static bool SeekBackwardsToSignature(Stream stream, uint signatureToFind) { - Debug.Assert(signatureToFind != 0); - Debug.Assert(maxBytesToRead > 0); - int bufferPointer = 0; uint currentSignature = 0; byte[] buffer = new byte[BackwardsSeekingBufferSize]; @@ -118,8 +114,7 @@ namespace System.IO.Compression bool outOfBytes = false; bool signatureFound = false; - int bytesRead = 0; - while (!signatureFound && !outOfBytes && bytesRead <= maxBytesToRead) + while (!signatureFound && !outOfBytes) { outOfBytes = SeekBackwardsAndRead(stream, buffer, out bufferPointer); @@ -137,8 +132,6 @@ namespace System.IO.Compression bufferPointer--; } } - - bytesRead += buffer.Length; } if (!signatureFound) diff --git a/src/libraries/System.IO.Compression/tests/ZipArchive/zip_InvalidParametersAndStrangeFiles.cs b/src/libraries/System.IO.Compression/tests/ZipArchive/zip_InvalidParametersAndStrangeFiles.cs index b08064f..cff07e0 100644 --- a/src/libraries/System.IO.Compression/tests/ZipArchive/zip_InvalidParametersAndStrangeFiles.cs +++ b/src/libraries/System.IO.Compression/tests/ZipArchive/zip_InvalidParametersAndStrangeFiles.cs @@ -3,9 +3,12 @@ // See the LICENSE file in the project root for more information. using System.Collections.Generic; +using System.Diagnostics; +using System.IO; using System.Text; using System.Threading.Tasks; using Xunit; +using Xunit.Sdk; namespace System.IO.Compression.Tests { @@ -757,94 +760,5 @@ namespace System.IO.Compression.Tests } } } - - /// - /// Opens an empty file that has a 64KB EOCD comment. - /// Adds two 64KB text entries. Verifies they can be read correctly. - /// Appends 64KB of garbage at the end of the file. Verifies we throw. - /// Prepends 64KB of garbage at the beginning of the file. Verifies we throw. - /// - [Fact] - public static void ReadArchive_WithEOCDComment_TrailingPrecedingGarbage() - { - void InsertEntry(ZipArchive archive, string name, string contents) - { - ZipArchiveEntry entry = archive.CreateEntry(name); - using (StreamWriter writer = new StreamWriter(entry.Open())) - { - writer.WriteLine(contents); - } - } - - int GetEntryContentsLength(ZipArchiveEntry entry) - { - int length = 0; - using (Stream stream = entry.Open()) - { - using (var reader = new StreamReader(stream)) - { - length = reader.ReadToEnd().Length; - } - } - return length; - } - - void VerifyValidEntry(ZipArchiveEntry entry, string expectedName, int expectedMinLength) - { - Assert.NotNull(entry); - Assert.Equal(expectedName, entry.Name); - // The file has a few more bytes, but should be at least as large as its contents - Assert.True(GetEntryContentsLength(entry) >= expectedMinLength); - } - - string name0 = "huge0.txt"; - string name1 = "huge1.txt"; - string str64KB = new string('x', ushort.MaxValue); - byte[] byte64KB = Text.Encoding.ASCII.GetBytes(str64KB); - - // Open empty file with 64KB EOCD comment - string path = strange("extradata/emptyWith64KBComment.zip"); - using (MemoryStream archiveStream = StreamHelpers.CreateTempCopyStream(path).Result) - { - // Insert 2 64KB txt entries - using (ZipArchive archive = new ZipArchive(archiveStream, ZipArchiveMode.Update, leaveOpen: true)) - { - InsertEntry(archive, name0, str64KB); - InsertEntry(archive, name1, str64KB); - } - - // Open and verify items - archiveStream.Seek(0, SeekOrigin.Begin); - using (ZipArchive archive = new ZipArchive(archiveStream, ZipArchiveMode.Read, leaveOpen: true)) - { - Assert.Equal(2, archive.Entries.Count); - VerifyValidEntry(archive.Entries[0], name0, ushort.MaxValue); - VerifyValidEntry(archive.Entries[1], name1, ushort.MaxValue); - } - - // Append 64KB of garbage - archiveStream.Seek(0, SeekOrigin.End); - archiveStream.Write(byte64KB, 0, byte64KB.Length); - - // Open should not be possible because we can't find the EOCD in the max search length from the end - Assert.Throws(() => - { - ZipArchive archive = new ZipArchive(archiveStream, ZipArchiveMode.Read, leaveOpen: true); - }); - - // Create stream with 64KB of prepended garbage, then the above stream appended - // Attempting to create a ZipArchive should fail: no EOCD found - using (MemoryStream prependStream = new MemoryStream()) - { - prependStream.Write(byte64KB, 0, byte64KB.Length); - archiveStream.WriteTo(prependStream); - - Assert.Throws(() => - { - ZipArchive archive = new ZipArchive(prependStream, ZipArchiveMode.Read); - }); - } - } - } } }