From 3b4966ccd3e85ffdda28062a91deab108ff7c41a Mon Sep 17 00:00:00 2001 From: Richard Webb <webby@beardmouse.co.uk> Date: Mon, 21 Jan 2019 11:49:25 +0000 Subject: [PATCH] Merge PR #302: Add a leaveOpen parameter to the ZipFile constructor Used to set the default value of isStreamOwner. * Add a leaveOpen parameter to the ZipFile constructor, used to set the default value of isStreamOwner * Add unit tests for the leaveOpen parameter on the ZipFile constructor --- src/ICSharpCode.SharpZipLib/Zip/ZipFile.cs | 49 +++- .../TestSupport/Streams.cs | 68 ++++++ .../Zip/ZipFileHandling.cs | 229 ++++++++++++++++++ 3 files changed, 342 insertions(+), 4 deletions(-) diff --git a/src/ICSharpCode.SharpZipLib/Zip/ZipFile.cs b/src/ICSharpCode.SharpZipLib/Zip/ZipFile.cs index fa4c09401..2d151a1bf 100644 --- a/src/ICSharpCode.SharpZipLib/Zip/ZipFile.cs +++ b/src/ICSharpCode.SharpZipLib/Zip/ZipFile.cs @@ -425,7 +425,25 @@ public ZipFile(string name) /// <exception cref="ZipException"> /// The file doesn't contain a valid zip archive. /// </exception> - public ZipFile(FileStream file) + public ZipFile(FileStream file) : + this(file, false) + { + + } + + /// <summary> + /// Opens a Zip file reading the given <see cref="FileStream"/>. + /// </summary> + /// <param name="file">The <see cref="FileStream"/> to read archive data from.</param> + /// <param name="leaveOpen">true to leave the <see cref="FileStream">file</see> open when the ZipFile is disposed, false to dispose of it</param> + /// <exception cref="ArgumentNullException">The supplied argument is null.</exception> + /// <exception cref="IOException"> + /// An i/o error occurs. + /// </exception> + /// <exception cref="ZipException"> + /// The file doesn't contain a valid zip archive. + /// </exception> + public ZipFile(FileStream file, bool leaveOpen) { if (file == null) { @@ -439,7 +457,7 @@ public ZipFile(FileStream file) baseStream_ = file; name_ = file.Name; - isStreamOwner = true; + isStreamOwner = !leaveOpen; try { @@ -468,7 +486,30 @@ public ZipFile(FileStream file) /// <exception cref="ArgumentNullException"> /// The <see cref="Stream">stream</see> argument is null. /// </exception> - public ZipFile(Stream stream) + public ZipFile(Stream stream) : + this(stream, false) + { + + } + + /// <summary> + /// Opens a Zip file reading the given <see cref="Stream"/>. + /// </summary> + /// <param name="stream">The <see cref="Stream"/> to read archive data from.</param> + /// <param name="leaveOpen">true to leave the <see cref="Stream">stream</see> open when the ZipFile is disposed, false to dispose of it</param> + /// <exception cref="IOException"> + /// An i/o error occurs + /// </exception> + /// <exception cref="ZipException"> + /// The stream doesn't contain a valid zip archive.<br/> + /// </exception> + /// <exception cref="ArgumentException"> + /// The <see cref="Stream">stream</see> doesnt support seeking. + /// </exception> + /// <exception cref="ArgumentNullException"> + /// The <see cref="Stream">stream</see> argument is null. + /// </exception> + public ZipFile(Stream stream, bool leaveOpen) { if (stream == null) { @@ -481,7 +522,7 @@ public ZipFile(Stream stream) } baseStream_ = stream; - isStreamOwner = true; + isStreamOwner = !leaveOpen; if (baseStream_.Length > 0) { diff --git a/test/ICSharpCode.SharpZipLib.Tests/TestSupport/Streams.cs b/test/ICSharpCode.SharpZipLib.Tests/TestSupport/Streams.cs index dd8dd1dd9..03eb42ce1 100644 --- a/test/ICSharpCode.SharpZipLib.Tests/TestSupport/Streams.cs +++ b/test/ICSharpCode.SharpZipLib.Tests/TestSupport/Streams.cs @@ -99,6 +99,74 @@ public bool IsDisposed #endregion Instance Fields } + /// <summary> + /// An extended <see cref="FileStream">file stream</see> + /// that tracks closing and disposing + /// </summary> + public class TrackedFileStream : FileStream + { + /// <summary> + /// Initializes a new instance of the <see cref="TrackedMemoryStream"/> class. + /// </summary> + /// <param name="buffer">The buffer.</param> + public TrackedFileStream(string path, FileMode mode = FileMode.Open, FileAccess access = FileAccess.Read) + : base(path, mode, access) + { + } + + /// <summary> + /// Releases the unmanaged resources used by the <see cref="T:System.IO.MemoryStream"/> class and optionally releases the managed resources. + /// </summary> + /// <param name="disposing">true to release both managed and unmanaged resources; false to release only unmanaged resources.</param> + protected override void Dispose(bool disposing) + { + isDisposed_ = true; + base.Dispose(disposing); + } + + /// <summary> + /// Closes the current stream and releases any resources (such as sockets and file handles) associated with the current stream. + /// </summary> + public override void Close() + { + if (isClosed_) + { + throw new InvalidOperationException("Already closed"); + } + + isClosed_ = true; + base.Close(); + } + + /// <summary> + /// Gets a value indicating whether this instance is closed. + /// </summary> + /// <value><c>true</c> if this instance is closed; otherwise, <c>false</c>.</value> + public bool IsClosed + { + get { return isClosed_; } + } + + /// <summary> + /// Gets a value indicating whether this instance is disposed. + /// </summary> + /// <value> + /// <c>true</c> if this instance is disposed; otherwise, <c>false</c>. + /// </value> + public bool IsDisposed + { + get { return isDisposed_; } + } + + #region Instance Fields + + private bool isDisposed_; + + private bool isClosed_; + + #endregion Instance Fields + } + /// <summary> /// A <see cref="Stream"/> that cannot seek. /// </summary> diff --git a/test/ICSharpCode.SharpZipLib.Tests/Zip/ZipFileHandling.cs b/test/ICSharpCode.SharpZipLib.Tests/Zip/ZipFileHandling.cs index 6cbcfd824..e3f108190 100644 --- a/test/ICSharpCode.SharpZipLib.Tests/Zip/ZipFileHandling.cs +++ b/test/ICSharpCode.SharpZipLib.Tests/Zip/ZipFileHandling.cs @@ -1138,5 +1138,234 @@ public void UnreferencedZipFileClosingPartialStream() s.ReadByte(); } + + /// <summary> + /// Check that input stream is closed when IsStreamOwner is true (default), or leaveOpen is false + /// </summary> + [Test] + [Category("Zip")] + public void StreamClosedWhenOwner() + { + var ms = new MemoryStream(); + MakeZipFile(ms, false, "StreamClosedWhenOwner", 1, 10, "test"); + ms.Seek(0, SeekOrigin.Begin); + var zipData = ms.ToArray(); + + // Stream should be closed when leaveOpen is unspecified + { + var inMemoryZip = new TrackedMemoryStream(zipData); + Assert.IsFalse(inMemoryZip.IsClosed, "Input stream should NOT be closed"); + + using (var zipFile = new ZipFile(inMemoryZip)) + { + Assert.IsTrue(zipFile.IsStreamOwner, "Should be stream owner by default"); + } + + Assert.IsTrue(inMemoryZip.IsClosed, "Input stream should be closed by default"); + } + + // Stream should be closed when leaveOpen is false + { + var inMemoryZip = new TrackedMemoryStream(zipData); + Assert.IsFalse(inMemoryZip.IsClosed, "Input stream should NOT be closed"); + + using (var zipFile = new ZipFile(inMemoryZip, false)) + { + Assert.IsTrue(zipFile.IsStreamOwner, "Should be stream owner when leaveOpen is false"); + } + + Assert.IsTrue(inMemoryZip.IsClosed, "Input stream should be closed when leaveOpen is false"); + } + } + + /// <summary> + /// Check that input stream is not closed when IsStreamOwner is false; + /// </summary> + [Test] + [Category("Zip")] + public void StreamNotClosedWhenNotOwner() + { + var ms = new TrackedMemoryStream(); + MakeZipFile(ms, false, "StreamNotClosedWhenNotOwner", 1, 10, "test"); + ms.Seek(0, SeekOrigin.Begin); + + Assert.IsFalse(ms.IsClosed, "Input stream should NOT be closed"); + + // Stream should not be closed when leaveOpen is true + { + using (var zipFile = new ZipFile(ms, true)) + { + Assert.IsFalse(zipFile.IsStreamOwner, "Should NOT be stream owner when leaveOpen is true"); + } + + Assert.IsFalse(ms.IsClosed, "Input stream should NOT be closed when leaveOpen is true"); + } + + ms.Seek(0, SeekOrigin.Begin); + + // Stream should not be closed when IsStreamOwner is set to false after opening + { + using (var zipFile = new ZipFile(ms, false)) + { + Assert.IsTrue(zipFile.IsStreamOwner, "Should be stream owner when leaveOpen is false"); + zipFile.IsStreamOwner = false; + Assert.IsFalse(zipFile.IsStreamOwner, "Should be able to set IsStreamOwner to false"); + } + + Assert.IsFalse(ms.IsClosed, "Input stream should NOT be closed when IsStreamOwner is false"); + } + } + + /// <summary> + /// Check that input file is closed when IsStreamOwner is true (default), or leaveOpen is false + /// </summary> + [Test] + [Category("Zip")] + public void FileStreamClosedWhenOwner() + { + string tempFile = GetTempFilePath(); + Assert.IsNotNull(tempFile, "No permission to execute this test?"); + + tempFile = Path.Combine(tempFile, "SharpZipFileStreamClosedWhenOwnerTest.Zip"); + if (File.Exists(tempFile)) + { + File.Delete(tempFile); + } + + MakeZipFile(tempFile, "FileStreamClosedWhenOwner", 2, 10, "test"); + + // Stream should be closed when leaveOpen is unspecified + { + var fileStream = new TrackedFileStream(tempFile); + Assert.IsFalse(fileStream.IsClosed, "Input file should NOT be closed"); + + using (var zipFile = new ZipFile(fileStream)) + { + Assert.IsTrue(zipFile.IsStreamOwner, "Should be stream owner by default"); + } + + Assert.IsTrue(fileStream.IsClosed, "Input stream should be closed by default"); + } + + // Stream should be closed when leaveOpen is false + { + var fileStream = new TrackedFileStream(tempFile); + Assert.IsFalse(fileStream.IsClosed, "Input stream should NOT be closed"); + + using (var zipFile = new ZipFile(fileStream, false)) + { + Assert.IsTrue(zipFile.IsStreamOwner, "Should be stream owner when leaveOpen is false"); + } + + Assert.IsTrue(fileStream.IsClosed, "Input stream should be closed when leaveOpen is false"); + } + + File.Delete(tempFile); + } + + /// <summary> + /// Check that input file is not closed when IsStreamOwner is false; + /// </summary> + [Test] + [Category("Zip")] + public void FileStreamNotClosedWhenNotOwner() + { + string tempFile = GetTempFilePath(); + Assert.IsNotNull(tempFile, "No permission to execute this test?"); + + tempFile = Path.Combine(tempFile, "SharpZipFileStreamNotClosedWhenNotOwner.Zip"); + if (File.Exists(tempFile)) + { + File.Delete(tempFile); + } + + MakeZipFile(tempFile, "FileStreamClosedWhenOwner", 2, 10, "test"); + + // Stream should not be closed when leaveOpen is true + { + using (var fileStream = new TrackedFileStream(tempFile)) + { + Assert.IsFalse(fileStream.IsClosed, "Input file should NOT be closed"); + + using (var zipFile = new ZipFile(fileStream, true)) + { + Assert.IsFalse(zipFile.IsStreamOwner, "Should NOT be stream owner when leaveOpen is true"); + } + + Assert.IsFalse(fileStream.IsClosed, "Input stream should NOT be closed when leaveOpen is true"); + } + } + + // Stream should not be closed when IsStreamOwner is set to false after opening + { + using (var fileStream = new TrackedFileStream(tempFile)) + { + Assert.IsFalse(fileStream.IsClosed, "Input file should NOT be closed"); + + using (var zipFile = new ZipFile(fileStream, false)) + { + Assert.IsTrue(zipFile.IsStreamOwner, "Should be stream owner when leaveOpen is false"); + zipFile.IsStreamOwner = false; + Assert.IsFalse(zipFile.IsStreamOwner, "Should be able to set IsStreamOwner to false"); + } + + Assert.IsFalse(fileStream.IsClosed, "Input stream should NOT be closed when leaveOpen is true"); + } + } + + File.Delete(tempFile); + } + + /// <summary> + /// Check that input stream is closed when construction fails and leaveOpen is false + /// </summary> + [Test] + public void StreamClosedOnError() + { + var ms = new TrackedMemoryStream(new byte[32]); + + Assert.IsFalse(ms.IsClosed, "Underlying stream should NOT be closed initially"); + bool blewUp = false; + try + { + using (var zipFile = new ZipFile(ms, false)) + { + Assert.Fail("Exception not thrown"); + } + } + catch + { + blewUp = true; + } + + Assert.IsTrue(blewUp, "Should have failed to load the file"); + Assert.IsTrue(ms.IsClosed, "Underlying stream should be closed"); + } + + /// <summary> + /// Check that input stream is not closed when construction fails and leaveOpen is true + /// </summary> + [Test] + public void StreamNotClosedOnError() + { + var ms = new TrackedMemoryStream(new byte[32]); + + Assert.IsFalse(ms.IsClosed, "Underlying stream should NOT be closed initially"); + bool blewUp = false; + try + { + using (var zipFile = new ZipFile(ms, true)) + { + Assert.Fail("Exception not thrown"); + } + } + catch + { + blewUp = true; + } + + Assert.IsTrue(blewUp, "Should have failed to load the file"); + Assert.IsFalse(ms.IsClosed, "Underlying stream should NOT be closed"); + } } }