From b62951e0c1f45c16defdedd1628f9dc85c24027b Mon Sep 17 00:00:00 2001 From: Richard Webb Date: Tue, 9 Jun 2020 19:02:59 +0100 Subject: [PATCH 1/6] Unit tests for using ZipFile to update file entries that have descriptors --- .../Zip/ZipFileHandling.cs | 83 +++++++++++++++++++ 1 file changed, 83 insertions(+) diff --git a/test/ICSharpCode.SharpZipLib.Tests/Zip/ZipFileHandling.cs b/test/ICSharpCode.SharpZipLib.Tests/Zip/ZipFileHandling.cs index 7ae234523..4bb429181 100644 --- a/test/ICSharpCode.SharpZipLib.Tests/Zip/ZipFileHandling.cs +++ b/test/ICSharpCode.SharpZipLib.Tests/Zip/ZipFileHandling.cs @@ -1611,5 +1611,88 @@ public void AddFileWithAlternateName() } } } + + /// + /// Test for https://github.com/icsharpcode/SharpZipLib/issues/147, when deleting items in a zip + /// + /// Whether Zip64 should be used in the test archive + [TestCase(UseZip64.On)] + [TestCase(UseZip64.Off)] + [Category("Zip")] + public void TestDescriptorUpdateOnDelete(UseZip64 useZip64) + { + MemoryStream msw = new MemoryStreamWithoutSeek(); + using (ZipOutputStream outStream = new ZipOutputStream(msw)) + { + outStream.UseZip64 = useZip64; + outStream.IsStreamOwner = false; + outStream.PutNextEntry(new ZipEntry("StripedMarlin")); + outStream.WriteByte(89); + + outStream.PutNextEntry(new ZipEntry("StripedMarlin2")); + outStream.WriteByte(91); + } + + var zipData = msw.ToArray(); + Assert.IsTrue(ZipTesting.TestArchive(zipData)); + + using (var memoryStream = new MemoryStream(zipData)) + { + using (var zipFile = new ZipFile(memoryStream, leaveOpen: true)) + { + zipFile.BeginUpdate(); + zipFile.Delete("StripedMarlin"); + zipFile.CommitUpdate(); + } + + memoryStream.Position = 0; + + using (var zipFile = new ZipFile(memoryStream, leaveOpen: true)) + { + Assert.That(zipFile.TestArchive(true), Is.True); + } + } + } + + /// + /// Test for https://github.com/icsharpcode/SharpZipLib/issues/147, when adding items to a zip + /// + /// Whether Zip64 should be used in the test archive + [TestCase(UseZip64.On)] + [TestCase(UseZip64.Off)] + [Category("Zip")] + public void TestDescriptorUpdateOnAdd(UseZip64 useZip64) + { + MemoryStream msw = new MemoryStreamWithoutSeek(); + using (ZipOutputStream outStream = new ZipOutputStream(msw)) + { + outStream.UseZip64 = useZip64; + outStream.IsStreamOwner = false; + outStream.PutNextEntry(new ZipEntry("StripedMarlin")); + outStream.WriteByte(89); + } + + var zipData = msw.ToArray(); + Assert.IsTrue(ZipTesting.TestArchive(zipData)); + + using (var memoryStream = new MemoryStream()) + { + memoryStream.Write(zipData, 0, zipData.Length); + + using (var zipFile = new ZipFile(memoryStream, leaveOpen: true)) + { + zipFile.BeginUpdate(); + zipFile.Add(new StringMemoryDataSource("stripey"), "Zebra"); + zipFile.CommitUpdate(); + } + + memoryStream.Position = 0; + + using (var zipFile = new ZipFile(memoryStream, leaveOpen: true)) + { + Assert.That(zipFile.TestArchive(true), Is.True); + } + } + } } } From f28a12e41619067acca7ae7db342f1a42de5e3d8 Mon Sep 17 00:00:00 2001 From: Richard Webb Date: Mon, 22 Jun 2020 22:47:20 +0100 Subject: [PATCH 2/6] fix size calculation in GetDescriptorSize --- src/ICSharpCode.SharpZipLib/Zip/ZipFile.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/ICSharpCode.SharpZipLib/Zip/ZipFile.cs b/src/ICSharpCode.SharpZipLib/Zip/ZipFile.cs index 02fd30778..64fb6c43c 100644 --- a/src/ICSharpCode.SharpZipLib/Zip/ZipFile.cs +++ b/src/ICSharpCode.SharpZipLib/Zip/ZipFile.cs @@ -2466,10 +2466,10 @@ private int GetDescriptorSize(ZipUpdate update) int result = 0; if ((update.Entry.Flags & (int)GeneralBitFlags.Descriptor) != 0) { - result = ZipConstants.DataDescriptorSize - 4; + result = ZipConstants.DataDescriptorSize; if (update.Entry.LocalHeaderRequiresZip64) { - result = ZipConstants.Zip64DataDescriptorSize - 4; + result = ZipConstants.Zip64DataDescriptorSize; } } return result; From 20e0a812d436543cd7eab4291ea3407dcc9a2f93 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?nils=20m=C3=A5s=C3=A9n?= Date: Tue, 11 Aug 2020 02:09:00 +0200 Subject: [PATCH 3/6] Handle optional descriptor signature when updating --- src/ICSharpCode.SharpZipLib/Zip/ZipFile.cs | 214 ++++++++++++--------- 1 file changed, 124 insertions(+), 90 deletions(-) diff --git a/src/ICSharpCode.SharpZipLib/Zip/ZipFile.cs b/src/ICSharpCode.SharpZipLib/Zip/ZipFile.cs index 64fb6c43c..585dfc076 100644 --- a/src/ICSharpCode.SharpZipLib/Zip/ZipFile.cs +++ b/src/ICSharpCode.SharpZipLib/Zip/ZipFile.cs @@ -367,10 +367,9 @@ public string Password } else { + rawPassword_ = value; key = PkzipClassic.GenerateKeys(ZipStrings.ConvertToArray(value)); } - - rawPassword_ = value; } } @@ -534,7 +533,7 @@ public ZipFile(Stream stream, bool leaveOpen) catch { DisposeInternal(true); - throw; + throw; } } else @@ -999,16 +998,19 @@ public bool TestArchive(bool testData, TestStrategy strategy, ZipTestResultHandl if (this[entryIndex].Crc != data.Crc) { status.AddError(); + resultHandler?.Invoke(status, "Descriptor CRC mismatch"); } if (this[entryIndex].CompressedSize != data.CompressedSize) { status.AddError(); + resultHandler?.Invoke(status, "Descriptor compressed size mismatch"); } if (this[entryIndex].Size != data.Size) { status.AddError(); + resultHandler?.Invoke(status, "Descriptor size mismatch"); } } } @@ -2105,7 +2107,7 @@ private void WriteLocalEntryHeader(ZipUpdate update) WriteLEShort(entry.Version); WriteLEShort(entry.Flags); - WriteLEShort((byte)entry.CompressionMethodForHeader); + WriteLEShort((byte)entry.CompressionMethod); WriteLEInt((int)entry.DosTime); if (!entry.HasCrc) @@ -2215,7 +2217,7 @@ private int WriteCentralDirectoryHeader(ZipEntry entry) unchecked { - WriteLEShort((byte)entry.CompressionMethodForHeader); + WriteLEShort((byte)entry.CompressionMethod); WriteLEInt((int)entry.DosTime); WriteLEInt((int)entry.Crc); } @@ -2382,26 +2384,37 @@ private byte[] GetBuffer() private void CopyDescriptorBytes(ZipUpdate update, Stream dest, Stream source) { - int bytesToCopy = GetDescriptorSize(update); + // Don't include the signature size to allow copy without seeking + var bytesToCopy = GetDescriptorSize(update, false); + + // Don't touch the source stream if no descriptor is present + if (bytesToCopy == 0) return; + + var buffer = GetBuffer(); - if (bytesToCopy > 0) + // Copy the first 4 bytes of the descriptor + source.Read(buffer, 0, sizeof(int)); + dest.Write(buffer, 0, sizeof(int)); + + if (BitConverter.ToUInt32(buffer, 0) != ZipConstants.DataDescriptorSignature) { - byte[] buffer = GetBuffer(); + // The initial bytes wasn't the descriptor, reduce the pending byte count + bytesToCopy -= buffer.Length; + } - while (bytesToCopy > 0) - { - int readSize = Math.Min(buffer.Length, bytesToCopy); + while (bytesToCopy > 0) + { + int readSize = Math.Min(buffer.Length, bytesToCopy); - int bytesRead = source.Read(buffer, 0, readSize); - if (bytesRead > 0) - { - dest.Write(buffer, 0, bytesRead); - bytesToCopy -= bytesRead; - } - else - { - throw new ZipException("Unxpected end of stream"); - } + int bytesRead = source.Read(buffer, 0, readSize); + if (bytesRead > 0) + { + dest.Write(buffer, 0, bytesRead); + bytesToCopy -= bytesRead; + } + else + { + throw new ZipException("Unxpected end of stream"); } } } @@ -2460,32 +2473,37 @@ private void CopyBytes(ZipUpdate update, Stream destination, Stream source, /// Get the size of the source descriptor for a . /// /// The update to get the size for. - /// The descriptor size, zero if there isnt one. - private int GetDescriptorSize(ZipUpdate update) + /// Whether to include the signature size + /// The descriptor size, zero if there isn't one. + private int GetDescriptorSize(ZipUpdate update, bool includingSignature) { - int result = 0; - if ((update.Entry.Flags & (int)GeneralBitFlags.Descriptor) != 0) - { - result = ZipConstants.DataDescriptorSize; - if (update.Entry.LocalHeaderRequiresZip64) - { - result = ZipConstants.Zip64DataDescriptorSize; - } - } - return result; + if (!((GeneralBitFlags)update.Entry.Flags).HasFlag(GeneralBitFlags.Descriptor)) + return 0; + + var descriptorWithSignature = update.Entry.LocalHeaderRequiresZip64 + ? ZipConstants.Zip64DataDescriptorSize + : ZipConstants.DataDescriptorSize; + + return includingSignature + ? descriptorWithSignature + : descriptorWithSignature - sizeof(int); } private void CopyDescriptorBytesDirect(ZipUpdate update, Stream stream, ref long destinationPosition, long sourcePosition) { - int bytesToCopy = GetDescriptorSize(update); + var buffer = GetBuffer(); ; + + stream.Position = sourcePosition; + stream.Read(buffer, 0, sizeof(int)); + var sourceHasSignature = BitConverter.ToUInt32(buffer, 0) == ZipConstants.DataDescriptorSignature; + + var bytesToCopy = GetDescriptorSize(update, sourceHasSignature); while (bytesToCopy > 0) { - var readSize = (int)bytesToCopy; - byte[] buffer = GetBuffer(); - stream.Position = sourcePosition; - int bytesRead = stream.Read(buffer, 0, readSize); + + var bytesRead = stream.Read(buffer, 0, bytesToCopy); if (bytesRead > 0) { stream.Position = destinationPosition; @@ -2496,7 +2514,7 @@ private void CopyDescriptorBytesDirect(ZipUpdate update, Stream stream, ref long } else { - throw new ZipException("Unxpected end of stream"); + throw new ZipException("Unexpected end of stream"); } } } @@ -2743,6 +2761,7 @@ private void CopyEntryDirect(ZipFile workFile, ZipUpdate update, ref long destin // Clumsy way of handling retrieving the original name and extra data length for now. // TODO: Stop re-reading name and data length in CopyEntryDirect. + uint nameLength = ReadLEUshort(); uint extraLength = ReadLEUshort(); @@ -2751,14 +2770,29 @@ private void CopyEntryDirect(ZipFile workFile, ZipUpdate update, ref long destin if (skipOver) { if (update.OffsetBasedSize != -1) + { destinationPosition += update.OffsetBasedSize; + } else + { // TODO: Find out why this calculation comes up 4 bytes short on some entries in ODT (Office Document Text) archives. // WinZip produces a warning on these entries: // "caution: value of lrec.csize (compressed size) changed from ..." - destinationPosition += - (sourcePosition - entryDataOffset) + NameLengthOffset + // Header size - update.Entry.CompressedSize + GetDescriptorSize(update); + + // Skip entry header + destinationPosition += (sourcePosition - entryDataOffset) + NameLengthOffset; + + // Skip entry compressed data + destinationPosition += update.Entry.CompressedSize; + + // Seek to end of entry to check for descriptor signature + baseStream_.Seek(destinationPosition, SeekOrigin.Begin); + + var descriptorHasSignature = ReadLEUint() == ZipConstants.DataDescriptorSignature; + + // Skip descriptor and it's signature (if present) + destinationPosition += GetDescriptorSize(update, descriptorHasSignature); + } } else { @@ -3616,9 +3650,9 @@ private Stream CreateAndInitDecryptionStream(Stream baseStream, ZipEntry entry) { if (entry.Version >= ZipConstants.VERSION_AES) { - // Issue #471 - accept an empty string as a password, but reject null. + // OnKeysRequired(entry.Name); - if (rawPassword_ == null) + if (HaveKeys == false) { throw new ZipException("No password available for AES encrypted stream"); } @@ -4007,12 +4041,12 @@ public override long Position /// /// The total number of bytes read into the buffer. This can be less than the number of bytes requested if that many bytes are not currently available, or zero (0) if the end of the stream has been reached. /// - /// The sum of offset and count is larger than the buffer length. - /// Methods were called after the stream was closed. - /// The stream does not support reading. - /// buffer is null. - /// An I/O error occurs. - /// offset or count is negative. + /// The sum of offset and count is larger than the buffer length. + /// Methods were called after the stream was closed. + /// The stream does not support reading. + /// buffer is null. + /// An I/O error occurs. + /// offset or count is negative. public override int Read(byte[] buffer, int offset, int count) { return 0; @@ -4022,13 +4056,13 @@ public override int Read(byte[] buffer, int offset, int count) /// Sets the position within the current stream. /// /// A byte offset relative to the origin parameter. - /// A value of type indicating the reference point used to obtain the new position. + /// A value of type indicating the reference point used to obtain the new position. /// /// The new position within the current stream. /// - /// An I/O error occurs. - /// The stream does not support seeking, such as if the stream is constructed from a pipe or console output. - /// Methods were called after the stream was closed. + /// An I/O error occurs. + /// The stream does not support seeking, such as if the stream is constructed from a pipe or console output. + /// Methods were called after the stream was closed. public override long Seek(long offset, SeekOrigin origin) { return 0; @@ -4038,9 +4072,9 @@ public override long Seek(long offset, SeekOrigin origin) /// Sets the length of the current stream. /// /// The desired length of the current stream in bytes. - /// The stream does not support both writing and seeking, such as if the stream is constructed from a pipe or console output. - /// An I/O error occurs. - /// Methods were called after the stream was closed. + /// The stream does not support both writing and seeking, such as if the stream is constructed from a pipe or console output. + /// An I/O error occurs. + /// Methods were called after the stream was closed. public override void SetLength(long value) { } @@ -4051,12 +4085,12 @@ public override void SetLength(long value) /// An array of bytes. This method copies count bytes from buffer to the current stream. /// The zero-based byte offset in buffer at which to begin copying bytes to the current stream. /// The number of bytes to be written to the current stream. - /// An I/O error occurs. - /// The stream does not support writing. - /// Methods were called after the stream was closed. - /// buffer is null. - /// The sum of offset and count is greater than the buffer length. - /// offset or count is negative. + /// An I/O error occurs. + /// The stream does not support writing. + /// Methods were called after the stream was closed. + /// buffer is null. + /// The sum of offset and count is greater than the buffer length. + /// offset or count is negative. public override void Write(byte[] buffer, int offset, int count) { baseStream_.Write(buffer, offset, count); @@ -4136,12 +4170,12 @@ public override int ReadByte() /// /// The total number of bytes read into the buffer. This can be less than the number of bytes requested if that many bytes are not currently available, or zero (0) if the end of the stream has been reached. /// - /// The sum of offset and count is larger than the buffer length. - /// Methods were called after the stream was closed. - /// The stream does not support reading. - /// buffer is null. - /// An I/O error occurs. - /// offset or count is negative. + /// The sum of offset and count is larger than the buffer length. + /// Methods were called after the stream was closed. + /// The stream does not support reading. + /// buffer is null. + /// An I/O error occurs. + /// offset or count is negative. public override int Read(byte[] buffer, int offset, int count) { lock (baseStream_) @@ -4175,12 +4209,12 @@ public override int Read(byte[] buffer, int offset, int count) /// An array of bytes. This method copies count bytes from buffer to the current stream. /// The zero-based byte offset in buffer at which to begin copying bytes to the current stream. /// The number of bytes to be written to the current stream. - /// An I/O error occurs. - /// The stream does not support writing. - /// Methods were called after the stream was closed. - /// buffer is null. - /// The sum of offset and count is greater than the buffer length. - /// offset or count is negative. + /// An I/O error occurs. + /// The stream does not support writing. + /// Methods were called after the stream was closed. + /// buffer is null. + /// The sum of offset and count is greater than the buffer length. + /// offset or count is negative. public override void Write(byte[] buffer, int offset, int count) { throw new NotSupportedException(); @@ -4190,9 +4224,9 @@ public override void Write(byte[] buffer, int offset, int count) /// When overridden in a derived class, sets the length of the current stream. /// /// The desired length of the current stream in bytes. - /// The stream does not support both writing and seeking, such as if the stream is constructed from a pipe or console output. - /// An I/O error occurs. - /// Methods were called after the stream was closed. + /// The stream does not support both writing and seeking, such as if the stream is constructed from a pipe or console output. + /// An I/O error occurs. + /// Methods were called after the stream was closed. public override void SetLength(long value) { throw new NotSupportedException(); @@ -4202,13 +4236,13 @@ public override void SetLength(long value) /// When overridden in a derived class, sets the position within the current stream. /// /// A byte offset relative to the origin parameter. - /// A value of type indicating the reference point used to obtain the new position. + /// A value of type indicating the reference point used to obtain the new position. /// /// The new position within the current stream. /// - /// An I/O error occurs. - /// The stream does not support seeking, such as if the stream is constructed from a pipe or console output. - /// Methods were called after the stream was closed. + /// An I/O error occurs. + /// The stream does not support seeking, such as if the stream is constructed from a pipe or console output. + /// Methods were called after the stream was closed. public override long Seek(long offset, SeekOrigin origin) { long newPos = readPos_; @@ -4233,7 +4267,7 @@ public override long Seek(long offset, SeekOrigin origin) throw new ArgumentException("Negative position is invalid"); } - if (newPos > end_) + if (newPos >= end_) { throw new IOException("Cannot seek past end"); } @@ -4244,7 +4278,7 @@ public override long Seek(long offset, SeekOrigin origin) /// /// Clears all buffers for this stream and causes any buffered data to be written to the underlying device. /// - /// An I/O error occurs. + /// An I/O error occurs. public override void Flush() { // Nothing to do. @@ -4255,9 +4289,9 @@ public override void Flush() /// /// /// The current position within the stream. - /// An I/O error occurs. - /// The stream does not support seeking. - /// Methods were called after the stream was closed. + /// An I/O error occurs. + /// The stream does not support seeking. + /// Methods were called after the stream was closed. public override long Position { get { return readPos_ - start_; } @@ -4270,7 +4304,7 @@ public override long Position throw new ArgumentException("Negative position is invalid"); } - if (newPos > end_) + if (newPos >= end_) { throw new InvalidOperationException("Cannot seek past end"); } @@ -4283,8 +4317,8 @@ public override long Position /// /// /// A long value representing the length of the stream in bytes. - /// A class derived from Stream does not support seeking. - /// Methods were called after the stream was closed. + /// A class derived from Stream does not support seeking. + /// Methods were called after the stream was closed. public override long Length { get { return length_; } From 5c09afb9f9dba8261e86140eae4e0c4b7b698fa3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?nils=20m=C3=A5s=C3=A9n?= Date: Tue, 11 Aug 2020 02:20:27 +0200 Subject: [PATCH 4/6] Handle optional descriptor signature when updating Without reverting unrelated code this time hopefully --- src/ICSharpCode.SharpZipLib/Zip/ZipFile.cs | 111 ++++++++++----------- 1 file changed, 53 insertions(+), 58 deletions(-) diff --git a/src/ICSharpCode.SharpZipLib/Zip/ZipFile.cs b/src/ICSharpCode.SharpZipLib/Zip/ZipFile.cs index 585dfc076..d8fcd3606 100644 --- a/src/ICSharpCode.SharpZipLib/Zip/ZipFile.cs +++ b/src/ICSharpCode.SharpZipLib/Zip/ZipFile.cs @@ -367,9 +367,10 @@ public string Password } else { - rawPassword_ = value; key = PkzipClassic.GenerateKeys(ZipStrings.ConvertToArray(value)); } + + rawPassword_ = value; } } @@ -533,7 +534,7 @@ public ZipFile(Stream stream, bool leaveOpen) catch { DisposeInternal(true); - throw; + throw; } } else @@ -1919,11 +1920,9 @@ public void Modify(ZipEntry original, ZipEntry updated) if ( original == null ) { throw new ArgumentNullException("original"); } - if ( updated == null ) { throw new ArgumentNullException("updated"); } - CheckUpdating(); contentsEdited_ = true; updates_.Add(new ZipUpdate(original, updated)); @@ -2107,7 +2106,7 @@ private void WriteLocalEntryHeader(ZipUpdate update) WriteLEShort(entry.Version); WriteLEShort(entry.Flags); - WriteLEShort((byte)entry.CompressionMethod); + WriteLEShort((byte)entry.CompressionMethodForHeader); WriteLEInt((int)entry.DosTime); if (!entry.HasCrc) @@ -2217,7 +2216,7 @@ private int WriteCentralDirectoryHeader(ZipEntry entry) unchecked { - WriteLEShort((byte)entry.CompressionMethod); + WriteLEShort((byte)entry.CompressionMethodForHeader); WriteLEInt((int)entry.DosTime); WriteLEInt((int)entry.Crc); } @@ -2775,10 +2774,6 @@ private void CopyEntryDirect(ZipFile workFile, ZipUpdate update, ref long destin } else { - // TODO: Find out why this calculation comes up 4 bytes short on some entries in ODT (Office Document Text) archives. - // WinZip produces a warning on these entries: - // "caution: value of lrec.csize (compressed size) changed from ..." - // Skip entry header destinationPosition += (sourcePosition - entryDataOffset) + NameLengthOffset; @@ -3650,9 +3645,9 @@ private Stream CreateAndInitDecryptionStream(Stream baseStream, ZipEntry entry) { if (entry.Version >= ZipConstants.VERSION_AES) { - // + // Issue #471 - accept an empty string as a password, but reject null. OnKeysRequired(entry.Name); - if (HaveKeys == false) + if (rawPassword_ == null) { throw new ZipException("No password available for AES encrypted stream"); } @@ -4041,12 +4036,12 @@ public override long Position /// /// The total number of bytes read into the buffer. This can be less than the number of bytes requested if that many bytes are not currently available, or zero (0) if the end of the stream has been reached. /// - /// The sum of offset and count is larger than the buffer length. - /// Methods were called after the stream was closed. - /// The stream does not support reading. - /// buffer is null. - /// An I/O error occurs. - /// offset or count is negative. + /// The sum of offset and count is larger than the buffer length. + /// Methods were called after the stream was closed. + /// The stream does not support reading. + /// buffer is null. + /// An I/O error occurs. + /// offset or count is negative. public override int Read(byte[] buffer, int offset, int count) { return 0; @@ -4056,13 +4051,13 @@ public override int Read(byte[] buffer, int offset, int count) /// Sets the position within the current stream. /// /// A byte offset relative to the origin parameter. - /// A value of type indicating the reference point used to obtain the new position. + /// A value of type indicating the reference point used to obtain the new position. /// /// The new position within the current stream. /// - /// An I/O error occurs. - /// The stream does not support seeking, such as if the stream is constructed from a pipe or console output. - /// Methods were called after the stream was closed. + /// An I/O error occurs. + /// The stream does not support seeking, such as if the stream is constructed from a pipe or console output. + /// Methods were called after the stream was closed. public override long Seek(long offset, SeekOrigin origin) { return 0; @@ -4072,9 +4067,9 @@ public override long Seek(long offset, SeekOrigin origin) /// Sets the length of the current stream. /// /// The desired length of the current stream in bytes. - /// The stream does not support both writing and seeking, such as if the stream is constructed from a pipe or console output. - /// An I/O error occurs. - /// Methods were called after the stream was closed. + /// The stream does not support both writing and seeking, such as if the stream is constructed from a pipe or console output. + /// An I/O error occurs. + /// Methods were called after the stream was closed. public override void SetLength(long value) { } @@ -4085,12 +4080,12 @@ public override void SetLength(long value) /// An array of bytes. This method copies count bytes from buffer to the current stream. /// The zero-based byte offset in buffer at which to begin copying bytes to the current stream. /// The number of bytes to be written to the current stream. - /// An I/O error occurs. - /// The stream does not support writing. - /// Methods were called after the stream was closed. - /// buffer is null. - /// The sum of offset and count is greater than the buffer length. - /// offset or count is negative. + /// An I/O error occurs. + /// The stream does not support writing. + /// Methods were called after the stream was closed. + /// buffer is null. + /// The sum of offset and count is greater than the buffer length. + /// offset or count is negative. public override void Write(byte[] buffer, int offset, int count) { baseStream_.Write(buffer, offset, count); @@ -4170,12 +4165,12 @@ public override int ReadByte() /// /// The total number of bytes read into the buffer. This can be less than the number of bytes requested if that many bytes are not currently available, or zero (0) if the end of the stream has been reached. /// - /// The sum of offset and count is larger than the buffer length. - /// Methods were called after the stream was closed. - /// The stream does not support reading. - /// buffer is null. - /// An I/O error occurs. - /// offset or count is negative. + /// The sum of offset and count is larger than the buffer length. + /// Methods were called after the stream was closed. + /// The stream does not support reading. + /// buffer is null. + /// An I/O error occurs. + /// offset or count is negative. public override int Read(byte[] buffer, int offset, int count) { lock (baseStream_) @@ -4209,12 +4204,12 @@ public override int Read(byte[] buffer, int offset, int count) /// An array of bytes. This method copies count bytes from buffer to the current stream. /// The zero-based byte offset in buffer at which to begin copying bytes to the current stream. /// The number of bytes to be written to the current stream. - /// An I/O error occurs. - /// The stream does not support writing. - /// Methods were called after the stream was closed. - /// buffer is null. - /// The sum of offset and count is greater than the buffer length. - /// offset or count is negative. + /// An I/O error occurs. + /// The stream does not support writing. + /// Methods were called after the stream was closed. + /// buffer is null. + /// The sum of offset and count is greater than the buffer length. + /// offset or count is negative. public override void Write(byte[] buffer, int offset, int count) { throw new NotSupportedException(); @@ -4224,9 +4219,9 @@ public override void Write(byte[] buffer, int offset, int count) /// When overridden in a derived class, sets the length of the current stream. /// /// The desired length of the current stream in bytes. - /// The stream does not support both writing and seeking, such as if the stream is constructed from a pipe or console output. - /// An I/O error occurs. - /// Methods were called after the stream was closed. + /// The stream does not support both writing and seeking, such as if the stream is constructed from a pipe or console output. + /// An I/O error occurs. + /// Methods were called after the stream was closed. public override void SetLength(long value) { throw new NotSupportedException(); @@ -4236,13 +4231,13 @@ public override void SetLength(long value) /// When overridden in a derived class, sets the position within the current stream. /// /// A byte offset relative to the origin parameter. - /// A value of type indicating the reference point used to obtain the new position. + /// A value of type indicating the reference point used to obtain the new position. /// /// The new position within the current stream. /// - /// An I/O error occurs. - /// The stream does not support seeking, such as if the stream is constructed from a pipe or console output. - /// Methods were called after the stream was closed. + /// An I/O error occurs. + /// The stream does not support seeking, such as if the stream is constructed from a pipe or console output. + /// Methods were called after the stream was closed. public override long Seek(long offset, SeekOrigin origin) { long newPos = readPos_; @@ -4267,7 +4262,7 @@ public override long Seek(long offset, SeekOrigin origin) throw new ArgumentException("Negative position is invalid"); } - if (newPos >= end_) + if (newPos > end_) { throw new IOException("Cannot seek past end"); } @@ -4278,7 +4273,7 @@ public override long Seek(long offset, SeekOrigin origin) /// /// Clears all buffers for this stream and causes any buffered data to be written to the underlying device. /// - /// An I/O error occurs. + /// An I/O error occurs. public override void Flush() { // Nothing to do. @@ -4289,9 +4284,9 @@ public override void Flush() /// /// /// The current position within the stream. - /// An I/O error occurs. - /// The stream does not support seeking. - /// Methods were called after the stream was closed. + /// An I/O error occurs. + /// The stream does not support seeking. + /// Methods were called after the stream was closed. public override long Position { get { return readPos_ - start_; } @@ -4304,7 +4299,7 @@ public override long Position throw new ArgumentException("Negative position is invalid"); } - if (newPos >= end_) + if (newPos > end_) { throw new InvalidOperationException("Cannot seek past end"); } @@ -4317,8 +4312,8 @@ public override long Position /// /// /// A long value representing the length of the stream in bytes. - /// A class derived from Stream does not support seeking. - /// Methods were called after the stream was closed. + /// A class derived from Stream does not support seeking. + /// Methods were called after the stream was closed. public override long Length { get { return length_; } From 3b33af9677aaf35e9ec64603991306349acc5fba Mon Sep 17 00:00:00 2001 From: Richard Webb Date: Wed, 19 Aug 2020 23:21:18 +0100 Subject: [PATCH 5/6] Remove descriptors from zip entries when moving them during updates. --- src/ICSharpCode.SharpZipLib/Zip/ZipFile.cs | 10 ++- .../Zip/ZipFileHandling.cs | 76 +++++++++++++++---- 2 files changed, 70 insertions(+), 16 deletions(-) diff --git a/src/ICSharpCode.SharpZipLib/Zip/ZipFile.cs b/src/ICSharpCode.SharpZipLib/Zip/ZipFile.cs index d8fcd3606..5ae35f096 100644 --- a/src/ICSharpCode.SharpZipLib/Zip/ZipFile.cs +++ b/src/ICSharpCode.SharpZipLib/Zip/ZipFile.cs @@ -2050,8 +2050,14 @@ private void WriteLocalEntryHeader(ZipUpdate update) // TODO: Local offset will require adjusting for multi-disk zip files. entry.Offset = baseStream_.Position; - // TODO: Need to clear any entry flags that dont make sense or throw an exception here. - if (update.Command != UpdateCommand.Copy) + // set flags based on the type of entry being written + if (update.Command == UpdateCommand.Copy) + { + // We're copying an existing entry during an update. + // In this case, we won't need descriptors as we should always know the size/CRC fields of the entry directly + entry.Flags &= ~(int)GeneralBitFlags.Descriptor; + } + else { if (entry.CompressionMethod == CompressionMethod.Deflated) { diff --git a/test/ICSharpCode.SharpZipLib.Tests/Zip/ZipFileHandling.cs b/test/ICSharpCode.SharpZipLib.Tests/Zip/ZipFileHandling.cs index 4bb429181..3e150d6d3 100644 --- a/test/ICSharpCode.SharpZipLib.Tests/Zip/ZipFileHandling.cs +++ b/test/ICSharpCode.SharpZipLib.Tests/Zip/ZipFileHandling.cs @@ -1616,10 +1616,10 @@ public void AddFileWithAlternateName() /// Test for https://github.com/icsharpcode/SharpZipLib/issues/147, when deleting items in a zip /// /// Whether Zip64 should be used in the test archive - [TestCase(UseZip64.On)] - [TestCase(UseZip64.Off)] + /// Whether safe or direct updates should be used + [Test] [Category("Zip")] - public void TestDescriptorUpdateOnDelete(UseZip64 useZip64) + public void TestDescriptorUpdateOnDelete([Values] UseZip64 useZip64, [Values] FileUpdateMode updateMode) { MemoryStream msw = new MemoryStreamWithoutSeek(); using (ZipOutputStream outStream = new ZipOutputStream(msw)) @@ -1640,15 +1640,10 @@ public void TestDescriptorUpdateOnDelete(UseZip64 useZip64) { using (var zipFile = new ZipFile(memoryStream, leaveOpen: true)) { - zipFile.BeginUpdate(); + zipFile.BeginUpdate(new MemoryArchiveStorage(updateMode)); zipFile.Delete("StripedMarlin"); zipFile.CommitUpdate(); - } - - memoryStream.Position = 0; - using (var zipFile = new ZipFile(memoryStream, leaveOpen: true)) - { Assert.That(zipFile.TestArchive(true), Is.True); } } @@ -1658,10 +1653,10 @@ public void TestDescriptorUpdateOnDelete(UseZip64 useZip64) /// Test for https://github.com/icsharpcode/SharpZipLib/issues/147, when adding items to a zip /// /// Whether Zip64 should be used in the test archive - [TestCase(UseZip64.On)] - [TestCase(UseZip64.Off)] + /// Whether safe or direct updates should be used + [Test] [Category("Zip")] - public void TestDescriptorUpdateOnAdd(UseZip64 useZip64) + public void TestDescriptorUpdateOnAdd([Values] UseZip64 useZip64, [Values] FileUpdateMode updateMode) { MemoryStream msw = new MemoryStreamWithoutSeek(); using (ZipOutputStream outStream = new ZipOutputStream(msw)) @@ -1681,16 +1676,69 @@ public void TestDescriptorUpdateOnAdd(UseZip64 useZip64) using (var zipFile = new ZipFile(memoryStream, leaveOpen: true)) { - zipFile.BeginUpdate(); + zipFile.BeginUpdate(new MemoryArchiveStorage(updateMode)); zipFile.Add(new StringMemoryDataSource("stripey"), "Zebra"); zipFile.CommitUpdate(); + + Assert.That(zipFile.TestArchive(true), Is.True); } + } + } + + /// + /// Test for https://github.com/icsharpcode/SharpZipLib/issues/147, when replacing items in a zip + /// + /// Whether Zip64 should be used in the test archive + /// Whether safe or direct updates should be used + [Test] + [Category("Zip")] + public void TestDescriptorUpdateOnReplacee([Values] UseZip64 useZip64, [Values] FileUpdateMode updateMode) + { + MemoryStream msw = new MemoryStreamWithoutSeek(); + using (ZipOutputStream outStream = new ZipOutputStream(msw)) + { + outStream.UseZip64 = useZip64; + outStream.IsStreamOwner = false; + outStream.PutNextEntry(new ZipEntry("StripedMarlin")); + outStream.WriteByte(89); + outStream.PutNextEntry(new ZipEntry("WhiteMarlin")); + outStream.WriteByte(91); + outStream.PutNextEntry(new ZipEntry("Sailfish")); + outStream.WriteByte(3); + } - memoryStream.Position = 0; + var zipData = msw.ToArray(); + Assert.IsTrue(ZipTesting.TestArchive(zipData)); + using (var memoryStream = new MemoryStream(zipData)) + { using (var zipFile = new ZipFile(memoryStream, leaveOpen: true)) { + zipFile.BeginUpdate(new MemoryArchiveStorage(updateMode)); + zipFile.Delete("WhiteMarlin"); + zipFile.Add(new StringMemoryDataSource("Kajikia albida"), "WhiteMarlin"); + zipFile.CommitUpdate(); + Assert.That(zipFile.TestArchive(true), Is.True); + Assert.That(zipFile.Count, Is.EqualTo(3)); + + // Test for expected descriptor states: + // 'StripedMarlin' should still have a descriptor in Direct update mode as the entry data will be kept, but won't have one + // in Safe update mode as that recreates the whole archive. + // 'WhiteMarlin' should no longer have one because the entry is new and didn't need one + // 'Sailfish' should have its descriptor removed. + var entry = zipFile.GetEntry("StripedMarlin"); + + if (updateMode == FileUpdateMode.Direct) + Assert.True(((GeneralBitFlags)entry.Flags).HasFlag(GeneralBitFlags.Descriptor)); + else + Assert.False(((GeneralBitFlags)entry.Flags).HasFlag(GeneralBitFlags.Descriptor)); + + entry = zipFile.GetEntry("WhiteMarlin"); + Assert.False(((GeneralBitFlags)entry.Flags).HasFlag(GeneralBitFlags.Descriptor)); + + entry = zipFile.GetEntry("Sailfish"); + Assert.False(((GeneralBitFlags)entry.Flags).HasFlag(GeneralBitFlags.Descriptor)); } } } From a3bed391da11863277a597f6ae9ce2a9b8c20f83 Mon Sep 17 00:00:00 2001 From: Richard Webb Date: Thu, 20 Aug 2020 00:02:06 +0100 Subject: [PATCH 6/6] Extra test for updating zips whose entries have descriptors with no signature bytes --- .../Zip/ZipFileHandling.cs | 60 ++++++++++++++++++- 1 file changed, 59 insertions(+), 1 deletion(-) diff --git a/test/ICSharpCode.SharpZipLib.Tests/Zip/ZipFileHandling.cs b/test/ICSharpCode.SharpZipLib.Tests/Zip/ZipFileHandling.cs index 3e150d6d3..e053bed04 100644 --- a/test/ICSharpCode.SharpZipLib.Tests/Zip/ZipFileHandling.cs +++ b/test/ICSharpCode.SharpZipLib.Tests/Zip/ZipFileHandling.cs @@ -1692,7 +1692,7 @@ public void TestDescriptorUpdateOnAdd([Values] UseZip64 useZip64, [Values] FileU /// Whether safe or direct updates should be used [Test] [Category("Zip")] - public void TestDescriptorUpdateOnReplacee([Values] UseZip64 useZip64, [Values] FileUpdateMode updateMode) + public void TestDescriptorUpdateOnReplace([Values] UseZip64 useZip64, [Values] FileUpdateMode updateMode) { MemoryStream msw = new MemoryStreamWithoutSeek(); using (ZipOutputStream outStream = new ZipOutputStream(msw)) @@ -1742,5 +1742,63 @@ public void TestDescriptorUpdateOnReplacee([Values] UseZip64 useZip64, [Values] } } } + + // This is the initial zipfile generated by the 'TestDescriptorUpdateOnReplace' test, but with descriptor + // fields that don't have the signature bytes. + const string TestZipFileWithSignaturelessDescriptors = @"UEsDBBQACAAIAHO8E1EAAAAAAAAAAAAAAAANAAAA + U3RyaXBlZE1hcmxpbosEAN0GtcADAAAAAQAAAFBLAwQUAAgACABzvBNRAAAAAAAAAAAAAAAACwAAAFdoaXRlTWFybGlui + wYA8We7LgMAAAABAAAAUEsDBBQACAAIAHO8E1EAAAAAAAAAAAAAAAAIAAAAU2FpbGZpc2hjBgA3vgtLAwAAAAEAAABQSw + ECMwAUAAgACABzvBNR3Qa1wAMAAAABAAAADQAAAAAAAAAAAAAAAAAAAAAAU3RyaXBlZE1hcmxpblBLAQIzABQACAAIAHO + 8E1HxZ7suAwAAAAEAAAALAAAAAAAAAAAAAAAAADoAAABXaGl0ZU1hcmxpblBLAQIzABQACAAIAHO8E1E3vgtLAwAAAAEA + AAAIAAAAAAAAAAAAAAAAAHIAAABTYWlsZmlzaFBLBQYAAAAAAwADAKoAAACnAAAAAAA="; + + /// + /// Test for https://github.com/icsharpcode/SharpZipLib/issues/147, when replacing items in a zip, using a file whose descriptors + /// don't have signature bytes + /// + /// Whether Zip64 should be used in the test archive + /// Whether safe or direct updates should be used + [Test] + [Category("Zip")] + public void TestSignaturelessDescriptorUpdateOnReplace([Values] UseZip64 useZip64, [Values] FileUpdateMode updateMode) + { + var fileBytes = Convert.FromBase64String(TestZipFileWithSignaturelessDescriptors); + + using (var memoryStream = new MemoryStream()) + { + memoryStream.Write(fileBytes, 0, fileBytes.Length); + memoryStream.Position = 0; + + using (var zipFile = new ZipFile(memoryStream, leaveOpen: true)) + { + zipFile.BeginUpdate(new MemoryArchiveStorage(updateMode)); + zipFile.Delete("WhiteMarlin"); + zipFile.Add(new StringMemoryDataSource("Kajikia albida"), "WhiteMarlin"); + zipFile.CommitUpdate(); + + // @@NOTE@@ TestArchive fails if an entry has a descriptor with no signature. + // Assert.That(zipFile.TestArchive(true), Is.True); + Assert.That(zipFile.Count, Is.EqualTo(3)); + + // Test for expected descriptor states: + // 'StripedMarlin' should still have a descriptor in Direct update mode as the entry data will be kept, but won't have one + // in Safe update mode as that recreates the whole archive. + // 'WhiteMarlin' should no longer have one because the entry is new and didn't need one + // 'Sailfish' should have its descriptor removed. + var entry = zipFile.GetEntry("StripedMarlin"); + + if (updateMode == FileUpdateMode.Direct) + Assert.True(((GeneralBitFlags)entry.Flags).HasFlag(GeneralBitFlags.Descriptor)); + else + Assert.False(((GeneralBitFlags)entry.Flags).HasFlag(GeneralBitFlags.Descriptor)); + + entry = zipFile.GetEntry("WhiteMarlin"); + Assert.False(((GeneralBitFlags)entry.Flags).HasFlag(GeneralBitFlags.Descriptor)); + + entry = zipFile.GetEntry("Sailfish"); + Assert.False(((GeneralBitFlags)entry.Flags).HasFlag(GeneralBitFlags.Descriptor)); + } + } + } } }